Closed martin-gaievski closed 5 months ago
Attention: Patch coverage is 58.82353%
with 7 lines
in your changes are missing coverage. Please review.
Project coverage is 84.74%. Comparing base (
cc6a6b2
) to head (0376e9b
). Report is 12 commits behind head on main.:exclamation: Current head 0376e9b differs from pull request most recent head e34a7bc. Consider uploading reports for the commit e34a7bc to get more accurate results
Files | Patch % | Lines |
---|---|---|
...ensearch/neuralsearch/query/HybridQueryScorer.java | 58.82% | 4 Missing and 3 partials :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
This is an interesting improvement for such a small change. Really liked the deep-dive here.
Few things I would like you to do here just for bookkeeping purpose:
Great catch @martin-gaievski . LGTM.
This is a great improvement, curious to know if you have checked the profiling after this optimization to see if the CPU usage goes down and which one is the next culprit to CPU usage
The backport to 2.13
failed:
The process '/usr/bin/git' failed with exit code 1
To backport manually, run these commands in your terminal:
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.13 2.13
# Navigate to the new working tree
cd .worktrees/backport-2.13
# Create a new branch
git switch --create backport/backport-706-to-2.13
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 b277b07e89e25f1abaa9de3a326fda3556dc8a77
# Push it to GitHub
git push --set-upstream origin backport/backport-706-to-2.13
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.13
Then, create a pull request where the base
branch is 2.13
and the compare
/head
branch is backport/backport-706-to-2.13
.
This is an interesting improvement for such a small change. Really liked the deep-dive here.
Few things I would like you to do here just for bookkeeping purpose:
- The numbers and details which are quoted in the description please update them in the Issue.
- [Next step] Can you look at other places where we might be using streams in the hybrid query clause which may be adding some extra latency?
This is a great improvement, curious to know if you have checked the profiling after this optimization to see if the CPU usage goes down and which one is the next culprit to CPU usage
Next slowest section after Stream.findFirst is extra doc collector. It's added by core for all the queries it consumes resources (per my results it's from 40 to 75% of CPU time) but for hybrid query those results are just ignored. Current feasible approach requires changes in both core and the plugin. I need to check how CPU usage is after the fix.
Description
Hybrid query is generally slower than other compound queries with similar child sub-queries/clauses. For instance if compared to Boolean it can be up to 12 times slower, depending on the dataset, query and index/cluster configuration. Check results of benchmark that I took for released 2.13 using noaa OSB workload, all time is in ms:
Based on results of profiling most of the CPU time (35 to 40%) is taken by Stream.findFirst call in HybridQueryScorer.
That code is executed for each document returned by each of sub-query. That explains much longer execution time for queries that return larger sub-sets of a dataset.
That section of the code can be optimized to a plain
for
loop, plus the list of Integer is replaced by the plain array of ints. After optimization same code section takes 5 to 8% of overall execution time. Total time for clean hybrid query has been decreased 3-4 times for large sub-sets.Below are detailed results for the same workload:
following were bool queries used in testing
equivalent hybrid queres are:
Issues Resolved
https://github.com/opensearch-project/neural-search/issues/705
Check List
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.