Avoid unnecessary DisjunctionMaxBulkScorer overhead

[shimpeko]

This change inspects clause bulk scorers up front and only uses DisjunctionMaxBulkScorer if at least one clause provides a non-default BulkScorer. Otherwise, we fall back to the scorer-based path.

c88f933 made DisjunctionMaxQuery use DisjunctionMaxBulkScorer when tieBreakerMultiplier == 0 and scoreMode == TOP_SCORES. However, this bulk path primarily pays off when at least one clause implements a specialized BulkScorer.

When all clauses return DefaultBulkScorer, the bulk windowing and replay logic adds overhead while preventing effective use of minCompetitiveScore and block-max optimizations that the scorer-based DisjunctionMaxScorer supports in TOP_SCORES mode.

In such cases, falling back to the scorer-based path typically results in better performance and restores competitive-score-based skipping.

Fixes: #15658
Related PR: #14040

[shimpeko]

I realized that we cannot call ScorerSupplier.get() more than once. Need another solutions.

[shimpeko]

I realized that we cannot call ScorerSupplier.get() more than once. Need another solutions.

I updated the code not to call ScorerSupplier.get() more than once.

[uschindler]

Hi, it would be nice to not always force push, this makes reviewing hard. I have no idea if you have fixed the linter warnings. Squashing the commits is done on our side while merging, please don't do it yourself.

Otherwise the change looks fine to me. but I leave it to @jpountz as he understand the code much better.

[uschindler]

Please fix the linter warning by running "./gradlew tidy" and commit, but don't sqash!

> Task :lucene:core:checkGoogleJavaFormat FAILED
java file(s) have google-java-format violations (run './gradlew tidy' to fix). An overview diff of changes:
== /home/runner/work/lucene/lucene/lucene/core/src/java/org/apache/lucene/search/DisjunctionMaxQuery.java
@@ -181,8 +181,7 @@
 ············}
 
 ············return·new·Weight.DefaultBulkScorer(
-················new·DisjunctionMaxScorer(tieBreakerMultiplier,·scorers,·scoreMode,·Long.MAX_VALUE)
-············);
+················new·DisjunctionMaxScorer(tieBreakerMultiplier,·scorers,·scoreMode,·Long.MAX_VALUE));
 ··········}
 
 ··········@Override

[shimpeko]

Thank you for the review, and sorry about the repeated force-pushes — I was trying to clean up the commit history and should have avoided doing that during review. I won’t force-push going forward. Ran tidy and pushed 68ada56. Will wait for @jpountz's review.

[jpountz]

When all clauses return DefaultBulkScorer, the bulk windowing and replay logic adds overhead while preventing effective use of minCompetitiveScore and block-max optimizations

It's true that the windowing+replay logic isn't free but I remember that it was still better than using a Scorer which had to keep reordering a heap on every document. As far as block-max optimizations are concerned, DisjunctionBulkMaxScorer tracks the min competitive score and passes it to its sub clauses whenever scoring a window (

lucene/lucene/core/src/java/org/apache/lucene/search/DisjunctionMaxBulkScorer.java

Line 81 in 7ebdb93

scorer.setMinCompetitiveScore(topLevelScorable.minCompetitiveScore);

), so this should work fine.

Let's benchmark this change with luceneutil and our dismax tasks (https://github.com/mikemccand/luceneutil/blob/main/tasks/wikinightly.tasks#L257-L276)? DisMaxTerm in particular should be a good task since TermQuery doesn't have a bulk scorer. I haven't played with this for a while, it's possible that bulk scoring isn't helping anymore.

[shimpeko]

