Pre-calculate minRequiredScore to speedup filterCompetitiveHits #14827
+57
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-project-automation
bot
moved this to Open
in OpenSearch Lucene & Core Performance Tracking
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
For what it's worth, the reason of this PR is that I find Also, |
jpountz
reviewed
Jun 25, 2025
There was a problem hiding this comment.
Nice speedup! I wonder if we can make it work in the general case with something like that (untested):
diff --git a/lucene/core/src/java/org/apache/lucene/search/ScorerUtil.java b/lucene/core/src/java/org/apache/lucene/search/ScorerUtil.java
index 5c677d7a464..0b1e2f30a6e 100644
--- a/lucene/core/src/java/org/apache/lucene/search/ScorerUtil.java
+++ b/lucene/core/src/java/org/apache/lucene/search/ScorerUtil.java
@@ -128,15 +128,20 @@ class ScorerUtil {
double maxRemainingScore,
float minCompetitiveScore,
int numScorers) {
- if ((float) MathUtil.sumUpperBound(maxRemainingScore, numScorers) >= minCompetitiveScore) {
+
+ // Compute minRequiredScore as the greatest float value so that (float) MathUtil.sumUpperBound(minRequiredScore + maxRemainingScore, numScorers) <= minCompetitiveScore.
+ float minRequiredScore = (float) (minCompetitiveScore - maxRemainingScore);
+ while ((float) MathUtil.sumUpperBound(minRequiredScore + maxRemainingScore, numScorers) > minCompetitiveScore) {
+ minRequiredScore = Math.nextDown(minRequiredScore);
+ }
+
+ if (minRequiredScore <= 0) {
return;
}
int newSize = 0;
for (int i = 0; i < buffer.size; ++i) {
- float maxPossibleScore =
- (float) MathUtil.sumUpperBound(buffer.scores[i] + maxRemainingScore, numScorers);
- if (maxPossibleScore >= minCompetitiveScore) {
+ if (buffer.scores[i] >= minRequiredScore) {
buffer.docs[newSize] = buffer.docs[i];
buffer.scores[newSize] = buffer.scores[i];
newSize++;
|
FWIW I could confirm a speedup as well with this change on wiibigall. |
Thanks for your reply! These change totaly make sense to me. I've also tried to merge these two code paths, but got some terrifying output when running luceneutil :( (even after I explicitly set Maybe there is something I'm not familiar with? Or should I just change the code according to your advice? :) |
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
This suggests that the |
|
Got it, I'll dig into this, thanks for your explaination! It helps me a lot :) |
|
I played a bit more with your change, and it looks like we could make things even further as a follow-up through vectorization by using |
|
Thinking a bit more about this problem, finding the smallest float that meets the condition may not always be cheap. However, finding a good-enough float should be easy, and it's fine if the hit count changes. |
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
This PR does not have an entry in lucene/CHANGES.txt. Consider adding one. If the PR doesn't need a changelog entry, then add the skip-changelog label to it and you will stop receiving this reminder on future updates to the PR. |
|
Sorry for the late reply. Here is a example which can reproduce the two problems above: package org.apache.lucene;
import org.apache.lucene.util.MathUtil;
public class Run {
public static void main(String[] args) {
{
// problem that doc collected in patch but not in baseline
System.out.println("problem that doc collected in patch but not in baseline");
float minCompetitiveScore = 3.5382755f;
double score = 2.201078414916992d;
double maxRemainingScore = 1.337196946144104d;
// false, means this can not be collected in baseline
System.out.println(
((float) MathUtil.sumUpperBound(score + maxRemainingScore, 2)) >= minCompetitiveScore);
{
float minRequiredScore = (float) (minCompetitiveScore - maxRemainingScore);
while ((float) MathUtil.sumUpperBound(minRequiredScore + maxRemainingScore, 2) > minCompetitiveScore) {
minRequiredScore = Math.nextDown(minRequiredScore);
}
// score=2.201078414916992d, minRequiredScore=2.2010784f
// true, means this will be collected in patch
System.out.println(score >= minRequiredScore);
}
{
double minRequiredScore = (minCompetitiveScore - maxRemainingScore);
while ((float) MathUtil.sumUpperBound(minRequiredScore + maxRemainingScore, 2) > minCompetitiveScore) {
minRequiredScore = Math.nextDown(minRequiredScore);
}
// false, score=2.201078414916992d, minRequiredScoreD=2.2010785341262817d
System.out.println(score >= minRequiredScore);
}
}
System.out.println();
{
// problem that doc collected in baseline but not in patch
System.out.println("problem that doc collected in baseline but not in patch");
float minCompetitiveScore = 7.638806f;
double score = 7.638805627822876d;
double maxRemainingScore = 0.0d;
int numScorers = 33;
double minRequiredScore = (minCompetitiveScore - maxRemainingScore);
while ((float) MathUtil.sumUpperBound(minRequiredScore + maxRemainingScore, numScorers) > minCompetitiveScore) {
minRequiredScore = Math.nextDown(minRequiredScore);
}
// false, means this can not be collected by current patch.
System.out.println(score >= minRequiredScore);
// true, means this can be collected in baseline
// MathUtil.sumUpperBound(score + maxRemainingScore, numScorers)=7.638805627822984d;
// (float) MathUtil.sumUpperBound(score + maxRemainingScore, numScorers)=7.638806f == minCompetitiveScore;
//
// The original double (before cast to float) is actually smaller than minCompetitiveScore
// which means we can actually prune this doc safely.
System.out.println(
((float) MathUtil.sumUpperBound(score + maxRemainingScore, numScorers)) >= minCompetitiveScore);
}
}
}I did another quick one iteration full task luceneutils to verify the hit count, this time it still complains about the hit count, but all the diffs are one direction (patch less than basline), which I think is expected behavior: And I run another luceneutil on The while loop containing call to |
I'll run another full task luceneutil to verify the performance impact tonight |
Thanks for your advice! I think i understand what you mean, will try to find a proper way to utilize the vectorization these days. |
|
luceneutil result under same setup with lastest code: |
jpountz
reviewed
Jul 1, 2025
There was a problem hiding this comment.
Very cool to see speedups on queries that have more than 2 clauses as well! I left some comments. When we want to vectorize this (if it helps, but I think it may), we'll want minRequiredScore to be a float again, but we can figure it out in a follow-up.
Maybe extract this computation of minRequiredScore to a pkg-private helper function so that we can unit-test it?
HUSTERGS
changed the title
filterCompetitiveHits when have exact 2 clauses
jpountz
reviewed
Jul 2, 2025
|
FWIW, I was a little bit worried about the cost of final patch vs baseline cached
|
jpountz
reviewed
Jul 2, 2025
| int numScorers = random().nextInt(1, 10); | ||
|
|
||
| double minRequiredScore = | ||
| ScorerUtil.minRequiredScore(maxRemainingScore, minCompetitiveScore, numScorers); |
There was a problem hiding this comment.
I wonder if there's a way to track the number of iterations and fail the test if it took more than X (e.g. 10) iterations to converge.
There was a problem hiding this comment.
I tried to assert it in a way that relies on the internal implemenation detail of this function, not sure whether it's accecptable
There was a problem hiding this comment.
Or should we assert it inside minRequiredScore, still a little bit ugly though
lucene/core/src/test/org/apache/lucene/search/TestScorerUtil.java
Outdated
Show resolved
Hide resolved
added 2 commits
July 3, 2025 08:07
|
The change from luceneutil |
github-project-automation
bot
moved this from Open
to Merged
in OpenSearch Lucene & Core Performance Tracking
|
Thank you, we may want to look into making this better in follow-ups, but this looks good enough for me so I merged. 👍 |
jpountz
pushed a commit
that referenced
this pull request
Jul 3, 2025
Co-authored-by: gesong.samuel <gesong.samuel@bytedance.com>
|
Builds are failing after this has been merged? |
|
I'm taking a look now. |
|
The seed did not reproduce for me, but I think I understand the problem. The code assumes that if I pushed a fix. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR propose to specialize function
filterCompetitiveHitswhen we have exact 2 scorers, in order to reduce float calculation and potential function callsLuceneutil result on
wikimediumallwithsearchConcurrency=0,taskCountPerCat=5,taskRepeatCount=50after 20 iterations