Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] (mem tracker) Fix some memory leaks, inaccurate statistics, core dump, deadlock bugs #10072

Merged
merged 2 commits into from
Jun 14, 2022

Conversation

xinyiZzz
Copy link
Contributor

@xinyiZzz xinyiZzz commented Jun 10, 2022

Proposed changes

Issue Number: close #10006 #10071

Problem Summary:

  1. Fix the memory leak. When the load task is canceled, the IndexChannel and NodeChannel mem trackers cannot be destructed in time.

  2. Fix Load task being frequently canceled by oom and inaccurate LoadChannel mem tracker limit, and rewrite the variable name of mem limit in LoadChannel.

  3. Fix core dump, when logout task mem tracker, phmap erase fails, resulting in repeated logout of the same tracker.

  4. Fix the deadlock, when add_child_tracker mem limit exceeds, calling log_usage causes _child_trackers_lock deadlock.

  5. Fix frequent log printing when thread mem tracker limit exceeds, which will affect readability and performance.

  6. Optimize some details of mem tracker display.

Checklist(Required)

  1. Does it affect the original behavior: (No】)
  2. Has unit tests been added: (No Need)
  3. Has document been added or modified: (No Need)
  4. Does it need to update dependencies: (No)
  5. Are there any changes that cannot be rolled back: (No)

Further comments

If this is a relatively large or complex change, kick off the discussion at [email protected] by explaining why you chose the solution you did and what alternatives you considered, etc...

@xinyiZzz xinyiZzz requested a review from dataroaring June 10, 2022 15:07
@xinyiZzz xinyiZzz force-pushed the fix_memtracker_0608 branch from d9df8ce to 590dc6a Compare June 11, 2022 17:42
@morningman morningman added kind/fix Categorizes issue or PR as related to a bug. area/memory-consumption labels Jun 12, 2022
@morningman morningman added this to the v1.2 milestone Jun 12, 2022
@@ -124,14 +124,17 @@ std::shared_ptr<MemTracker> MemTracker::create_tracker_impl(
std::string reset_label;
MemTracker* task_parent_tracker = reset_parent->parent_task_mem_tracker();
if (task_parent_tracker) {
reset_label = fmt::format("{}:{}", label, split(task_parent_tracker->label(), ":")[1]);
reset_label = fmt::format("{}&{}", label, split(task_parent_tracker->label(), "&")[1]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change to &?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because : has many meanings as a separator in task mem tracker label, and & is only used to split out queryId or loadID or tabletID.

The latest code replaces & with #, which seems a little nicer :)

}
}
for (auto tid : expired_tasks) {
// This means that after all RuntimeState is destructed,
// there are still task mem trackers that are get or register.
// The only known case: after an load task ends all fragments on a BE,`tablet_writer_open` is still
// called to create a channel, and the load task tracker will be re-registered in the channel open.
// https://github.com/apache/incubator-doris/issues/9905
if (_task_mem_trackers[tid].use_count() == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_task_mem_trackers[tid] maybe nullptr here?
Because you add a case if (!it->second) before.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it's a bug,
fixed, thks~

@morningman
Copy link
Contributor

And need to rebase to make p0 happy

@xinyiZzz xinyiZzz force-pushed the fix_memtracker_0608 branch from 590dc6a to 6b3fede Compare June 12, 2022 16:13
@xinyiZzz
Copy link
Contributor Author

And need to rebase to make p0 happy

done

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Jun 13, 2022
@github-actions
Copy link
Contributor

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

PR approved by anyone and no changes requested.

@morningman morningman merged commit 85362a9 into apache:master Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. area/memory-consumption kind/fix Categorizes issue or PR as related to a bug. reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] memtracker heap-use-after-free
3 participants