I tried to run wikinightly but it didn't finish after 10 hours on my laptop so just picked some dismax tasks. Looking at the result, this change (edit: This PR) basically has no effect on performance of those dismax tasks including DismaxTerm. It also means that the bulk scoring is not improving performance of DismaxTerm.

                            TaskQPS baseline      StdDevQPS my_modified_version      StdDev                Pct diff p-value
                 DismaxOrHighMed      218.28     (15.2%)      216.35     (14.6%)   -0.9% ( -26% -   34%) 0.767
                      DismaxTerm      633.42     (16.9%)      635.44     (16.2%)    0.3% ( -28% -   40%) 0.923
                        PKLookup      230.86     (14.8%)      232.32     (14.0%)    0.6% ( -24% -   34%) 0.826
                DismaxOrHighHigh      202.17     (15.9%)      205.03     (15.3%)    1.4% ( -25% -   38%) 0.650
         FilteredDismaxOrHighMed      140.01     (11.7%)      142.35     (10.1%)    1.7% ( -18% -   26%) 0.445
        FilteredDismaxOrHighHigh       59.48     (12.1%)       61.13      (8.9%)    2.8% ( -16% -   27%) 0.191
              FilteredDismaxTerm      127.87     (13.5%)      133.05      (8.8%)    4.1% ( -16% -   30%) 0.075

I'll dig around deeper.

Commands to run bench and task detail

util % grep -A 5 'sourceData =' src/python/localrun.py 
  sourceData = competition.Data(
    "wikimediumall",
    constants.WIKI_MEDIUM_DOCS_LINE_FILE,
    constants.WIKI_MEDIUM_DOCS_COUNT,
    constants.DISMAX_TERM_TASKS_FILE,
  )
util % cat src/python/localconstants.py 
BASE_DIR = '/Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home'
BENCH_BASE_DIR = '/Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home/util'
DISMAX_TERM_TASKS_FILE = '%s/tasks/dismax_term_only.tasks' % BENCH_BASE_DIR
util % cd ../lucene_baseline && git show -s --oneline HEAD && cd ../util/
7ebdb9316e5 (HEAD -> main, origin/main, origin/HEAD) Add next minor version 10.5.0
util % cd ../lucene_candidate && git show -s --oneline HEAD && cd ../util 
68ada56464 (HEAD -> dismax-bulk-heuristic, origin/dismax-bulk-heuristic) ./gradlew tidy --rerun-tasks
util % python src/python/localrun.py --iterations=50 --warmups=50 -b /Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home/lucene_baseline -c /Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home/lucene_candidate > result.txt
util % cat tasks/dismax_term_only.tasks 
DismaxTerm: 0 +dismaxFields=titleTokenized,body
DismaxTerm: names +dismaxFields=titleTokenized,body
DismaxTerm: nbsp +dismaxFields=titleTokenized,body
DismaxTerm: part +dismaxFields=titleTokenized,body
DismaxTerm: st +dismaxFields=titleTokenized,body

DismaxOrHighHigh: are last +dismaxFields=titleTokenized,body
DismaxOrHighHigh: at united +dismaxFields=titleTokenized,body
DismaxOrHighHigh: but year +dismaxFields=titleTokenized,body
DismaxOrHighHigh: name its +dismaxFields=titleTokenized,body
DismaxOrHighHigh: to but +dismaxFields=titleTokenized,body

DismaxOrHighMed: at mostly +dismaxFields=titleTokenized,body
DismaxOrHighMed: his interview +dismaxFields=titleTokenized,body
DismaxOrHighMed: http 9 +dismaxFields=titleTokenized,body
DismaxOrHighMed: they hard +dismaxFields=titleTokenized,body
DismaxOrHighMed: title bay +dismaxFields=titleTokenized,body

FilteredDismaxTerm: 0 +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxTerm: names +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxTerm: nbsp +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxTerm: part +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxTerm: st +dismaxFields=titleTokenized,body +filter=5%

FilteredDismaxOrHighHigh: are last +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighHigh: at united +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighHigh: but year +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighHigh: name its +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighHigh: to but +dismaxFields=titleTokenized,body +filter=5%

FilteredDismaxOrHighMed: at mostly +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighMed: his interview +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighMed: http 9 +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighMed: they hard +dismaxFields=titleTokenized,body +filter=5%
FilteredDismaxOrHighMed: title bay +dismaxFields=titleTokenized,body +filter=5%
util %

[shimpeko]

cont. #15659 (comment)

DismaxTerm task uses BatchScoreBulkScorer so baseline and candidate were using the same path in the benchmark on #15659 (comment).

[shimpeko]

I added test case that are closer to my query which has dismax + constant_score as https://github.com/shimpeko/luceneutil/pull/1/changes. Looking at the following benchmark result, I think I can say that the changes on this PR has significant positive impact on the performance of specifc type of query.

