Update TermStates.java to remove deferred lambda in TermStates.get#15458
Update TermStates.java to remove deferred lambda in TermStates.get#15458shubhamsrkdev wants to merge 2 commits intoapache:mainfrom
Conversation
Removed lambda function
gradle tidy
|
This was added for prefetching: #13359 How does this perform in scenarios where prefetching is beneficial (e.g low memory)? |
Thanks @benwtrent - is there a standard way to test this? How much would we consider low memory? |
|
@benwtrent this change gives us significant enough performance boost, I think it is because BooleanWeight has optimizations for clauses which return But looks like it is bad for low memory scenarios, so I wonder if propagating |
I honestly don't fully understand all the code here. I am simply pointing out that this change was made with a particular thing in mind, and that thing seems completely ignored (which we shouldn't do).
I am not sure it is? I am pushing back on this change simply because it is effectively a revert of a previous commit and we need a good reason for that (e.g. do we need prefetching? Are we wanting to handling it differently?) |
|
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! |
|
I ran the benchmarks with low memory : The following was the result, It is mostly mixed: We have a long term solution here: #15515 so I will close this. |
Description
Remove the lambda from
TermStates.get(LeafReaderContext). The method now eagerly resolves the term lookup. I am not sure if this change would expressly benefit everyone, feedback here would be greatly appreciated.Benchmarks
The results looks like noise as the p-value is high for all of them.