LUCENE-8762: Introduce Specialized Impacts For Doc + Freq #779
Conversation
|
cc: @jpountz |
|
Any thoughts on this one? I am hoping this will bring gains to our nightly benchmarks :) |
lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50PostingsReader.java
Show resolved
Hide resolved
lucene/core/src/java/org/apache/lucene/codecs/lucene50/Lucene50PostingsReader.java
Show resolved
Hide resolved
|
@jpountz I have fixed your comments, please let me know if they look fine. luceneutil is running right now with 10k documents, will post results ASAP |
|
I don't expect your last commit to help since impacts should always be requested with frequencies. What I had in mind when mentioning loading frequencies lazily was to do something similar to what was done in #595 to only read and decode the block of frequencies if freq() is called on the current block. |
|
Ah, yes, that makes sense. I did a couple of more runs with 10m docs and saw similar results. Will update the PR |
|
@jpountz Updated the PR with lazy loading. Please let me know if it looks fine |
|
luceneutil run: https://gist.github.com/atris/1b2c25021ca11138338aa73efde2aa38 wikimedium2m |
| private int accum; // accumulator for doc deltas | ||
| private int freq; // freq we last read | ||
|
|
||
| private boolean needsFreq; // true if the caller actually needs frequencies |
There was a problem hiding this comment.
This will always be true, it is illegal to read impacts and not ask for term frequencies (it doesn't make much sense).
|
I ran luceneutil on this patch on my machine too and I'm getting similar results. It's a bit disappointing, I was expecting some gains though I'm rather happy that we can keep things easier to maintain by not having to sepcialize too much. Sorry for the time you spent on it but I think it's better to keep things the way they are today? |
No sweat, I understand. Makes sense to keep things straightforward! |
ant test for both Lucene and Solr pass. ant precommit passes as well.
I did not add new tests since multiple existing tests exercise the new functionality