Skip to content
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

Pass empty QueryCollectorContext in case of hybrid query to improve latencies by 20% #731

Conversation

martin-gaievski
Copy link
Member

@martin-gaievski martin-gaievski commented May 3, 2024

Description

In this change we're improving hybrid query latencies by avoiding redundant document score collection.

As per flamegraphs posted in #729 from 40 to 80% of CPU time of hybrid query processing is taken by TopDocsCollector in core. Those scores are not needed and should be removed from the execution flow in case this is hybrid query.

At the hight level the idea is to set empty doc collector instead of top docs collector in case incoming query is of type hybrid.

Ability to pass empty doc collector has been added to core in earlier PR. In this PR we're using this feature in neural plugin by passing new empty query collector context to query phase searcher. This is done by overriding searchWith method of both default and concurrent query phase searchers. The only collector and collector manager that will be executed for hybrid query are custom plugin implementations: HybridTopScoreDocCollector and HybridCollectorManager.

Below are metrics for hybrid query latency that I've collected after this change. There are based on 2.x (2.14) version and noaa OSB workload, all times are in ms:

One sub-query that selects 11M documents

Bool: p50 80.0931 | p90 80.4647
Hybrid: p50 182.314 | p90 204.263

One sub-query that selects 1.6K documents

Bool: p50 73.1325 | p90 73.7713
Hybrid: p50 72.9788 | p90 73.211

Three sub-query that select 15M documents

Bool: p50 85.8744 | p90 90.3619
Hybrid: p50 274.728 | p90 293.725

following is the baseline results, measurements are done before this change

One sub-query that selects 11M documents

Bool: p50 97.9306 | p90 116.299
Hybrid: p50 228.696 | p90 249.665

One sub-query that selects 1.6K documents

Bool: p50 87.3152 | p90 89.3061
Hybrid: p50 89.9654 | p90 92.349

Three sub-query that select 15M documents

Bool: p50 97.9891 | p90 114.396
Hybrid: p50 353.631 | p90 377.527

Following flamegraph (taken after this change) shows that there is no call to TopDocsCollector. More deep dive is needed to find next focus area for optimizations:

profile_hybrq_baseline_before_no_topdocscoll_5_2_1820

Issues Resolved

#729
#704

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has javadoc added
  • Commits are signed as per the DCO using --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.

@martin-gaievski martin-gaievski added backport 2.x Label will add auto workflow to backport PR to 2.x branch v2.15.0 hybrid search hybrid query performance optimization labels May 3, 2024
@martin-gaievski martin-gaievski force-pushed the pass_empty_query_collector_context_for_hybrid_query branch from d2c96c7 to 9659df1 Compare May 3, 2024 00:35
@martin-gaievski martin-gaievski changed the title Pass empty QueryCollectorContext in case of hybrid query Pass empty QueryCollectorContext in case of hybrid query to improve latencies by 20% May 3, 2024
@martin-gaievski
Copy link
Member Author

BWC rolling upgrade tests will fail for 2.14 while the release is ongoing and core is already switched to 2.15 for 2.x branch, restart BWCs are fine

@martin-gaievski martin-gaievski force-pushed the pass_empty_query_collector_context_for_hybrid_query branch from efba181 to 2257721 Compare May 4, 2024 00:17
Copy link
Member

@VijayanB VijayanB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@martin-gaievski martin-gaievski merged commit 2c556d2 into opensearch-project:main May 6, 2024
67 of 73 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 6, 2024
…atencies by 20% (#731)

* Pass empty QueryCollectorContext in case of hybrid query

Signed-off-by: Martin Gaievski <[email protected]>
(cherry picked from commit 2c556d2)
martin-gaievski added a commit that referenced this pull request May 20, 2024
…atencies by 20% (#731)

* Pass empty QueryCollectorContext in case of hybrid query

Signed-off-by: Martin Gaievski <[email protected]>
(cherry picked from commit 2c556d2)
vibrantvarun pushed a commit that referenced this pull request May 20, 2024
…atencies by 20% (#731) (#735)

* Pass empty QueryCollectorContext in case of hybrid query

Signed-off-by: Martin Gaievski <[email protected]>
(cherry picked from commit 2c556d2)

Co-authored-by: Martin Gaievski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Label will add auto workflow to backport PR to 2.x branch hybrid query performance optimization hybrid search v2.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants