You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "shubhamsrkdev (via GitHub)" <gi...@apache.org> on 2026/01/28 23:25:35 UTC
[PR] Deferring lambda in TermStates.java according to prefetch [lucene]
shubhamsrkdev opened a new pull request, #15627:
URL: https://github.com/apache/lucene/pull/15627
### Problem
Currently, for term lookup `TermStates.get` defers for every case (to the lambda in TermStates.java). This works great for cold indexes but might not be the best for hot indexes.
### Solution
We can depend upon prefetch for deferring. If prefetch is being done then index is still cold hence we should defer termstate lookup through lambda, if not we should short-circuit and look up termstate directly instead of deferring.
### Testing
TK
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4185431559
The reason for the failure after merge is quite simple: the branch here was created long ago and was not merged up to main recently.
The method failing was added in another PR two weeks ago.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4169169009
@shubhamsrkdev do you think this can be backported to 10.x? I'm not sure if the prefetch APIs there are compatible?
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4155336497
This looks ready to me -- do you have any concerns, @uschindler ?
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2742153654
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
+ };
+ if (termExistsSupplier.doDefer()) {
+ return stateSupplier;
+ } else {
+ stateSupplier.get();
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
Review Comment:
Same as above - we night want to use stateSupplier.get() results instead
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
Review Comment:
I think before we kept `this.states[ctx.ord]` in a variable to guarantee single array read? Not sure if hotspot always optimizes it.
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
+ };
+ if (termExistsSupplier.doDefer()) {
+ return stateSupplier;
+ } else {
+ stateSupplier.get();
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
+ }
}
- return () -> state;
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
Review Comment:
Same as above
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4043086062
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2747612664
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
+ };
+ if (termExistsSupplier.doDefer()) {
+ return stateSupplier;
+ } else {
+ stateSupplier.get();
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
Review Comment:
Hmm `stateSupplier.get()` plummets the perf.. I will check why that happens, reverted to the simple lookup.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-3900293542
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r3021807268
##########
lucene/core/src/java/org/apache/lucene/util/IOBooleanSupplier.java:
##########
@@ -34,4 +34,13 @@ public interface IOBooleanSupplier {
* @throws IOException if supplying the result throws an {@link IOException}
*/
boolean get() throws IOException;
+
+ /**
Review Comment:
Why is this method in IOBooleanSupplier. It should not be here, that has nothing to do with this interface!
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r3022396073
##########
lucene/core/src/java/org/apache/lucene/util/IOBooleanSupplier.java:
##########
@@ -34,4 +34,13 @@ public interface IOBooleanSupplier {
* @throws IOException if supplying the result throws an {@link IOException}
*/
boolean get() throws IOException;
+
+ /**
Review Comment:
Thanks @uschindler let me create another PR to remove this.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2747510918
##########
lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java:
##########
@@ -369,14 +370,25 @@ private void updateReadAdvice(ReadAdvice readAdvice) throws IOException {
long offset = 0;
for (MemorySegment seg : segments) {
- advise(offset, seg.byteSize(), segment -> nativeAccess.madvise(segment, readAdvice));
+ advise(
+ offset,
+ seg.byteSize(),
+ segment -> {
+ nativeAccess.madvise(segment, readAdvice);
+ return true;
+ });
offset += seg.byteSize();
}
}
- void advise(long offset, long length, IOConsumer<MemorySegment> advice) throws IOException {
+ @FunctionalInterface
+ interface ReadAdviceConsumer {
Review Comment:
Thanks, much better, done!
##########
lucene/core/src/java/org/apache/lucene/util/IOBooleanSupplier.java:
##########
@@ -34,4 +34,8 @@ public interface IOBooleanSupplier {
* @throws IOException if supplying the result throws an {@link IOException}
*/
boolean get() throws IOException;
+
+ default boolean doDefer() {
Review Comment:
Added!
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2747510138
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
Review Comment:
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-3825379643
> +31% QPS - nice improvement for hot index scenario! I don't think we should see cold index improvements though - is the index cold enough 🥶?
Hmm I have tried with even lower memory (4 GB) with same results.
> I'm curious if we want to add the task to luceneutil permanently, as it looks like no other task exercises needScores = false code path for boolean queries?
Makes sense!
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4169763072
Hi the merging to main branch caused build failures because there were new tests which did not adapt to new method signature.
This can't be backported to 10.x because it has a significant API change in a public and often used API (prefetech).
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2742596883
##########
lucene/core/src/java/org/apache/lucene/util/IOBooleanSupplier.java:
##########
@@ -34,4 +34,8 @@ public interface IOBooleanSupplier {
* @throws IOException if supplying the result throws an {@link IOException}
*/
boolean get() throws IOException;
+
+ default boolean doDefer() {
Review Comment:
javadocs misisng
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2742607469
##########
lucene/core/src/java/org/apache/lucene/store/MemorySegmentIndexInput.java:
##########
@@ -369,14 +370,25 @@ private void updateReadAdvice(ReadAdvice readAdvice) throws IOException {
long offset = 0;
for (MemorySegment seg : segments) {
- advise(offset, seg.byteSize(), segment -> nativeAccess.madvise(segment, readAdvice));
+ advise(
+ offset,
+ seg.byteSize(),
+ segment -> {
+ nativeAccess.madvise(segment, readAdvice);
+ return true;
+ });
offset += seg.byteSize();
}
}
- void advise(long offset, long length, IOConsumer<MemorySegment> advice) throws IOException {
+ @FunctionalInterface
+ interface ReadAdviceConsumer {
Review Comment:
You could use `IOFunction<MemorySegment,Boolean>`. There's no overhead by the returned instance as there are only 2 boolean instances.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "epotyom (via GitHub)" <gi...@apache.org>.
epotyom commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-3818279119
+31% QPS - nice improvement for hot index scenario! I don't think we should see cold index improvements though - is the index cold enough 🥶?
I'm curious if we want to add the task to luceneutil permanently, as it looks like no other task exercises needScores = false code path for boolean queries?
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-3945811913
We merged a [PR](https://github.com/mikemccand/luceneutil/pull/538) In luceneutil which adds the task to measure perf improvement from this PR.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "msokolov (via GitHub)" <gi...@apache.org>.
msokolov merged PR #15627:
URL: https://github.com/apache/lucene/pull/15627
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4170871224
I filed #15909 to figure out why the checks passed but jenkins discovered a problem.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r3022667937
##########
lucene/core/src/java/org/apache/lucene/util/IOBooleanSupplier.java:
##########
@@ -34,4 +34,13 @@ public interface IOBooleanSupplier {
* @throws IOException if supplying the result throws an {@link IOException}
*/
boolean get() throws IOException;
+
+ /**
Review Comment:
Thinking about this more, if we create a separate class we will end up replacing `IOBooleanSupplier` everywhere with this new class and `IOBooleanSupplier` wouldn't have any usage in lucene's code (I understand it's a public interface so we will still keep it)?
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "mikemccand (via GitHub)" <gi...@apache.org>.
mikemccand commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4226132590
> The reason for the failure after merge is quite simple: the branch here was created long ago and was not merged up to main recently.
>
> The method failing was added in another PR two weeks ago.
Hmm this is a frustrating blind spot. GH actions passed on old branch. Main branch keeps moving forward. Then this (old) branch is merge/squashed with a simple rebase onto main that didn't produce source-level conflicts so GH/git is happy, and it's pushed, causing a compilation error ... so it is just up to whoever merges to remember to first merge main if branch is old, wait for GH actions to be happy, then merge?
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-3959395757
We have the first data point from running AndMissingHigh in nightlies [here](https://benchmarks.mikemccandless.com/AndMissingHigh.html):
<img width="1916" height="715" alt="image" src="https://github.com/user-attachments/assets/d2456852-8dc0-49bf-92f5-fbae25a8b327" />
This would be a good baseline to judge when this change is merged!
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r3021818701
##########
lucene/core/src/java/org/apache/lucene/util/IOBooleanSupplier.java:
##########
@@ -34,4 +34,13 @@ public interface IOBooleanSupplier {
* @throws IOException if supplying the result throws an {@link IOException}
*/
boolean get() throws IOException;
+
+ /**
Review Comment:
If we need something like this, it should be a separate class and not reusing this interface for something totally unrelated.
--
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on code in PR #15627:
URL: https://github.com/apache/lucene/pull/15627#discussion_r2747509458
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
+ };
+ if (termExistsSupplier.doDefer()) {
+ return stateSupplier;
+ } else {
+ stateSupplier.get();
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
Review Comment:
Done!
##########
lucene/core/src/java/org/apache/lucene/index/TermStates.java:
##########
@@ -193,28 +193,25 @@ public IOSupplier<TermState> get(LeafReaderContext ctx) throws IOException {
this.states[ctx.ord] = EMPTY_TERMSTATE;
return null;
}
- return () -> {
- if (this.states[ctx.ord] == null) {
- TermState state = null;
- if (termExistsSupplier.get()) {
- state = termsEnum.termState();
- this.states[ctx.ord] = state;
- } else {
- this.states[ctx.ord] = EMPTY_TERMSTATE;
- }
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
- }
- return state;
- };
- }
- TermState state = this.states[ctx.ord];
- if (state == EMPTY_TERMSTATE) {
- return null;
+ IOSupplier<TermState> stateSupplier =
+ () -> {
+ if (this.states[ctx.ord] == null) {
+ if (termExistsSupplier.get()) {
+ this.states[ctx.ord] = termsEnum.termState();
+ } else {
+ this.states[ctx.ord] = EMPTY_TERMSTATE;
+ }
+ }
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : this.states[ctx.ord];
+ };
+ if (termExistsSupplier.doDefer()) {
+ return stateSupplier;
+ } else {
+ stateSupplier.get();
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
+ }
}
- return () -> state;
+ return this.states[ctx.ord] == EMPTY_TERMSTATE ? null : () -> this.states[ctx.ord];
Review Comment:
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] Deferring lambda in TermStates.java according to prefetch [lucene]
Posted by "shubhamsrkdev (via GitHub)" <gi...@apache.org>.
shubhamsrkdev commented on PR #15627:
URL: https://github.com/apache/lucene/pull/15627#issuecomment-4206597603
We see an improvement of around +39.47% (from 4.94K to 6.89K) in `AndMissingHigh` task after the merging of this change:
<img width="1917" height="764" alt="image" src="https://github.com/user-attachments/assets/2404b669-bb1f-4dbd-b278-642444e26933" />
--
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