[fix](be) Fix DCHECK in LocalExchangeSharedState::sub_total_mem_usage#63742
Open
jacktengg wants to merge 1 commit into
Open
[fix](be) Fix DCHECK in LocalExchangeSharedState::sub_total_mem_usage#63742jacktengg wants to merge 1 commit into
jacktengg wants to merge 1 commit into
Conversation
Issue Number: close #xxx
Problem Summary: In LocalExchangeSharedState::sub_total_mem_usage(),
`mem_usage` is `std::atomic<int64_t>` but `delta` is `size_t`. The
existing debug check
DCHECK_GE(prev_usage - delta, 0);
was never effective: the usual arithmetic conversions promote
`prev_usage - delta` to `size_t`, and an unsigned expression is
trivially `>= 0`. So the guard against `mem_usage` underflow
(subtracting more than was added) silently passed in all debug builds,
leaving any over-subtraction undetected.
Fix: compare `prev_usage` (int64_t) against `cast_set<int64_t>(delta)`
so the comparison is performed entirely in signed space, and a real
underflow will actually trip the DCHECK with the original prev_usage
and delta values in the failure message. The release-mode guard on the
next line (`cast_set<int64_t>(prev_usage - delta)` throws on underflow
because the wrapped size_t result exceeds INT64_MAX) is preserved as-is.
Contributor
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Review result: no blocking issues found.
Critical checkpoint conclusions:
- Goal and proof: the PR fixes an ineffective debug underflow check in
LocalExchangeSharedState::sub_total_mem_usage; the new signed comparison directly validatesprev_usage >= deltabefore the existing post-subtraction threshold check. No new test is included, but this is a narrow DCHECK-only correction. - Scope and clarity: the modification is minimal and focused on the broken assertion.
- Concurrency: the changed code remains on the existing atomic
mem_usage.fetch_subpath; no new shared state, locks, dependency transitions, or lock ordering are introduced. - Lifecycle/static initialization: no new objects or lifecycle management are introduced.
- Configuration/compatibility: no config, protocol, storage, or rolling-upgrade compatibility impact.
- Parallel code paths: this is the local-exchange total memory accounting path; no separate changed path appears necessary for this assertion fix.
- Error handling and invariants: using DCHECK for the invariant is appropriate, and the existing release-path
cast_setbehavior is preserved. - Memory safety/performance/observability: no allocations, ownership changes, hot-path extra work in release builds, or observability requirements are introduced.
- Tests: no test result changes. Given the change only corrects a debug assertion expression, existing coverage is acceptable.
User focus: no additional user-provided review focus was specified.
Contributor
Author
|
run buildall |
Contributor
TPC-H: Total hot run time: 31494 ms |
Contributor
TPC-DS: Total hot run time: 172108 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
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.
Issue Number: close #xxx
Problem Summary: In LocalExchangeSharedState::sub_total_mem_usage(),
mem_usageisstd::atomic<int64_t>butdeltaissize_t. The existing debug checkwas never effective: the usual arithmetic conversions promote
prev_usage - deltatosize_t, and an unsigned expression is trivially>= 0. So the guard againstmem_usageunderflow (subtracting more than was added) silently passed in all debug builds, leaving any over-subtraction undetected.Fix: compare
prev_usage(int64_t) againstcast_set<int64_t>(delta)so the comparison is performed entirely in signed space, and a real underflow will actually trip the DCHECK with the original prev_usage and delta values in the failure message. The release-mode guard on the next line (cast_set<int64_t>(prev_usage - delta)throws on underflow because the wrapped size_t result exceeds INT64_MAX) is preserved as-is.What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)