Using cached StoredFieldsReader for scroll query optimizations #20112
+447
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Prudhvi Godithi <[email protected]>
WalkthroughAdds per-segment StoredFieldsReader caching for scrolls: ScrollContext gains a cache, caching APIs, and close semantics; FetchPhase uses the cache for active scroll fetches; LegacyReaderContext registers scroll contexts for lifecycle cleanup. New unit and integration tests validate caching and several test classes receive codec-suppression annotations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant FetchPhase
participant ScrollContext
participant StoredFieldsReader
Client->>FetchPhase: fetch request (scroll)
activate FetchPhase
FetchPhase->>FetchPhase: is scroll active?
alt scroll active
FetchPhase->>ScrollContext: getCachedSequentialReader(segmentKey)
activate ScrollContext
alt cached
ScrollContext-->>FetchPhase: cached StoredFieldsReader
else not cached
ScrollContext-->>FetchPhase: null
FetchPhase->>StoredFieldsReader: create sequential reader
activate StoredFieldsReader
StoredFieldsReader-->>FetchPhase: new reader
deactivate StoredFieldsReader
FetchPhase->>ScrollContext: cacheSequentialReader(segmentKey, reader)
end
deactivate ScrollContext
FetchPhase->>StoredFieldsReader: use reader to fetch stored fields
else not a scroll
FetchPhase->>StoredFieldsReader: get sequential reader (no cache)
StoredFieldsReader-->>FetchPhase: reader
end
FetchPhase-->>Client: return documents
deactivate FetchPhase
Client->>ScrollContext: close scroll
activate ScrollContext
ScrollContext->>StoredFieldsReader: close each cached reader
ScrollContext->>ScrollContext: clear cache
deactivate ScrollContext
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2025-12-02T22:44:14.761ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
🔇 Additional comments (3)
Comment |
Member
Author
|
{"run-benchmark-test": "id_3"} |
Member
Author
|
{"run-benchmark-test": "id_11"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/5213/ . Final results will be published once the job is completed. |
Contributor
|
❌ Gradle check result for 56dca5e: 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? |
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/5213/
|
Collaborator
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/214/
|
Signed-off-by: Prudhvi Godithi <[email protected]>
Member
Author
|
{"run-benchmark-test": "id_3"} |
prudhvigodithi
changed the title
Member
Author
|
{"run-benchmark-test": "id_3"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/5214/ . Final results will be published once the job is completed. |
Contributor
|
❌ Gradle check result for c5e1e19: 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? |
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/5214/
|
Collaborator
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/215/
|
Member
Author
|
{"run-benchmark-test": "id_3"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/5224/ . Final results will be published once the job is completed. |
Member
Author
|
{"run-benchmark-test": "id_11"} |
Contributor
|
❌ Gradle check result for c5e1e19: TIMEOUT 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? |
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/5224/
|
Collaborator
Benchmark Baseline Comparison ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-compare/216/
|
Member
Author
|
{"run-benchmark-test": "id_11"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/5226/ . Final results will be published once the job is completed. |
Member
Author
|
{"run-benchmark-test": "id_3"} |
Contributor
|
The Jenkins job url is https://build.ci.opensearch.org/job/benchmark-pull-request/5227/ . Final results will be published once the job is completed. |
Collaborator
Benchmark ResultsBenchmark Results for Job: https://build.ci.opensearch.org/job/benchmark-pull-request/5226/
|
prudhvigodithi
requested review from
a team,
saratvemulapalli,
shwetathareja and
sohami
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/org/opensearch/search/fetch/FetchPhase.java (1)
193-214: Verify thread-safety guarantee for cached StoredFieldsReader in ScrollContextThe change introduces per-segment caching of
StoredFieldsReaderviaScrollContext, which violates Lucene's documented constraint thatStoredFieldsReaderis stateful and must only be consumed by a single thread ("should only be consumed in the thread where it was acquired"). Lucene'sAssertingStoredFieldsFormat(test framework) enforces this via runtime assertions.While standard OpenSearch scrolls are designed for sequential per-scroll-ID execution (matching the single-thread requirement), the provided code snippet contains no explicit synchronization or documentation confirming that
ScrollContextreaders cannot be accessed concurrently. Additionally, OpenSearch's concurrent-segment search feature may cause a singleScrollContextto be accessed by multiple threads from the index_searcher thread pool, violating Lucene's contract.Clarify one of the following:
- Single-thread guarantee: Document that
ScrollContextcached readers are guaranteed to be accessed by only one thread over the context's lifetime, and confirm this holds even with concurrent-segment search enabled.- Thread-safe caching alternative: If concurrent access is possible, redesign to avoid sharing stateful
StoredFieldsReaderinstances across threads (e.g., thread-local caching or per-thread reader instances).Without this clarity, the caching introduces a latent concurrency correctness risk in the fetch pipeline.
🧹 Nitpick comments (2)
server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollIT.java (1)
96-107:@SuppressCodecs("*")is very broad for the whole classApplying
@LuceneTestCase.SuppressCodecs("*")at class level disables all asserting/test codecs for every test here, not just those that exercise the StoredFieldsReader cache. That reduces Lucene’s safety checks for many unrelated scroll tests.If possible, consider:
- Narrowing the suppression to just the Asserting codec (e.g.,
"Asserting") or a smaller set of codecs actually causing failures, and/or- Applying the suppression at the individual test level that depends on cross-thread reader reuse, so the rest of the suite still benefits from assertion-heavy codecs.
This keeps the workaround as localized as possible while still allowing the new optimization tests to run.
server/src/internalClusterTest/java/org/opensearch/search/scroll/ScrollStoredFieldsCacheIT.java (1)
32-39: Broad codec suppression reduces Lucene’s test assertions here as wellSimilar to
SearchScrollIT, this class-level@LuceneTestCase.SuppressCodecs("*")disables all asserting codecs for every test in this class. That’s a heavy hammer just to work around theStoredFieldsReaderthread-affinity checks.If feasible, consider:
- Suppressing only the Asserting codec (or the minimal problematic set), and/or
- Restricting the suppression to the specific tests that require cross-thread reader reuse.
That would keep the rest of the scroll cache integration tests running under Lucene’s full assertion coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java(2 hunks)server/src/internalClusterTest/java/org/opensearch/search/scroll/ScrollStoredFieldsCacheIT.java(1 hunks)server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollIT.java(4 hunks)server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollWithFailingNodesIT.java(2 hunks)server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java(2 hunks)server/src/main/java/org/opensearch/search/fetch/FetchPhase.java(2 hunks)server/src/main/java/org/opensearch/search/internal/LegacyReaderContext.java(1 hunks)server/src/main/java/org/opensearch/search/internal/ScrollContext.java(1 hunks)server/src/test/java/org/opensearch/search/internal/ScrollContextReaderCacheTests.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java (3)
server/src/internalClusterTest/java/org/opensearch/search/scroll/ScrollStoredFieldsCacheIT.java (1)
LuceneTestCase(38-151)server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollIT.java (1)
LuceneTestCase(106-913)server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollWithFailingNodesIT.java (1)
LuceneTestCase(68-142)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
🔇 Additional comments (9)
server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java (1)
84-96: Well-documented codec suppression for scroll reader caching.The Javadoc clearly explains why
@SuppressCodecs("*")is necessary and references the relevant method. This is consistent with other test files in this PR (ScrollStoredFieldsCacheIT, SearchScrollIT).Note: Suppressing all codecs (
"*") is broader than strictly necessary (onlyAssertingStoredFieldsFormatcauses the thread affinity assertion). However, as mentioned in the PR discussion, more targeted suppression (e.g.,@SuppressAssertingFormats) is not currently available in the Lucene test framework.CHANGELOG.md (1)
38-38: LGTM!The changelog entry is concise, accurately describes the optimization, and follows the established format with proper PR link.
server/src/main/java/org/opensearch/search/internal/LegacyReaderContext.java (1)
84-85: Essential lifecycle management for cached readers.Registering
scrollContextwithaddOnClose()ensures that cachedStoredFieldsReaderinstances are properly released when the context is closed. This follows the same pattern used for the delegate searcher on line 74 and prevents resource leaks.server/src/internalClusterTest/java/org/opensearch/search/stats/SearchStatsIT.java (1)
83-91: LGTM!The annotation and documentation are consistent with the pattern applied to other scroll-related test classes in this PR. The
testOpenContexts()method exercises scroll functionality, so this suppression is necessary.server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollWithFailingNodesIT.java (1)
60-68: LGTM!The annotation and documentation follow the established pattern. This test class explicitly exercises scroll functionality with node failures, making the codec suppression necessary to support the cached reader optimization.
server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollIT.java (1)
838-904: New scroll test is solid, but it also relies on the contentious reader caching semanticsThe new test does a good job validating:
- No duplicates across batches.
- Correct sort order and full coverage of
0..numDocs-1._sourceintegrity for both numeric and text fields.- Multiple scroll batches are actually exercised.
However, note that this test’s correctness predicates on the cross-request
StoredFieldsReaderreuse being safe (the behavior under debate with Lucene’s asserting formats). If the underlying contract changes or we later adjust caching semantics, this test will either keep passing while hiding subtle threading bugs, or start failing in ways that are hard to diagnose.No code changes required here, but please keep this test aligned with whatever thread-safety story we ultimately settle on for
ScrollContext’s cache; it’s a good candidate place to encode any new invariants (for example, asserting we are always using the same access thread if you enforce that server-side).server/src/test/java/org/opensearch/search/internal/ScrollContextReaderCacheTests.java (1)
30-107: Unit coverage for cache lifecycle and error handling looks goodThese tests nicely cover the core behaviors of
ScrollContext’s cache:
- Put/get semantics per segment key.
- Proper closure of all cached readers and cache clearing.
- Safe handling of
IOExceptionfrom a single reader without skipping others.- Multi-segment caching across multiple “batches”.
Given the production code’s current single-close semantics, this looks sufficient. If in future we make
ScrollContext.close()potentially re-entrant or callable from multiple places, you might want to add a test that callingclose()twice is safe and doesn’t attempt to close already-closed readers, but that’s an optional enhancement.server/src/internalClusterTest/java/org/opensearch/search/scroll/ScrollStoredFieldsCacheIT.java (1)
53-150: Integration tests exercise the cache well across single and multi-segment scenariosBoth tests do a good job of validating the new optimization end-to-end:
- They run under both settings of
CLUSTER_CONCURRENT_SEGMENT_SEARCH_SETTING.testScrollWithSequentialReaderCachechecks:
- No duplicate doc IDs.
- Correct
_sourcecontents for each hit.- All documents are retrieved via scroll.
testScrollAcrossMultipleSegmentsforces multiple segments via per-batch refreshes and asserts:
- Complete coverage of all expected documents.
- The number of scroll batches lines up with the computed
expectedBatches, which indirectly validates the batching behavior.These should give solid confidence that the reader cache doesn’t drop or duplicate hits under typical scroll patterns. Once the underlying threading story for the cache is clarified/adjusted, these tests will be valuable for guarding against regressions.
server/src/main/java/org/opensearch/search/internal/ScrollContext.java (1)
58-90: ScrollContext cache lacks thread-safety guarantees and should document its limitationsThe new unsynchronized cache presents thread-safety concerns worth addressing:
sequentialReaderCacheis unsynchronized with no per-context locking, making it vulnerable to data races if multiple threads access the sameScrollContextinstance concurrently.- The Javadoc asserts safety based on "serialized" client requests, but OpenSearch's scroll API allows concurrent access through sliced scrolls (each with independent scroll contexts), not through concurrent requests on a single scroll_id. If the server permits reusing a single
ScrollContextacross threads (even serially), it contradicts Lucene's documented restrictions on reader reuse, which is why tests need to suppress asserting codecs.Recommendations:
- If a single
ScrollContextmust be reusable across different threads, either add explicit per-context synchronization or redesign to use a per-thread cache to avoid cross-thread reader sharing.- Update the Javadoc to clearly state whether this cache is intended only for single-threaded per-request use or if cross-thread reuse is supported, and document the implications for Lucene's reader contract.
- The exception swallowing in
close()is acceptable given the current test coverage, but consider adding debug-level logging for repeated failures to aid leak diagnostics.
Member
Author
|
@msfroh @jainankitk @rishabhmaurya I have changed this PR ready to review. Please take a look and let me know if we can ship this. Thanks |
Contributor
|
Hello! |
Contributor
|
Benchmark shows ~19% improvement, very nice! |
Contributor
|
❌ Gradle check result for 1ba5185: 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? |
Signed-off-by: Prudhvi Godithi <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java (1)
84-92: Scope and assumptions behind@SuppressCodecs("*")The explanatory Javadoc is helpful, and aligning this IT with other scroll-related tests using
@LuceneTestCase.SuppressCodecs("*")makes the intent clear. Two points to tighten up:
- The comment asserts that different scroll batches may run on different threads “(but never concurrently)”. Since
StoredFieldsReaderis stateful, it would be good to explicitly treat this as an invariant you rely on (and ideally reference or enforce it nearScrollContext#getCachedSequentialReader), so future refactors don’t accidentally introduce concurrent use of the same cached reader.@LuceneTestCase.SuppressCodecsis documented as avoiding certain codec types; using"*"broadly disables randomized codecs for this class. Please double‑check against the Lucene version in this repo that"*"has the intended effect (primarily avoidingAssertingCodec/AssertingStoredFieldsFormatwhile still exercising a realistic production codec), and consider narrowing the suppression if feasible so you retain more codec coverage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java(2 hunks)server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java (1)
server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java (1)
LuceneTestCase(92-460)
server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java (3)
server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java (1)
LuceneTestCase(81-369)server/src/internalClusterTest/java/org/opensearch/search/scroll/ScrollStoredFieldsCacheIT.java (1)
LuceneTestCase(38-151)server/src/internalClusterTest/java/org/opensearch/search/scroll/SearchScrollIT.java (1)
LuceneTestCase(106-913)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: check-files
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
🔇 Additional comments (3)
server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java (2)
37-37: LGTM!The import is necessary for the
@LuceneTestCase.SuppressCodecsannotation used on the test class.
74-77: Verify thread safety guarantees for cross-thread StoredFieldsReader access.The javadoc states that "different batches may run on different threads (but never concurrently)." However, thread pool execution patterns don't inherently guarantee sequential access ordering or that concurrent access cannot occur under load. Given that Lucene maintainers have confirmed StoredFieldsReader is stateful and should not be shared across threads, verify that ScrollContext's caching implementation includes appropriate synchronization mechanisms (locks, synchronized blocks, or thread-local storage) to enforce the sequential cross-thread access guarantee.
server/src/internalClusterTest/java/org/opensearch/search/basic/TransportTwoNodesSearchIT.java (1)
37-37: Import forLuceneTestCaseis appropriateThe new import is required for the
@LuceneTestCase.SuppressCodecsannotation and is consistent with other scroll-related ITs using the same annotation.
server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java
Show resolved
Hide resolved
Contributor
|
❌ Gradle check result for 94d599a: 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? |
Member
Author
|
For #20112 (comment) I have a PR in Lucene apache/lucene#15468, Once agreed and released we can use |
Contributor
|
❌ Gradle check result for 94d599a: 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? |
Member
Author
|
The The automation should create a flaky issue for |
Contributor
|
❌ Gradle check result for 94d599a: 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? |
Contributor
|
❌ Gradle check result for 94d599a: 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? |
Signed-off-by: Prudhvi Godithi <[email protected]>
Contributor
|
❌ Gradle check result for 1cc0d36: 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? |
Contributor
Member
Author
|
@msfroh @rishabhmaurya Just a gentle ping. |
Member
Author
|
Tagging @jainankitk, please take a look. This should be a good improvement and we can add it for 3.4.0 opensearch-project/opensearch-build#5764. |
msfroh
approved these changes
Dec 4, 2025
Contributor
msfroh
left a comment
msfroh
left a comment
There was a problem hiding this comment.
I believe this should be good.
From my understanding of LegacyReaderContext and ReaderContext, this will work with scrolls, but not with PITs, which should be fine. In general, I don't think we can guarantee that PITs will access documents sequentially.
Incidentally, this should yield a significant improvement for any subclass of AbstractBulkByScrollRequest -- that is update-by-query, delete-by-query, and reindex.
Can you try benchmarking a large reindex operation with and without this change? I'd be curious to see the impact. (I imagine it would be decent, depending on how much time is spent on the read side versus the write side of the reindex task.)
github-project-automation
bot
moved this from Todo
to In Progress
in Performance Roadmap
Member
Author
Thanks Froh. Make sense as if i'm not wrong |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When testing the big5 scroll query, in the following flamegraph I can see the
Lucene90CompressingStoredFieldsReaderis called multiple times for all the scroll batches. The idea is cache the StoredFieldsReader in a private field within SequentialStoredFieldsLeafReader. The cached reader is reused across all subsequent calls to getSequentialStoredFieldsReader() for the lifetime of the SequentialStoredFieldsLeafReader instance.Related Issues
There is a regression issue reported in past #16262. This enhancement could help reduce the scroll API regressions.
Check List
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.
Summary by CodeRabbit
Bug Fixes
New Features / Performance
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.