From 295c5d35767c2af9361f79dfc11511ad5ababd81 Mon Sep 17 00:00:00 2001 From: Jakub Slowinski <32519034+slow-J@users.noreply.github.com> Date: Tue, 9 Jul 2024 15:25:02 +0100 Subject: [PATCH] GITHUB#13175: Stop double-checking priority queue inserts in some FacetCount classes (#13488) * GITHUB#13175: Stop double-checking priority queue inserts Removing 2 cases of bottomX optimizations where insertWithOverflow already handles the check. Closes #13175 * Update CHANGES.txt --------- Co-authored-by: Jakub Slowinski --- lucene/CHANGES.txt | 2 + .../lucene/facet/StringValueFacetCounts.java | 48 +++++++------------ .../AbstractSortedSetDocValueFacetCounts.java | 28 ++++------- 3 files changed, 29 insertions(+), 49 deletions(-) diff --git a/lucene/CHANGES.txt b/lucene/CHANGES.txt index 1fa11f6625a..16000083199 100644 --- a/lucene/CHANGES.txt +++ b/lucene/CHANGES.txt @@ -278,6 +278,8 @@ Optimizations * GITHUB#12941: Don't preserve auxiliary buffer contents in LSBRadixSorter if it grows. (Stefan Vodita) +* GITHUB#13175: Stop double-checking priority queue inserts in some FacetCount classes. (Jakub Slowinski) + Changes in runtime behavior --------------------- diff --git a/lucene/facet/src/java/org/apache/lucene/facet/StringValueFacetCounts.java b/lucene/facet/src/java/org/apache/lucene/facet/StringValueFacetCounts.java index 9e63043f3fa..655e80546f8 100644 --- a/lucene/facet/src/java/org/apache/lucene/facet/StringValueFacetCounts.java +++ b/lucene/facet/src/java/org/apache/lucene/facet/StringValueFacetCounts.java @@ -180,8 +180,6 @@ public class StringValueFacetCounts extends Facets { topN = Math.min(topN, cardinality); TopOrdAndIntQueue q = null; TopOrdAndIntQueue.OrdAndInt reuse = null; - int bottomCount = 0; - int bottomOrd = Integer.MAX_VALUE; int childCount = 0; // total number of labels with non-zero count if (sparseCounts != null) { @@ -189,7 +187,22 @@ public class StringValueFacetCounts extends Facets { childCount++; // every count in sparseValues should be non-zero int ord = sparseCount.key; int count = sparseCount.value; - if (count > bottomCount || (count == bottomCount && ord < bottomOrd)) { + if (q == null) { + // Lazy init for sparse case: + q = new TopOrdAndIntQueue(topN); + } + if (reuse == null) { + reuse = (TopOrdAndIntQueue.OrdAndInt) q.newOrdAndValue(); + } + reuse.ord = ord; + reuse.value = count; + reuse = (TopOrdAndIntQueue.OrdAndInt) q.insertWithOverflow(reuse); + } + } else if (denseCounts != null) { + for (int i = 0; i < denseCounts.length; i++) { + int count = denseCounts[i]; + if (count != 0) { + childCount++; if (q == null) { // Lazy init for sparse case: q = new TopOrdAndIntQueue(topN); @@ -197,36 +210,9 @@ public class StringValueFacetCounts extends Facets { if (reuse == null) { reuse = (TopOrdAndIntQueue.OrdAndInt) q.newOrdAndValue(); } - reuse.ord = ord; + reuse.ord = i; reuse.value = count; reuse = (TopOrdAndIntQueue.OrdAndInt) q.insertWithOverflow(reuse); - if (q.size() == topN) { - bottomCount = ((TopOrdAndIntQueue.OrdAndInt) q.top()).value; - bottomOrd = q.top().ord; - } - } - } - } else if (denseCounts != null) { - for (int i = 0; i < denseCounts.length; i++) { - int count = denseCounts[i]; - if (count != 0) { - childCount++; - if (count > bottomCount || (count == bottomCount && i < bottomOrd)) { - if (q == null) { - // Lazy init for sparse case: - q = new TopOrdAndIntQueue(topN); - } - if (reuse == null) { - reuse = (TopOrdAndIntQueue.OrdAndInt) q.newOrdAndValue(); - } - reuse.ord = i; - reuse.value = count; - reuse = (TopOrdAndIntQueue.OrdAndInt) q.insertWithOverflow(reuse); - if (q.size() == topN) { - bottomCount = ((TopOrdAndIntQueue.OrdAndInt) q.top()).value; - bottomOrd = q.top().ord; - } - } } } } diff --git a/lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java b/lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java index 230e9bf9e6c..03a0ce72190 100644 --- a/lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java +++ b/lucene/facet/src/java/org/apache/lucene/facet/sortedset/AbstractSortedSetDocValueFacetCounts.java @@ -322,8 +322,6 @@ abstract class AbstractSortedSetDocValueFacetCounts extends Facets { private TopChildrenForPath computeTopChildren( PrimitiveIterator.OfInt childOrds, int topN, DimConfig dimConfig, int pathOrd) { TopOrdAndIntQueue q = null; - int bottomCount = 0; - int bottomOrd = Integer.MAX_VALUE; int pathCount = 0; int childCount = 0; @@ -334,23 +332,17 @@ abstract class AbstractSortedSetDocValueFacetCounts extends Facets { if (count > 0) { pathCount += count; childCount++; - if (count > bottomCount || (count == bottomCount && ord < bottomOrd)) { - if (q == null) { - // Lazy init, so we don't create this for the - // sparse case unnecessarily - q = new TopOrdAndIntQueue(topN); - } - if (reuse == null) { - reuse = (TopOrdAndIntQueue.OrdAndInt) q.newOrdAndValue(); - } - reuse.ord = ord; - reuse.value = count; - reuse = (TopOrdAndIntQueue.OrdAndInt) q.insertWithOverflow(reuse); - if (q.size() == topN) { - bottomCount = ((TopOrdAndIntQueue.OrdAndInt) q.top()).value; - bottomOrd = q.top().ord; - } + if (q == null) { + // Lazy init, so we don't create this for the + // sparse case unnecessarily + q = new TopOrdAndIntQueue(topN); } + if (reuse == null) { + reuse = (TopOrdAndIntQueue.OrdAndInt) q.newOrdAndValue(); + } + reuse.ord = ord; + reuse.value = count; + reuse = (TopOrdAndIntQueue.OrdAndInt) q.insertWithOverflow(reuse); } }