-
Notifications
You must be signed in to change notification settings - Fork 114
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize reduceToTopK in ResultUtil by removing pre-filling and reducing peek calls #2146
Optimize reduceToTopK in ResultUtil by removing pre-filling and reducing peek calls #2146
Conversation
98c5a4b
to
e988ccf
Compare
src/main/java/org/opensearch/knn/index/query/ExactSearcher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/ExactSearcher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/ExactSearcher.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/query/ExactSearcher.java
Outdated
Show resolved
Hide resolved
…ing peek calls Signed-off-by: Junqiu Lei <[email protected]>
e988ccf
to
7d1ad23
Compare
Offline synced with @navneet1v and @jmazanec15, we now focused on optimizing reduceToTopK function in ResultUtil, updated the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@junqiu-lei Did we compare removing the map with this approach to see which is faster? or is it not possible? I feel like its a simpler code that way if its yielding better results
- removing the map approach: See if we can preserve the order of the results for each leaf instead of converting it to map.
We might have to first check if KNNResults array is in order. but that way we can do something similar to what lucene does and just pick the top elements and stop at k in reduceToTopK
I think this is another place we can optimize, no we don't have comparing the difference so far. For this PR, we might can use micro benchmark for testing the improvement if possible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I think the most optimal way to do this would be to require that each perLeafResults comes in order from top scores to worst scores. Then, a simple algorithm that basically just scans the top of each set of results and then only takes the top k. That being, that would require how results are returned in the different methods. Do you think we should do this @navneet1v ?
Signed-off-by: Junqiu Lei <[email protected]>
I can raise other PR for this part optimization. |
Yes I think we should change the return types |
…ing peek calls (#2146) Signed-off-by: Junqiu Lei <[email protected]> (cherry picked from commit e0c3afe)
…ing peek calls (#2146) (#2164) Signed-off-by: Junqiu Lei <[email protected]> (cherry picked from commit e0c3afe) Co-authored-by: Junqiu Lei <[email protected]>
Description
This PR optimizes the reduceToTopK method by eliminating unnecessary pre-filling of the priority queue, reducing redundant peek() calls, and adding null safety checks for better performance.
Related Issues
Closes #2145
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.