Speed up reading history operations from Lucene

[andrross]

Previously when reading history, every document would fetch a fresh StoredFields instance, which decompress that compression block for every document. This optimization detects a contiguous gapless range of docIDs and serves them from the same (sequential) stored fields reader.

The optimization is gated on the same thread that created the StoredFields instance must read it, as Lucene documents that instances are not safe to hand across threads. Additionally, a min batch size threshold must be met to avoid decompress the block eagerly in the case where it does not matter.

Benchmark results:

┌────────────────────┬──────────┬─────────┐
│ layout             │ baseline │ change  │
├────────────────────┼──────────┼─────────┤
│ CONTIGUOUS         │ 331 ms   │ 19.2 ms │
├────────────────────┼──────────┼─────────┤
│ STRIDED (stride 2) │ 379 ms   │ 324 ms  │
├────────────────────┼──────────┼─────────┤
│ INTERLEAVED        │ 334 ms   │ 312 ms  │
└────────────────────┴──────────┴─────────┘

Check List

• Functionality includes testing.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Flag computed only for first batch useSequentialStoredFieldsReader is set inside fillParallelArray, which the constructor calls for the first batch of scoreDocs. On subsequent batches (when nextBatch()/further searches populate more scoreDocs), the flag is re-evaluated based on the new batch. If the first batch is contiguous but a later batch is not (or vice versa), the reused sequentialStoredFields instance may be reset via releaseSequentialStoredFieldsReader() correctly, but the sort ordering assumption differs between batches: prior batches were not sorted by docID but later batches will be sorted. Since each batch is processed independently via docIndex, this is likely fine, but it's worth confirming the interaction across batches when the sequential flag flips. useSequentialStoredFieldsReader = scoreDocs.length >= MIN_SEQUENTIAL_ACCESS_BATCH_SIZE && hasSequentialAccess(scoreDocs); if (useSequentialStoredFieldsReader == false) { releaseSequentialStoredFieldsReader(); // for better loading performance we sort the array by docID and // then visit all leaves in order. ArrayUtil.introSort(scoreDocs, Comparator.comparingInt(i -> i.doc)); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Always reset cached sequential reader per batch fillParallelArray is called once per batch (via searchOperations), so the value of useSequentialStoredFieldsReader can flip between batches. When it flips from true to false, the previously cached sequentialStoredFields is still valid but never invoked; when it flips from false to true after having been true, the cached reader may point at a stale leaf. However, when it stays true across batches but the new batch is in a different leaf, the check sequentialStoredFieldsLeafOrd != leaf.ord handles it. The concern is that when flipping true→false, releaseSequentialStoredFieldsReader() correctly clears the cache — good. But when flipping false→true after previously being true, no reset happens. Consider always calling releaseSequentialStoredFieldsReader() before recomputing, so the cache is never stale across batches. server/src/main/java/org/opensearch/index/engine/LuceneChangesSnapshot.java [240-246] +releaseSequentialStoredFieldsReader(); useSequentialStoredFieldsReader = scoreDocs.length >= MIN_SEQUENTIAL_ACCESS_BATCH_SIZE && hasSequentialAccess(scoreDocs); if (useSequentialStoredFieldsReader == false) { - releaseSequentialStoredFieldsReader(); // for better loading performance we sort the array by docID and // then visit all leaves in order. ArrayUtil.introSort(scoreDocs, Comparator.comparingInt(i -> i.doc)); } Suggestion importance[1-10]: 4 __ Why: The concern about staleness when flipping false→true is partially mitigated by the sequentialStoredFieldsLeafOrd != leaf.ord and thread ownership checks in storedFieldsFor, but proactively releasing the cache before recomputing is a reasonable defensive improvement. Impact is minor since existing checks likely handle this correctly.	Low
General	Avoid strong reference to owning thread Holding a Thread reference in a long-lived field can prevent that thread from being garbage-collected if the snapshot outlives it. Since only reference equality is needed, storing it as a WeakReference (or clearing it eagerly when the reader is re-acquired) avoids this leak risk while preserving the identity check. server/src/main/java/org/opensearch/index/engine/LuceneChangesSnapshot.java [104-106] private StoredFields sequentialStoredFields; private int sequentialStoredFieldsLeafOrd = -1; -private Thread sequentialStoredFieldsOwner; +private java.lang.ref.WeakReference<Thread> sequentialStoredFieldsOwner; Suggestion importance[1-10]: 2 __ Why: The snapshot is typically short-lived, so the practical risk of preventing thread GC is very low. This is a minor stylistic concern rather than a real bug.	Low

[github-actions]

❌ Gradle check result for 3ff67c4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[andrross]

FYI @jainankitk @rishabhmaurya

I'm deliberately starting small here. The check to use this optimization is narrow enough that I don't see any feasible downside to this. The INTERLEAVED case is the next case that would be worth optimizing in my opinion, as newly created segments under heavy indexing are often interleaved. Things get more complicated because we'll have to hold some of the decompressed state in memory, which means defining some bounds and more extensive testing to ensure we don't introduce regressions.

[github-actions]

❌ Gradle check result for 3ff67c4: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 3ff67c4: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.39%. Comparing base (0456d83) to head (3ff67c4).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...opensearch/index/engine/LuceneChangesSnapshot.java	93.93%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #22603   +/-   ##
=========================================
  Coverage     71.39%   71.39%           
- Complexity    76760    76797   +37     
=========================================
  Files          6148     6148           
  Lines        357951   357980   +29     
  Branches      52170    52177    +7     
=========================================
+ Hits         255556   255596   +40     
- Misses        82024    82077   +53     
+ Partials      20371    20307   -64     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.