You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "gsmiller (via GitHub)" <gi...@apache.org> on 2024/08/15 01:55:16 UTC
[PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
gsmiller opened a new pull request, #13657:
URL: https://github.com/apache/lucene/pull/13657
After merging #13568 @mikemccand and @jpountz [noticed](https://github.com/apache/lucene/pull/13568#issuecomment-2288666763) some surprising nightly benchmark performance regressions. @epotyom and I were both able to [reproduce](https://github.com/apache/lucene/pull/13568#issuecomment-2289109976) the regressions locally with luceneutil and push a short-term change to unwind an isolated change to `IndexSearcher` that appeared to fix the regression (#13656).
Since then, I've spend some time trying to really tightly isolate the change that could cause the performance regression. I've finally been able to reproduce it by just moving the creation of the "collectors list" as demonstrated in this demo PR (not to be merged!). I can't wrap my head around how the diff captured by this PR could possibly lead to the large benchmark impact I'm seeing with luceneutil (posted below). @mikemccand, @jpountz, @epotyom ... others... any thoughts on this? I'm stumped. But, connecting it back to how #13568 caused the regression, I think the explanation is that `CollectorOwner` initializes the collector list early during initialization, effectively having the same effect as this PR by creating the list earlier.
Results of running luceneutil with wikimedium10m on this PR:
```
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
MedTerm 513.21 (4.9%) 369.43 (4.8%) -28.0% ( -35% - -19%) 0.000
HighTerm 523.20 (6.9%) 402.11 (5.0%) -23.1% ( -32% - -12%) 0.000
LowTerm 837.70 (3.9%) 715.94 (3.9%) -14.5% ( -21% - -6%) 0.000
BrowseDayOfYearSSDVFacets 11.97 (18.9%) 11.31 (11.9%) -5.5% ( -30% - 31%) 0.273
MedTermDayTaxoFacets 23.03 (4.9%) 21.95 (6.4%) -4.7% ( -15% - 6%) 0.009
HighPhrase 143.93 (8.3%) 139.35 (4.7%) -3.2% ( -14% - 10%) 0.136
Fuzzy2 53.03 (9.0%) 51.50 (7.3%) -2.9% ( -17% - 14%) 0.265
MedSpanNear 50.70 (5.1%) 49.26 (3.0%) -2.8% ( -10% - 5%) 0.032
LowPhrase 70.38 (4.9%) 68.60 (5.3%) -2.5% ( -12% - 8%) 0.118
MedPhrase 88.15 (5.2%) 86.03 (4.2%) -2.4% ( -11% - 7%) 0.105
OrHighMedDayTaxoFacets 7.01 (5.5%) 6.86 (5.4%) -2.0% ( -12% - 9%) 0.237
HighSpanNear 28.95 (2.7%) 28.42 (2.9%) -1.8% ( -7% - 3%) 0.043
MedSloppyPhrase 201.71 (3.3%) 198.58 (3.1%) -1.6% ( -7% - 4%) 0.124
BrowseDateTaxoFacets 23.97 (28.7%) 23.62 (22.8%) -1.5% ( -41% - 70%) 0.858
AndHighMedDayTaxoFacets 32.81 (5.8%) 32.35 (7.1%) -1.4% ( -13% - 12%) 0.493
AndHighHighDayTaxoFacets 27.86 (6.1%) 27.50 (6.5%) -1.3% ( -13% - 12%) 0.507
LowSloppyPhrase 149.20 (2.9%) 147.50 (3.0%) -1.1% ( -6% - 4%) 0.227
HighTermTitleBDVSort 66.72 (6.6%) 66.04 (5.7%) -1.0% ( -12% - 12%) 0.604
AndHighHigh 187.45 (7.4%) 185.75 (6.7%) -0.9% ( -13% - 14%) 0.684
LowSpanNear 102.21 (2.1%) 101.50 (1.5%) -0.7% ( -4% - 3%) 0.242
OrHighHigh 218.06 (6.3%) 216.74 (4.1%) -0.6% ( -10% - 10%) 0.721
HighTermTitleSort 132.14 (1.5%) 131.93 (1.3%) -0.2% ( -2% - 2%) 0.724
HighSloppyPhrase 31.43 (5.4%) 31.39 (6.6%) -0.1% ( -11% - 12%) 0.949
BrowseRandomLabelSSDVFacets 7.91 (10.2%) 7.91 (11.2%) -0.0% ( -19% - 23%) 0.992
AndHighMed 288.24 (4.9%) 288.33 (4.0%) 0.0% ( -8% - 9%) 0.982
AndHighLow 1339.09 (3.2%) 1345.87 (4.8%) 0.5% ( -7% - 8%) 0.694
OrHighMed 473.22 (3.9%) 476.21 (3.8%) 0.6% ( -6% - 8%) 0.603
BrowseDayOfYearTaxoFacets 23.67 (28.7%) 23.82 (23.5%) 0.6% ( -40% - 74%) 0.938
HighTermDayOfYearSort 415.29 (5.2%) 418.26 (5.9%) 0.7% ( -9% - 12%) 0.684
BrowseDateSSDVFacets 2.14 (21.4%) 2.16 (22.4%) 1.0% ( -35% - 56%) 0.887
Wildcard 489.21 (4.3%) 494.69 (4.5%) 1.1% ( -7% - 10%) 0.420
TermDTSort 216.56 (5.9%) 219.04 (4.8%) 1.1% ( -8% - 12%) 0.499
PKLookup 139.24 (8.7%) 140.89 (10.8%) 1.2% ( -16% - 22%) 0.703
Fuzzy1 74.44 (9.7%) 75.42 (8.3%) 1.3% ( -15% - 21%) 0.643
Respell 48.52 (7.2%) 49.20 (6.6%) 1.4% ( -11% - 16%) 0.519
OrNotHighLow 1260.39 (3.0%) 1279.03 (2.7%) 1.5% ( -4% - 7%) 0.101
MedIntervalsOrdered 132.03 (9.2%) 134.25 (12.6%) 1.7% ( -18% - 25%) 0.630
BrowseMonthTaxoFacets 24.51 (26.9%) 25.02 (25.5%) 2.0% ( -39% - 74%) 0.804
HighTermMonthSort 1117.15 (4.1%) 1143.38 (4.6%) 2.3% ( -6% - 11%) 0.090
BrowseRandomLabelTaxoFacets 15.54 (25.0%) 15.93 (19.7%) 2.5% ( -33% - 62%) 0.724
Prefix3 667.73 (11.1%) 684.51 (11.1%) 2.5% ( -17% - 27%) 0.474
LowIntervalsOrdered 118.38 (14.5%) 121.55 (14.8%) 2.7% ( -23% - 37%) 0.564
HighIntervalsOrdered 30.52 (9.2%) 31.34 (7.0%) 2.7% ( -12% - 20%) 0.298
OrNotHighMed 365.66 (5.9%) 376.73 (6.1%) 3.0% ( -8% - 15%) 0.110
OrHighLow 586.67 (5.7%) 608.48 (5.6%) 3.7% ( -7% - 15%) 0.037
OrNotHighHigh 257.09 (5.8%) 267.66 (6.5%) 4.1% ( -7% - 17%) 0.034
BrowseMonthSSDVFacets 11.21 (9.1%) 11.69 (7.1%) 4.3% ( -11% - 22%) 0.100
OrHighNotLow 446.78 (8.7%) 479.82 (7.1%) 7.4% ( -7% - 25%) 0.003
OrHighNotMed 591.66 (7.6%) 649.35 (4.8%) 9.8% ( -2% - 23%) 0.000
IntNRQ 202.12 (17.5%) 224.77 (28.1%) 11.2% ( -29% - 68%) 0.130
OrHighNotHigh 339.78 (8.0%) 393.02 (6.7%) 15.7% ( 0% - 33%) 0.000
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2296904888
Just for tracking, repeating this comment https://github.com/apache/lucene/pull/13656#issuecomment-2296813725 here:
> The biggest difference in the profiler seems to be that we spend more time in org.apache.lucene.search.similarities.BM25Similarity$BM25Scorer.score(float, long) now?
I've tried changing `SIMILARITY_DEFAULT` to `ClassicSimilarity` in `src/python/constants.py` . There is still regression seen in the caller method `org.apache.lucene.search.LeafSimScorer.score(int, float)`, but not so much in the `Similarity` itself:
JFRs:
[ClassicSimilarity.zip](https://github.com/user-attachments/files/16662602/ClassicSimilarity.zip)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2293708857
Found one more interesting nuance this morning. If the `collectors` ArrayList is created with no initial size, it has no impact on performance no matter where it gets created (before-or-after createWeight). But if there's an initial size, then the impact remains (where it regresses performance if done before createWeight and has no impact if done after). So I wonder if it has something to do with allocating memory on the heap before calling createWeight and less about this actual collection?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-3408724298
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2290359914
As one more thought, all I can think is that these impacted tasks are possibly taking the `leafSlices.length == 0` condition branch and avoiding the need to create the collectors list at all? Is it possible that creating these lists and reclaiming them in GC is somehow having this kind of outsized impact?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2339375272
This PR has not had activity in the past 2 weeks, labeling it as stale. If the PR is waiting for review, notify the dev@lucene.apache.org list. Thank you for your contribution!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2290926100
Interesting... Thanks for isolating the change that causes the regression Greg!
> impacted tasks are possibly taking the `leafSlices.length == 0` condition branch and avoiding the need to create the collectors list at all?
I don't think it is that - if I understand correctly `leafSlices.length == 0` only if there are no segments at all - which shouldn't be the case for benchmark tests we are running. Just in case I added a commit on top of this PR to never create the ArrayList for collectors if the length is zero: https://github.com/apache/lucene/commit/e30b76d77926d5f68d70f8b1d3b7129a73b76942 , and it also shows regression:
```
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 525.44 (3.5%) 441.68 (3.7%) -15.9% ( -22% - -9%) 0.000
HighSloppyPhrase 33.23 (2.7%) 28.13 (1.4%) -15.3% ( -18% - -11%) 0.000
MedTerm 479.40 (4.6%) 410.71 (4.4%) -14.3% ( -22% - -5%) 0.000
LowTerm 433.49 (3.1%) 387.45 (3.1%) -10.6% ( -16% - -4%) 0.000
BrowseMonthTaxoFacets 1.84 (9.0%) 1.70 (8.8%) -7.9% ( -23% - 10%) 0.160
BrowseRandomLabelTaxoFacets 1.37 (12.4%) 1.26 (0.5%) -7.6% ( -18% - 6%) 0.174
HighSpanNear 31.21 (1.7%) 28.97 (2.8%) -7.2% ( -11% - -2%) 0.000
...
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2298104290
> @epotyom
> > I've noticed by accident that creating two identical lists of collectors, before and after createWeight, makes regression disappear
>
> Hmm, that is strange. I would think that `collectors2` in your diff would get optimized away since it's never read from? So I thought maybe that explained why you weren't seeing the regression, but if `collectors1` is the one that gets left you'd think we'd still see a regression since it's before `createWeight`?
Exactly, without `collectors2` it is the code with the regression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2307371884
I've been running some experiments, so far this is what I observe: It is not important in which order we init collection of `Collector`s and call `createWeight`, but it is important that we allocate something on heap after `createWeight` call to **avoid** regression. The minimum size that has to be allocated seems to be ~44 bytes.
Still don't have a very good theory that explains it. I believe unused arrays decrease the Survivor/Eden promotion ratio. It might be that the decrease is signifiant enough and fewer objects overflow to old gen? And old gen GC is not needed as often? And in baseline we create Collectors list a little bit later too, which might mean that it is garbage-collected in Eden space more often hence also decreases the ratio? I'll experiment with `-XX:SurvivorRatio` see if different values change anything.
### Test results
I've added one line below `createWeight` which creates an empty array, e.g. `Object[] arr = new Object[7];` or `byte[] arr = new byte[23];`, etc
#### Object[] array
size 5: REGRESSION; size on heap (here and below the sizes are rough estimates to the best of my knowledge, not taking padding/OS cache, etc into account) `16+5*4 = 36`, 12+4 bytes object+length overhead; 4 bytes for each reference since max heap size is below 32G ([link](https://www.baeldung.com/java-size-of-object#:~:text=References%20have%20a%20typical%20size,%2D50%25%20more%20heap%20space.))
```
Report after iter 7:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 337.78 (2.2%) 324.30 (3.3%) -4.0% ( -9% - 1%) 0.004
```
size 6: REGRESSION; size on heap `16+6*4 = 40`
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 346.95 (3.5%) 332.90 (3.4%) -4.1% ( -10% - 2%) 0.000
```
size 7: NO regression; size on heap `16+7*4 = 44`
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 364.91 (4.1%) 360.21 (3.7%) -1.3% ( -8% - 6%) 0.295
```
#### int[] array
size 6; size on heap `16+6*4 = 40`; REGRESSION;
```
Report after iter 16:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 360.99 (3.0%) 349.82 (2.6%) -3.1% ( -8% - 2%) 0.002
```
size 7; size on heap `16+7*4 = 44`; NO regression
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 360.49 (4.4%) 355.37 (3.3%) -1.4% ( -8% - 6%) 0.252
```
#### byte[] array
size 10; size on heap `16+10 = 26`; REGRESSION
```
Report after iter 15:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 353.31 (3.9%) 325.77 (3.3%) -7.8% ( -14% - 0%) 0.000
```
size 20; size on heap `16+20 = 36`; REGRESSION
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 356.40 (3.5%) 338.70 (3.1%) -5.0% ( -11% - 1%) 0.000
```
size 22; size on heap `16+22 = 38`; REGRESSION
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 355.32 (4.9%) 342.58 (2.7%) -3.6% ( -10% - 4%) 0.004
```
size 23; size on heap `16+23 = 39`; REGRESSION
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 357.27 (4.2%) 341.16 (2.9%) -4.5% ( -11% - 2%) 0.000
```
size 24; size on heap `16+24 = 40`; REGRESSION/NO regression
```
Report after iter 15:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 349.07 (3.1%) 349.22 (2.6%) 0.0% ( -5% - 5%) 0.966
```
```
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 361.25 (4.1%) 338.94 (3.2%) -6.2% ( -12% - 1%) 0.000
```
size 25; size on heap `16+25 = 41`; REGRESSION/NO regression
```
1st run:
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 357.41 (3.5%) 356.03 (4.1%) -0.4% ( -7% - 7%) 0.747
2nd run:
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 359.64 (3.9%) 350.90 (3.6%) -2.4% ( -9% - 5%) 0.042
```
size 28; size on heap `16+28 = 44`; NO regression
```
1st run
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 360.73 (4.1%) 353.56 (3.9%) -2.0% ( -9% - 6%) 0.115
2nd run
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 359.99 (2.9%) 360.59 (3.9%) 0.2% ( -6% - 7%) 0.878
3rd run
Report after iter 19:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
HighTerm 353.56 (4.5%) 350.58 (4.3%) -0.8% ( -9% - 8%) 0.541
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2293718689
Note that setting the initial size requires calling `getSlices()` which might perhaps be put off by the compiler if its return value is not used. In turn `getSlices() does some complicated things including creating a `TaskExecutor` -- this seems to me it could possibly cause some mischief?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2293806502
Ah let me clarify a bit. I actually moved the call to getSlices back into the called private method to eliminate it from complicating things and used a constant value (randomly chose 8) to specify the initial capacity. So I think we can rule out interactions with getSlices.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2310853821
Added `-Xlog:gc* -Xlog:age*=debug` to test command, looks like GC spends almost equal time in baseline/candidate and is not the root cause here.
Baseline:
```
Statistics Ended at Mon Aug 26 18:48:46 UTC 2024
Elapsed time: 15551 ms
Time in JIT compilation: 7717 ms
Time in Young Generation GC: 628 ms (12 collections)
Time in Old Generation GC: 1874 ms (4 collections)
Garbage Generated in Young Generation: 0.0 MiB
Garbage Generated in Survivor Generation: 0.0 MiB
Garbage Generated in Old Generation: 0.0 MiB
Average CPU Load: 330.5843/1600
```
Candidate:
```
Statistics Ended at Mon Aug 26 18:48:30 UTC 2024
Elapsed time: 15735 ms
Time in JIT compilation: 7181 ms
Time in Young Generation GC: 626 ms (12 collections)
Time in Old Generation GC: 1903 ms (4 collections)
Garbage Generated in Young Generation: 0.0 MiB
Garbage Generated in Survivor Generation: 0.0 MiB
Garbage Generated in Old Generation: 0.0 MiB
Average CPU Load: 339.74777/1600
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2291353795
I got an even tighter isolation on the regressing change but still can't understand why it could be happening. It seems to have to do with whether-or-not the collectors list gets created before or after calling `createWeight`. So this order _does not_ cause any regression:
```
final Weight weight = createWeight(query, firstCollector.scoreMode(), 1);
final List<C> collectors = new ArrayList<>(leafSlices.length);
```
While this does:
```
final List<C> collectors = new ArrayList<>(leafSlices.length);
final Weight weight = createWeight(query, firstCollector.scoreMode(), 1);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2291351570
> leafSlices.length == 0 only if there are no segments at all
Right, agreed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2293467656
I've made some temporary changes in luceneutil to be able to only run a couple of tasks that show regression and have meaningful profiler results - profiler results that we get for all tasks seems to have too many samples for other tasks e.g. faceting.
Results after 20 runs:
```
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
MedTerm 581.44 (3.8%) 505.29 (3.5%) -13.1% ( -19% - -5%) 0.000
HighTerm 559.77 (3.5%) 501.17 (3.5%) -10.5% ( -16% - -3%) 0.000
```
The biggest difference in the profiler seems to be that we spend more time in `org.apache.lucene.search.similarities.BM25Similarity$BM25Scorer.score(float, long)` now?
Diff image:
JFR files:
[Archive.zip](https://github.com/user-attachments/files/16637306/Archive.zip)
The code that was tested is slightly different from this PR, sharing branches just in case:
- candidate: Branch with regression: https://github.com/epotyom/lucene/tree/IndexSearcher-search-regression
- baseline: Branch with NO regression: https://github.com/epotyom/lucene/tree/IndexSearcher-search-NO-regression
- Their diff: https://github.com/epotyom/lucene/pull/1/files
luceneutil branch to reproduce: https://github.com/mikemccand/luceneutil/compare/main...epotyom:luceneutil:tasks_with_regression
you'd need to generate task file manually as it seems to be to large for for github:
```
rm tasks/wikimedium.10M.regressed.tasks
cat tasks/wikimedium.10M.nostopwords.tasks | egrep '^(MedTerm|HighTerm):' > tasks/wikimedium.10M.regressed.tasks.1
for n in {1..10000}; do cat tasks/wikimedium.10M.regressed.tasks.1 >> tasks/wikimedium.10M.regressed.tasks; done
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2295025111
I've noticed by accident that creating two identical lists of collectors, before and after `createWeight`, makes regression disappear. Diff: https://github.com/epotyom/lucene/pull/2/files
```
Report after iter 18:
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
MedTerm 752.75 (3.6%) 753.41 (2.6%) 0.1% ( -5% - 6%) 0.932
HighTerm 520.02 (4.6%) 525.65 (3.3%) 1.1% ( -6% - 9%) 0.403
```
Not sure what to make of it, but it does feel like some heap allocation peculiarity.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2293872374
OK, I inlined the `IndexSearcher#createWeight` code to further isolate things. The ordering that matters is whether-or-not the `collections` list gets created before the call to `Query#createWeight` (so it rules out anything to do with the query cache stuff). I updated the diff in this PR to show this along with comments explaining.
And, as mentioned before, it matters whether-or-not an initial capacity is provided. If no initial capacity is provided then the ordering doesn't matter and no regressions are seen (so I'm standing by my guess that this has something to do with allocating heap before the call to `Query#createWeight` and not so much something specific with this `collections` list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2297489039
@epotyom
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2291516168
Just thinking out loud here, but could this have something to do with code optimizations or hotspot decisions the jvm is making? Could moving the initialization of this list around result in different decisions about inlining or compiling this hot code?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] nocommit: demonstrate how a minor change in IndexSearcher can have an inexplicable performance impact [lucene]
Posted by "gsmiller (via GitHub)" <gi...@apache.org>.
gsmiller commented on PR #13657:
URL: https://github.com/apache/lucene/pull/13657#issuecomment-2290353375
I'll also mention that I experimented with inlining the private search method to avoid having to pass leafSlices and collectors as arguments to the private method and I see the same effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org