Avoid eagerly loading StoredFieldsReader in fetch phase #83693
Conversation
Every time we create a hit document, we create a new SourceLookup and call setSegmentAndDocument. This in turn creates a new StoredFieldsReader, which is pretty expensive. In scenarios where you are retrieving a lot of hits, this can add significant overhead. Prior to version 7.11, we did not create a new SourceLookup per hit, so this is a performance regression. This PR updates setSegmentAndDocument to avoid eagerly creating a new StoredFieldsReader (through StoredFieldsReader#getMergeInstance).
jtibshirani
added
>bug
:Search/Search
|
Hi @jtibshirani, I've created a changelog YAML for you. |
|
Some notes:
|
|
|
||
| import java.util.function.Supplier; | ||
|
|
||
| public class MemoizedSupplier<T> implements Supplier<T> { |
There was a problem hiding this comment.
I restored this class, which was only very recently deleted because it was unused.
| // get better sequential access. | ||
| if (context.reader() instanceof SequentialStoredFieldsLeafReader lf) { | ||
| // Avoid eagerly loading the stored fields reader, since this can be expensive | ||
| Supplier<StoredFieldsReader> supplier = new MemoizedSupplier<>(lf::getSequentialStoredFieldsReader); |
There was a problem hiding this comment.
The main issue is that getSequentialStoredFieldsReader calls StoredFieldsReader#getMergeInstance, which creates a new stored fields reader. I wonder if this could be optimized in Lucene to avoid recreating it every time 🤔
There was a problem hiding this comment.
I think ideally we'd like to push as much of this up into Lucene as possible, as it is all hacks at the moment and if anything changes in how stored fields are merged then we are in trouble.
|
Pinging @elastic/es-search (Team:Search) |
| // get better sequential access. | ||
| if (context.reader() instanceof SequentialStoredFieldsLeafReader lf) { | ||
| // Avoid eagerly loading the stored fields reader, since this can be expensive | ||
| Supplier<StoredFieldsReader> supplier = new MemoizedSupplier<>(lf::getSequentialStoredFieldsReader); |
There was a problem hiding this comment.
I think ideally we'd like to push as much of this up into Lucene as possible, as it is all hacks at the moment and if anything changes in how stored fields are merged then we are in trouble.
|
Hi @jtibshirani, I've updated the changelog YAML for you. |
|
Thanks for the reviews! I forgot to link the original issue: #82777. |
|
@elasticmachine ok to test |
Every time we create a hit document, we create a new SourceLookup and call setSegmentAndDocument. This in turn creates a new StoredFieldsReader, which is pretty expensive. In scenarios where you are retrieving a lot of hits, this can add significant overhead. Prior to version 7.11, we did not create a new SourceLookup per hit, so this is a performance regression. This PR updates setSegmentAndDocument to avoid eagerly creating a new StoredFieldsReader (through StoredFieldsReader#getMergeInstance).
Every time we create a hit document, we create a new SourceLookup and call setSegmentAndDocument. This in turn creates a new StoredFieldsReader, which is pretty expensive. In scenarios where you are retrieving a lot of hits, this can add significant overhead. Prior to version 7.11, we did not create a new SourceLookup per hit, so this is a performance regression. This PR updates setSegmentAndDocument to avoid eagerly creating a new StoredFieldsReader (through StoredFieldsReader#getMergeInstance).
Every time we create a hit document, we create a new SourceLookup and call
setSegmentAndDocument. This in turn creates a new StoredFieldsReader, which is
pretty expensive. In scenarios where you are retrieving a lot of hits, this can
add significant overhead. Prior to version 7.11, we did not create a new
SourceLookup per hit, so this is a performance regression.
This PR updates setSegmentAndDocument to avoid eagerly creating a
new StoredFieldsReader (through StoredFieldsReader#getMergeInstance).
Closes #82777.