jpountz commented on code in PR #13254:
URL: https://github.com/apache/lucene/pull/13254#discussion_r1547286138
##########
lucene/core/src/java/org/apache/lucene/codecs/CompetitiveImpactAccumulator.java:
##########
@@ -107,26 +107,30 @@ public void copy(CompetitiveImpactAccumulator acc) {
/** Get the set of competitive freq and norm pairs, ordered by increasing
freq and norm. */
public Collection<Impact> getCompetitiveFreqNormPairs() {
- List<Impact> impacts = new ArrayList<>();
- int maxFreqForLowerNorms = 0;
- for (int i = 0; i < maxFreqs.length; ++i) {
- int maxFreq = maxFreqs[i];
- if (maxFreq > maxFreqForLowerNorms) {
- impacts.add(new Impact(maxFreq, (byte) i));
- maxFreqForLowerNorms = maxFreq;
- }
- }
-
if (otherFreqNormPairs.isEmpty()) {
+ List<Impact> impacts = new ArrayList<>();
+ int maxFreqForLowerNorms = 0;
+ for (int i = 0; i < maxFreqs.length; ++i) {
+ int maxFreq = maxFreqs[i];
+ if (maxFreq > maxFreqForLowerNorms) {
+ impacts.add(new Impact(maxFreq, (byte) i));
+ maxFreqForLowerNorms = maxFreq;
+ }
+ }
// Common case: all norms are bytes
return impacts;
+ } else {
+ TreeSet<Impact> freqNormPairs = new TreeSet<>(this.otherFreqNormPairs);
+ int maxFreqForLowerNorms = 0;
+ for (int i = 0; i < maxFreqs.length; ++i) {
+ int maxFreq = maxFreqs[i];
+ if (maxFreq > maxFreqForLowerNorms) {
+ add(new Impact(maxFreq, (byte) i), freqNormPairs);
+ maxFreqForLowerNorms = maxFreq;
+ }
+ }
+ return Collections.unmodifiableSet(freqNormPairs);
}
Review Comment:
This change is optimizing the uncommon path (when `otherFreqNormPairs` is
not empty) at the expense of additional code duplication. This doesn't look
like a good trade-off to me.
##########
lucene/core/src/java/org/apache/lucene/index/FieldInfos.java:
##########
@@ -165,8 +164,7 @@ public FieldInfos(FieldInfo[] infos) {
valuesTemp.add(byNumberTemp[i]);
}
}
- values =
-
Collections.unmodifiableCollection(Arrays.asList(valuesTemp.toArray(new
FieldInfo[0])));
+ values = Collections.unmodifiableCollection(valuesTemp);
Review Comment:
This essentially trades RAM for CPU, as we are saving a copy but the
`values` list may now be oversized. This doesn't obviously look like a better
trade-off to me?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
