Conversation
sollhui
requested review from
airborne12,
csun5285,
eldenmoon,
gavinchou,
liaoxin01 and
yiguolei
as code owners
September 21, 2026 03:15
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
### What problem does this PR solve? Related PR: apache#68304 SegmentCache retained large PK index roots while its LRU charge only reflected estimated metadata. Charge eagerly loaded roots at insertion and refresh the entry after lazy initialization, without charging the physical allocation twice. Grow LRU weights under the shard lock and preserve live handles and replacement identity. Discard partial readers on failed index initialization. ### Release note Reduce persistent PK root retention by accounting for pinned roots in SegmentCache eviction. Active readers can still retain pages beyond the cache budget, and more frequent eviction can increase index reloads. ### Check List (For Author) - Test: 11 new BE unit tests; execution blocked by dependency download timeouts. Actual low-level LRU source passed a standalone ASan/UBSan harness, including concurrent charge growth. Changed-file clang-format 16 and build hygiene pass. Full BE compilation and clang-tidy require CI (no compilation database locally). - Behavior changed: Yes, SegmentCache eviction includes loaded PK root bytes. - Does this need documentation: No.
sollhui
changed the title
### What problem does this PR solve? Related PR: apache#68304 Restore the original eager rowset PK-index loading and index-before-bloom-filter lookup order so the pinned-root SegmentCache charge change can be measured alone. Restore reader creation during index loading and replace BF-first tests with the six cache-charge tests. A definite bloom miss now verifies index-first loading and the corresponding cache charge increase. ### Release note Only the SegmentCache pinned-root accounting fix remains in this PR. ### Check List (For Author) - Test: Verified rowset loading and lookup implementation match baseline 695c88b. clang-format 16 and build hygiene passed. Full BE build/tests remain pending CI due to the previously observed dependency download failures. - Behavior changed: Yes, removes the earlier lazy-loading and BF-first changes. - Does this need documentation: No.
sollhui
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
TPC-H: Total hot run time: 28097 ms |
Contributor
TPC-DS: Total hot run time: 152651 ms |
Contributor
ClickBench: Total hot run time: 24.1 s |
### What problem does this PR solve? Related PR: apache#68304 A segment whose PK roots grow beyond its cache shard capacity evicts every other idle entry before being evicted itself. Remove cache ownership of the oversized entry immediately, preserving live handles and unrelated entries. Keep normal eviction when the growing entry can still fit in the shard. Update the Segment cache tests to the existing VerticalSegmentWriterOptions interface and matching build_segment helper signature. ### Release note Avoid unnecessary segment cache eviction when loaded PK roots exceed shard capacity. ### Check List (For Author) - Test: Added/updated BE unit tests for oversized entries, normal eviction, timestamp eviction, outstanding handles, and physical memory accounting. Extracted five low-level test bodies and the original LRU implementation into an external-dependency-stub harness: ASan/UBSan passed after the fix; the baseline failed the oversized-entry regression assertion. Changed-file clang-format 16, build hygiene and git diff --check passed. Full BE UT was attempted with CacheTest.UpdateCharge*:SegmentPrimaryKeyCacheTest.*; configuration failed before compilation because protoc, Boost and Snappy dependencies are missing. Full BE UT and clang-tidy remain pending. - Behavior changed: Yes; oversized growing entries are removed without evicting unrelated entries, while live handles retain ownership. - Does this need documentation: No
### What problem does this PR solve? Related PR: apache#68304 The eviction helpers use count >= limit to reserve a slot for insertion. Reusing them unconditionally for charge growth evicts an unrelated entry when the element limit is reached even if byte capacity is sufficient. Only invoke eviction from update_charge when usage exceeds capacity. ### Release note Avoid unnecessary segment cache eviction when updating entry size at the cache element-count limit. ### Check List (For Author) - Test: Added a BE regression test covering below, exactly at and above byte capacity while at the element-count limit, with both normal and timestamp eviction. Extracted production LRU code and six test bodies passed an ASan/UBSan harness with external dependency stubs; the new test fails on the preceding commit. Changed-file clang-format 16, build hygiene and git diff --check passed. Full BE UT and clang-tidy remain unavailable due to the previously confirmed missing protoc, Boost and Snappy dependencies. - Behavior changed: Yes; charge growth only initiates eviction when usage exceeds capacity, without reserving an additional element slot. - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
TPC-H: Total hot run time: 27724 ms |
Contributor
TPC-DS: Total hot run time: 152719 ms |
Contributor
ClickBench: Total hot run time: 24.09 s |
Contributor
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
What problem does this PR solve?
SegmentCache retained large PK index roots while its LRU charge only reflected
estimated metadata. With long keys, a metadata-sized cache entry can pin hundreds
of MiB of decompressed index pages and prevent the PK page cache from reclaiming
them.
The ownership chain is:
The reader's
PageHandleand PKIndexPageCache refer to the same page allocation,not separate copies. Keeping a Segment in SegmentCache can therefore keep its
loaded PK roots pinned even after the query finishes. Evicting the SegmentCache
entry removes cache ownership; once all remaining Segment references are released,
the reader releases its page handles and the PK page cache can reclaim those pages.
Charge loaded PK roots to SegmentCache eviction, both at insertion and when a
cached Segment initializes its PK index later. Physical memory remains tracked
by the PK page allocator; the additional eviction weight is not counted again
by MemTracker. Updates use the shard lock and check Segment identity so an old
instance cannot charge its replacement. The callback holds only a weak cache
reference. Failed index initialization discards partial readers and their page
handles.
The original eager rowset PK-index loading and index-before-bloom-filter lookup
order are preserved. This PR now isolates SegmentCache accounting; it does not
include the earlier segment lazy-loading or BF-first lookup optimizations.
This limits cache retention, not active-operation memory. Evicted Segments remain
valid through callers' shared pointers, and more frequent eviction can increase
index reloads.
Validation
monotonic/concurrent charge updates, eviction, active handles, replacement,
initialization before/after insertion, cache lifetime and partial-load failure.
695c88b5772.ASan/UBSan scenarios using external logging/time/value-base stubs. This is
supplementary validation, not a full BE unit-test run.
downloads repeatedly timed out before compilation in the local environment.
Test filter:
CacheTest.UpdateCharge*:SegmentPrimaryKeyCacheTest.*.from 9.25/8.93 GiB to a few MiB.
Before/after memory validation
The before/after test runs show a change from sustained memory growth to
repeated memory reclamation:
OOM. In the captured dashboard window, the three BEs had mean memory usage of
28.7–35.7 GiB, with the highest observed peak reaching 55.3 GiB.
transient spikes instead of maintaining the previous upward trend. In the
captured dashboard window, the three BEs had mean memory usage of
8.33–19.2 GiB, with the highest observed peak reaching 45.3 GiB.
The dashboard measurements above come from separate runs on different BE groups
and time windows.
These results validate the observed memory-retention improvement; transient
memory peaks remain, and no throughput comparison is claimed.
Release note
Account for pinned PK index roots in SegmentCache eviction to reduce persistent
index memory retention.