You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "original-brownbear (via GitHub)" <gi...@apache.org> on 2024/07/06 04:12:43 UTC
[PR] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
original-brownbear opened a new pull request, #13546:
URL: https://github.com/apache/lucene/pull/13546
The value for the global count is incremented a lot more than it is read, the space overhead of LongAdder seems irrelevant => lets use LongAdder. The performance gain from using it is the higher the more threads you use, but at 4 threads already very visible in benchmarks.
For wikimediumall I see:
```
TaskQPS baseline StdDevQPS my_modified_version StdDev Pct diff p-value
BrowseMonthSSDVFacets 4.75 (6.3%) 4.65 (8.2%) -2.2% ( -15% - 13%) 0.348
LowPhrase 40.41 (9.1%) 39.59 (7.0%) -2.0% ( -16% - 15%) 0.427
OrHighMedDayTaxoFacets 2.49 (7.0%) 2.48 (4.6%) -0.1% ( -10% - 12%) 0.957
BrowseRandomLabelSSDVFacets 3.53 (6.2%) 3.53 (6.4%) -0.0% ( -11% - 13%) 0.989
MedPhrase 586.66 (4.8%) 588.40 (5.0%) 0.3% ( -9% - 10%) 0.849
AndHighMed 251.69 (9.1%) 253.84 (3.6%) 0.9% ( -10% - 15%) 0.698
AndHighLow 1321.95 (10.0%) 1335.73 (5.9%) 1.0% ( -13% - 18%) 0.688
AndHighHighDayTaxoFacets 4.15 (3.8%) 4.20 (2.9%) 1.0% ( -5% - 7%) 0.323
MedTermDayTaxoFacets 20.33 (4.5%) 20.58 (4.1%) 1.2% ( -7% - 10%) 0.367
PKLookup 245.17 (3.6%) 248.31 (3.6%) 1.3% ( -5% - 8%) 0.266
BrowseMonthTaxoFacets 8.21 (55.1%) 8.32 (55.6%) 1.3% ( -70% - 249%) 0.940
LowIntervalsOrdered 50.77 (9.8%) 51.48 (10.5%) 1.4% ( -17% - 24%) 0.662
AndHighMedDayTaxoFacets 24.18 (1.5%) 24.56 (1.8%) 1.6% ( -1% - 4%) 0.003
BrowseRandomLabelTaxoFacets 4.21 (5.9%) 4.28 (5.9%) 1.8% ( -9% - 14%) 0.341
BrowseDayOfYearSSDVFacets 4.56 (5.9%) 4.64 (10.7%) 1.8% ( -13% - 19%) 0.512
HighPhrase 51.95 (6.3%) 52.90 (6.8%) 1.8% ( -10% - 15%) 0.381
Fuzzy2 77.27 (3.6%) 79.02 (5.7%) 2.3% ( -6% - 11%) 0.130
Respell 45.66 (1.8%) 46.73 (1.7%) 2.3% ( -1% - 5%) 0.000
Fuzzy1 103.35 (4.0%) 106.17 (3.6%) 2.7% ( -4% - 10%) 0.022
IntNRQ 27.92 (8.6%) 28.70 (6.7%) 2.8% ( -11% - 19%) 0.254
BrowseDateSSDVFacets 1.24 (13.2%) 1.28 (11.8%) 3.7% ( -18% - 33%) 0.351
OrHighHigh 61.55 (12.0%) 64.18 (12.0%) 4.3% ( -17% - 32%) 0.260
BrowseDateTaxoFacets 4.92 (5.8%) 5.14 (12.5%) 4.6% ( -12% - 24%) 0.139
OrNotHighLow 985.77 (6.9%) 1031.87 (3.7%) 4.7% ( -5% - 16%) 0.008
MedSloppyPhrase 98.83 (8.9%) 104.44 (9.1%) 5.7% ( -11% - 26%) 0.046
HighTermMonthSort 1340.66 (10.5%) 1419.43 (7.1%) 5.9% ( -10% - 26%) 0.038
BrowseDayOfYearTaxoFacets 4.93 (5.7%) 5.23 (12.4%) 6.0% ( -11% - 25%) 0.049
Wildcard 51.85 (6.9%) 55.03 (5.4%) 6.1% ( -5% - 19%) 0.002
AndHighHigh 145.76 (6.2%) 155.93 (10.0%) 7.0% ( -8% - 24%) 0.008
MedSpanNear 27.61 (8.1%) 29.85 (7.1%) 8.1% ( -6% - 25%) 0.001
LowSloppyPhrase 125.48 (12.5%) 135.78 (11.1%) 8.2% ( -13% - 36%) 0.028
OrHighMed 146.22 (8.3%) 159.53 (9.3%) 9.1% ( -7% - 29%) 0.001
Prefix3 923.91 (15.1%) 1013.30 (6.7%) 9.7% ( -10% - 37%) 0.009
OrHighLow 747.82 (4.7%) 820.26 (4.7%) 9.7% ( 0% - 19%) 0.000
HighSloppyPhrase 24.47 (4.6%) 26.93 (4.1%) 10.0% ( 1% - 19%) 0.000
MedIntervalsOrdered 68.46 (11.9%) 75.68 (12.4%) 10.5% ( -12% - 39%) 0.006
HighSpanNear 13.57 (3.2%) 15.32 (5.0%) 12.9% ( 4% - 21%) 0.000
LowSpanNear 84.84 (6.0%) 95.80 (4.7%) 12.9% ( 2% - 25%) 0.000
HighIntervalsOrdered 19.93 (8.4%) 23.28 (11.2%) 16.8% ( -2% - 39%) 0.000
MedTerm 670.60 (4.6%) 897.88 (9.5%) 33.9% ( 18% - 50%) 0.000
OrNotHighMed 201.35 (4.0%) 271.43 (6.6%) 34.8% ( 23% - 47%) 0.000
LowTerm 583.19 (3.6%) 872.31 (5.1%) 49.6% ( 39% - 60%) 0.000
OrNotHighHigh 227.42 (4.3%) 353.58 (8.3%) 55.5% ( 41% - 71%) 0.000
HighTermTitleBDVSort 22.16 (4.1%) 35.54 (12.1%) 60.4% ( 42% - 79%) 0.000
TermDTSort 79.33 (3.7%) 140.30 (17.6%) 76.9% ( 53% - 101%) 0.000
OrHighNotHigh 184.76 (3.6%) 339.75 (12.3%) 83.9% ( 65% - 103%) 0.000
HighTermTitleSort 33.86 (3.2%) 64.43 (14.3%) 90.3% ( 70% - 111%) 0.000
OrHighNotMed 187.30 (4.1%) 357.81 (14.8%) 91.0% ( 69% - 114%) 0.000
OrHighNotLow 245.45 (6.5%) 492.07 (11.3%) 100.5% ( 77% - 126%) 0.000
HighTerm 152.40 (4.3%) 313.65 (17.1%) 105.8% ( 80% - 132%) 0.000
HighTermDayOfYearSort 135.35 (2.3%) 311.95 (25.0%) 130.5% ( 100% - 161%) 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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2216965045
I pushed an annotation, the speedup on some queries is huge: https://people.apache.org/~mikemccand/lucenebench/CombinedHighHigh.html.
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2220333332
Makes sense @benwtrent! For `BufferedUpdatesStream` as its on the index side we should check on the indexing time and not the regular luceneutil benchmarks to check the QPS?
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2218438332
@benwtrent @original-brownbear Yes, I agree with both points. I haven't looked into the profiler but I was mainly referring to the usages I see in [DocumentsWriter](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/DocumentsWriter.java#L81) or [BufferedUpdatesStream](https://github.com/apache/lucene/blob/main/lucene/core/src/java/org/apache/lucene/index/BufferedUpdatesStream.java#L58) where we could have some high contention between the indexing threads?. This seems like the scenario of multiple threads(I assume usually greater than 2 or 3 indexing threads) updating it simultaneously ("many write & few read" as Ben mentioned or maybe "many write & many read")?
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "jpountz (via GitHub)" <gi...@apache.org>.
jpountz merged PR #13546:
URL: https://github.com/apache/lucene/pull/13546
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "shubhamvishu (via GitHub)" <gi...@apache.org>.
shubhamvishu commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2218065362
Nice! Does it make sense to change other `AtomicLong` occurences to `LongAdder`? Is see couple of other usages where there might be some opportunity to squeeze some more gains probably?
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2218295960
> Does it make sense to change other AtomicLong occurences
@shubhamvishu I would say only if those usages are a "many write & few read" scenarios.
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2218618086
@shubhamvishu looking at `BufferedUpdatesStream` & `bytesUsed`, that may be a candidate. However, benchmarking would have to prove it out.
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2211625398
The results are easily explained by the code change, the top methods in profiling go from:
```
10.79% 129797 org.apache.lucene.facet.sortedset.SortedSetDocValuesFacetCounts#countOneSegmentNHLD()
5.81% 69901 org.apache.lucene.facet.taxonomy.FastTaxonomyFacetCounts#countAll()
4.60% 55316 org.apache.lucene.codecs.lucene90.Lucene90DocValuesProducer$25#advance()
4.35% 52334 org.apache.lucene.util.packed.DirectReader$DirectPackedReader20#get()
4.28% 51449 org.apache.lucene.search.TopScoreDocCollector$SimpleTopScoreDocCollector$1#collect()
3.74% 45035 org.apache.lucene.util.packed.DirectReader$DirectPackedReader12#get()
3.44% 41324 org.apache.lucene.search.TopFieldCollector$TopFieldLeafCollector#countHit()
```
to
```
11.43% 131366 org.apache.lucene.facet.sortedset.SortedSetDocValuesFacetCounts#countOneSegmentNHLD()
6.19% 71133 org.apache.lucene.facet.taxonomy.FastTaxonomyFacetCounts#countAll()
4.88% 56035 org.apache.lucene.codecs.lucene90.Lucene90DocValuesProducer$25#advance()
4.48% 51495 org.apache.lucene.util.packed.DirectReader$DirectPackedReader20#get()
4.01% 46096 org.apache.lucene.util.packed.DirectReader$DirectPackedReader12#get()
3.30% 37975 jdk.internal.foreign.MemorySessionImpl#checkValidStateRaw()
3.15% 36257 org.apache.lucene.util.packed.DirectMonotonicReader#get()
```
with the top score collector moving way down to:
```
0.93% 10727 org.apache.lucene.search.TopScoreDocCollector$SimpleTopScoreDocCollector$1#collect()
```
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "original-brownbear (via GitHub)" <gi...@apache.org>.
original-brownbear commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2218302360
@shubhamvishu what Ben says, plus keep in mind that the adder is less consistent and probably slower for <= 2 or 3 threads competing scenarios on x86 hardware (that seems to be what it comes down empirically today at least). This needs handling with care :) Also, the better solution (if possible) is generally to just not have the contention rather than implementing a better solution. I think this one in particular has an even stronger possible solution (it still shows up in profiling and it should no), looking into that once I find some time :)
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2218607252
@shubhamvishu stuff like this gets tricky when you see patterns like
`if(pendingNumDocs.incrementAndGet() > IndexWriter.getActualMaxDocs()) {`
There, you want to be ABSOLUTELY sure that you incremented and trip on a get. This to me seems to make using LongAdder for something like `pendingNumDocs` a bad idea.
--
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] Replace AtomicLong with LongAdder in HitsThresholdChecker [lucene]
Posted by "benwtrent (via GitHub)" <gi...@apache.org>.
benwtrent commented on PR #13546:
URL: https://github.com/apache/lucene/pull/13546#issuecomment-2220694253
@shubhamvishu I do not know off hand which benchmarks should be 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