Skip to content

Conversation

@xinyiZzz
Copy link
Contributor

@xinyiZzz xinyiZzz commented Jul 11, 2022

Proposed changes

Issue Number: close #10741

Problem Summary:

Usability issues

The new MemTracker is difficult to use, hoping to reduce the cost of use, mainly:

  1. SCOPED_SWITCH_xxx_TRACKER related macros for switching TLS trackers are various and depend on each other, which is difficult to use;

    Most of the macros are mainly to improve performance and code robustness, because MemTracker currently uses std::shared_ptr in TLS to have performance problems under multi-threading, which is solved by a complex caching mechanism.

  2. Creating a new MemTracker is error-prone;

    Because the hierarchical relationship between MemTrackers needs to be considered to avoid statistical errors affecting the accuracy of the overall statistics;

Solution

Divide MemTracker into two subclasses

1. MemTrackerLimiter

The first type of MemTrackerLimiter, including four layers of process, task pool, query/load task tracker, and instance tracker, is used to track and limit the memory usage of process and query.

  1. Manual consumption of Tracker is prohibited, only automatic consumption in TCMalloc Hook is allowed to ensure accuracy.

  2. PageCache transfers memory ownership only when memory allocation and release occur.

  3. All MemTrackers use raw pointers to avoid performance problems caused by frequent switching of MemTrackers in tls;

2. MemTrackerObserve

The second type of MemTrackerObserve, all operators and other MemTrackers, are only used to track the memory usage of the specified code segment,

  1. There is no parent-child relationship between MemTrackerObserves. Both fathers are instance trakcers, but their consumption will not consume instance trakcers synchronously. Therefore, errors in statistics will not affect the memory tracking and restrictions of processes and Query;

  2. When the Hook consumes the thread tls MemTracker, it will consume the MemTrackerLimiter and all MemTrackerObserves respectively.

Checklist(Required)

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

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actions github-actions bot added area/vectorization kind/docs Categorizes issue or PR as related to documentation. labels Jul 11, 2022
@xinyiZzz xinyiZzz force-pushed the mem_tracker_factor_v2 branch from 8ff9f28 to 3647649 Compare July 11, 2022 16:12
@xinyiZzz xinyiZzz force-pushed the mem_tracker_factor_v2 branch from 3647649 to 5059b01 Compare July 12, 2022 06:36
Copy link
Contributor

@yiguolei yiguolei left a comment

Choose a reason for hiding this comment

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

LGTM

@yiguolei yiguolei merged commit 41f9ee2 into apache:master Jul 12, 2022
eldenmoon pushed a commit to eldenmoon/incubator-doris that referenced this pull request Aug 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/vectorization kind/docs Categorizes issue or PR as related to documentation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Enhancement] Refactor to improve the usability of MemTracker

2 participants