No tracing memory usage of shared column data in MPPTask's memory tracker#8131
No tracing memory usage of shared column data in MPPTask's memory tracker#8131ti-chi-bot[bot] merged 9 commits intopingcap:masterfrom
Conversation
|
Hi @JinheLin. Thanks for your PR. I'm waiting for a pingcap member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/run-all-tests |
a30c5c9 to
ef1801d
Compare
ef1801d to
a1bb74a
Compare
|
/run-all-tests |
|
/run-integration-test |
|
/run-all-tests |
|
/run-unit-test |
Co-authored-by: JaySon <tshent@qq.com>
|
/run-all-tests |
| if (!next.load(std::memory_order_relaxed)) | ||
| { | ||
| CurrentMetrics::add(metric, size); | ||
| if (shared_column_data_mem_tracker) |
There was a problem hiding this comment.
shared_column_data_mem_tracker is a global object and has initialized when server starting. I think there is no data race on it after its initialization completed.
There was a problem hiding this comment.
Yes, I aggree, but theoretically there will be data race here, not sure if our ASan tests will complain it or not. I'm ok with current implementation, and I think we can fix it if it breaks the ASan tests.
| // So the allocation and deallocation of this data may not be in the same MemoryTracker. | ||
| // This can lead to inaccurate memory statistics of MemoryTracker. | ||
| // To solve this problem, we use a independent global memory tracker to trace the shared column data in ColumnSharingCacheMap. | ||
| MemoryTrackerSetter mem_tracker_guard(true, has_concurrent_reader ? nullptr : current_memory_tracker); |
There was a problem hiding this comment.
Don't have to use this mem_tracker_guard if has_concurrent_reader is false?
| if (!next.load(std::memory_order_relaxed)) | ||
| { | ||
| CurrentMetrics::add(metric, size); | ||
| if (shared_column_data_mem_tracker) |
There was a problem hiding this comment.
Yes, I aggree, but theoretically there will be data race here, not sure if our ASan tests will complain it or not. I'm ok with current implementation, and I think we can fix it if it breaks the ASan tests.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, windtalker The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/run-all-tests |
|
@JinheLin: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests
If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
/run-unit-test |
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
What problem does this PR solve?
Issue Number: close #8128
What is changed and how it works?
DMFileReader::readCoumn, if there are some concurrentDMFileReaderof the same file existing, this column data is likely to be shared, so disable memory tracing by settingcurrent_memory_trackertonullptr.PODArraythe underlying memory of column data to beis_shared_memoryand useshared_column_data_mem_trackerto trace its memory.Before this PR(from issue #8128)
After this PR:
Check List
Tests
Side effects
Documentation
Release note