Apply sequential read advice during vector merges - #153423
Conversation
🔍 Preview links for changed docs⏳ Building and deploying preview... View progress This comment will be updated with preview links when the build is complete. |
3422179 to
e71e32b
Compare
ℹ️ Important: Docs version tagging👋 Thanks for updating the docs! Just a friendly reminder that our docs are now cumulative. This means all 9.x versions are documented on the same page and published off of the main branch, instead of creating separate pages for each minor version. We use applies_to tags to mark version-specific features and changes. Expand for a quick overviewWhen to use applies_to tags:✅ At the page level to indicate which products/deployments the content applies to (mandatory) What NOT to do:❌ Don't remove or replace information that applies to an older version 🤔 Need help?
|
|
Pinging @elastic/es-search-relevance (Team:Search Relevance) |
|
Hi @jimczi, I've updated the changelog YAML for you. |
IVFVectorsReader did not override getMergeInstance() or finishMerge(), which Lucene calls during a merge to switch the read advice and restore it afterwards. Because of that, the flat vector reader kept the RANDOM advice used for search throughout the merge, disabling read-ahead on the sequential scans (checkIntegrity and the vector copy) that a merge performs. Override both methods so the merge instance is backed by the flat readers' merge instances, which switch their input to SEQUENTIAL, and finishMerge() restores the search access pattern. The centroid and posting list inputs are opened with the default context and already use read-ahead advice, so they are left unchanged. Signed-off-by: Jim Ferenczi <jim.ferenczi@elastic.co>
12f3f34 to
5f164dc
Compare
|
Hi @jimczi, I've updated the changelog YAML for you. |
|
It's worth re-evaluating this in light of #155919, which removes the page cache from merges entirely |
MergeReaderWrapper returned its merge reader without calling getMergeInstance() on it, and never implemented finishMerge(). Both calls therefore stopped at the wrapper, so a reader that is only reachable behind it never got the chance to prepare for, or recover from, a merge. This is the path taken whenever direct I/O is enabled for the raw vectors. ES818BinaryQuantizedVectorsReader had the other half of the problem: it built a merge instance from the raw reader but never told that reader the merge had finished, leaving whatever getMergeInstance() changed in place for subsequent searches. Delegate both calls in both readers.
Lucene brackets a merge with
getMergeInstance()andfinishMerge()on the reader it ishanded: the first lets a reader prepare for merging (the flat vector readers use it to switch
their input from the
DataAccessHint.RANDOMadvice used for search toSEQUENTIAL), thesecond restores the search access pattern afterwards. Both calls have to travel the whole
reader chain to reach the reader that owns the raw vector input. Three links in that chain
did not pass them on.
IVFVectorsReader(bbq_disk) overrode neither, so the flat vector reader keptRANDOMadvice for the entire merge, disabling read-ahead on the sequential scans a merge performs
(
checkIntegrity()and the vector copy).getMergeInstance()now returns a copy of thereader — via a copy constructor — backed by the flat readers' merge instances, and
finishMerge()delegates to them. Only the flat vectors are affected: the centroid andposting list inputs (
ivfCentroids/ivfClusters) are opened with the default context andalready use read-ahead advice, so they are left unchanged.
MergeReaderWrapperreturned its merge reader without callinggetMergeInstance()on it,and did not implement
finishMerge()at all, so both calls stopped at the wrapper. This isthe reader that sits in front of the raw vectors whenever direct I/O is enabled for them
(
on_disk_rescore: true), which means the fix above had no effect at all in thatconfiguration — the call reached the wrapper and went no further. Both are now delegated.
ES818BinaryQuantizedVectorsReader(bbq_hnsw) had the opposite half of the problem: itbuilt a merge instance from the raw reader but never told that reader the merge had finished.
Since the flat reader mutates shared state and relies on
finishMerge()to revert it, the rawvector input stayed on sequential advice after the first merge — the wrong access pattern for
the rescoring path that reads it next.
finishMerge()now delegates.MergeReaderWrapperTestscovers the wrapper's half of the contract with recording readers:that both calls reach the merge reader, and that the search reader is never asked for a merge
instance nor told to finish one. The test fails on both counts without the production change.
Known gap left in place:
Lucene104ScalarQuantizedVectorsReader(int8_hnsw/int4_hnsw)overrides neither method, so nothing below it is reached for those field types. It keeps its
rawVectorsReaderprivate with no copy constructor, so the equivalent fix cannot be made fromthe Elasticsearch subclass and belongs upstream.