As far as block-max optimizations are concerned, DisjunctionBulkMaxScorer tracks the min competitive score and passes it to its sub clauses whenever scoring a window

This seems true, so I don't cleary understand why using DisjunctionBulkMaxScorer causing regression in this paticular case, yet. @jpountz do you have any idea.

Edit: I'll look deeper into DisjunctionBulkMaxScorer but happy if I could get feedbacks/suggestions, for this PR after having the benchmark result.

Result (50 warmups, 50 iter)

                            TaskQPS baseline      StdDevQPS my_modified_version      StdDev                Pct diff p-value
                      DismaxTerm     1034.68     (11.6%)      935.85     (11.7%)   -9.6% ( -29% -   15%) 0.000
                 DismaxOrHighMed      224.64     (16.2%)      206.15     (16.7%)   -8.2% ( -35% -   29%) 0.012
              FilteredDismaxTerm      201.44      (9.5%)      189.45     (12.1%)   -5.9% ( -25% -   17%) 0.006
        FilteredDismaxOrHighHigh       61.88     (14.1%)       59.53     (11.3%)   -3.8% ( -25% -   25%) 0.137
                DismaxOrHighHigh       99.12     (13.0%)       95.37     (12.9%)   -3.8% ( -26% -   25%) 0.143
                        PKLookup      240.78     (14.9%)      232.55     (15.0%)   -3.4% ( -29% -   31%) 0.254
         FilteredDismaxOrHighMed      170.69     (14.1%)      166.51     (13.5%)   -2.5% ( -26% -   29%) 0.374
                   DisMaxCsTerm1     1814.59     (12.8%)     2010.12     (16.9%)   10.8% ( -16% -   46%) 0.000
                  DisMaxCSTerm20      169.78     (12.3%)      307.85     (43.2%)   81.3% (  22% -  156%) 0.000

Commands to run bench

util % cd ../lucene_candidate && git show -s --oneline HEAD && cd ../util
68ada56464 (HEAD -> dismax-bulk-heuristic, origin/dismax-bulk-heuristic) ./gradlew tidy --rerun-tasks
util % cd ../lucene_baseline && git show -s --oneline HEAD && cd ../util
7ebdb9316e5 (HEAD -> main, origin/main, origin/HEAD) Add next minor version 10.5.0
util % cd ../lucene_candidate && git show -s --oneline HEAD && cd ../util
68ada56464 (HEAD -> dismax-bulk-heuristic, origin/dismax-bulk-heuristic) ./gradlew tidy --rerun-tasks
util % grep -A 5 'sourceData =' src/python/localrun.py                   
  sourceData = competition.Data(
    "wikimediumall",
    constants.WIKI_MEDIUM_DOCS_LINE_FILE,
    constants.WIKI_MEDIUM_DOCS_COUNT,
    constants.DISMAX_TASKS_FILE,
  )
util % cat src/python/localconstants.py 
import os

BASE_DIR = '/Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home'
BENCH_BASE_DIR = '/Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home/util'
DISMAX_TASKS_FILE = '%s/tasks/dismax_constantscore.tasks' % BENCH_BASE_DIR

#JAVA_HOME = os.environ.get("JAVA_HOME")
#java_bin = JAVA_HOME + "/bin/" if JAVA_HOME else ""
#if java_bin:
#  print("Using java from: %s" % java_bin)
#if "JAVA_EXE" not in globals():
#  JAVA_EXE = f"{java_bin}java"
#if "JAVAC_EXE" not in globals():
#  JAVAC_EXE = f"{java_bin}javac"
#if "JAVA_COMMAND" not in globals():
#  JAVA_COMMAND = "%s -server -Xms2g -Xmx2g --add-modules jdk.incubator.vector -XX:+HeapDumpOnOutOfMemoryError -XX:+UseParallelGC  -Dlucene.dismax.debug=true" % JAVA_EXE
#else:
#  print("use java command %s" % JAVA_COMMAND)  # pyright: ignore[reportUndefinedVariable] # TODO: fix how variables are managed here
util % python src/python/localrun.py --iteration=50 --warmups=50 -b /Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home/lucene_baseline -c /Users/shimpei-kodama/github.com/mikemccand/luceneutil/bench_home/lucene_candidate > result.txt