Skip to content

Optimize TopGroups.merge() maxScore computation for relevance sorting#15879

Open
gaobinlong wants to merge 2 commits intoapache:mainfrom
gaobinlong:GroupingOptimization
Open

Optimize TopGroups.merge() maxScore computation for relevance sorting#15879
gaobinlong wants to merge 2 commits intoapache:mainfrom
gaobinlong:GroupingOptimization

Conversation

@gaobinlong
Copy link
Copy Markdown
Contributor

Description

When docSort is Sort.RELEVANCE, derive each group's maxScore directly from mergedTopDocs.scoreDocs[0] instead of accumulating across shards via nonNANmax.

Also hoist docSort.equals(Sort.RELEVANCE) into a local boolean to avoid repeated equals() calls in the per-group and per-shard loops.

Signed-off-by: Binlong Gao <gbinlong@amazon.com>
@github-actions github-actions bot added this to the 10.5.0 milestone Mar 26, 2026
@benwtrent
Copy link
Copy Markdown
Member

Do we have any benchmarks to show the improvement? It seems intuitive to me that there should be nice improvements.

@gaobinlong
Copy link
Copy Markdown
Contributor Author

Do we have any benchmarks to show the improvement? It seems intuitive to me that there should be nice improvements.

Thanks @benwtrent , the current code in the benchmark tool luceneutil doesn't cover the TopGroups.merge() method, I cherry-picked the code in the opening PR which introduces TopGroupsCollectorManager and then did benchmark test for this optimization, here're the result:

 TaskQPS baseline      StdDevQPS my_modified_version      StdDev                Pct diff
p-value
                    TermGroup10K       43.17      (5.1%)       43.51      (7.6%)    0.8% ( -11% -   14%) 0.700
                     TermGroup1M       24.44      (6.3%)       24.79      (9.4%)    1.5% ( -13% -   18%) 0.565
                    TermGroup100      104.37      (6.9%)      108.18      (8.8%)    3.6% ( -11% -   20%) 0.145

Signed-off-by: Binlong Gao <gbinlong@amazon.com>
@gaobinlong
Copy link
Copy Markdown
Contributor Author

Hi @benwtrent, could you help to review this PR too? It's as simple as #15942.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants