[doris] branch master updated: [fix] (mem tracker) Refactor memtable mem tracker, fix flush memtable DCHECK failed (#10156)

[mo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

morningman pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git


The following commit(s) were added to refs/heads/master by this push:
     new 6ad024a2bf [fix] (mem tracker) Refactor memtable mem tracker, fix flush memtable DCHECK failed (#10156)
6ad024a2bf is described below

commit 6ad024a2bfa1d11c3bf5d7a65a7890c99ccbd15c
Author: Xinyi Zou <zo...@gmail.com>
AuthorDate: Sun Jun 19 16:48:42 2022 +0800

    [fix] (mem tracker) Refactor memtable mem tracker, fix flush memtable DCHECK failed (#10156)
    
    1. Added memory leak detection for `DeltaWriter` and `MemTable` mem tracker
    2. Modify memtable mem tracker to virtual to avoid frequent recursive consumption of parent tracker.
    3. Disable memtable flush thread attach memtable tracker, ensure that memtable mem tracker is completely accurate.
    4. Modify `memory_verbose_track=false`. At present, there is a performance problem in the frequent switch thread mem tracker.
          - Because the mem tracker exists as a shared_ptr in the thread local. Each time it is switched, the atomic variable use_count in the shared_ptr of the current tracker will be -1, and the tracker to be replaced use_count +1, multi-threading Frequent changes to the same tracker shared_ptr are slow.
          - TODO: 1. Reduce unnecessary thread mem tracker switch, 2. Consider using raw pointers for mem tracker in thread local.
---
 be/src/common/config.h                  |  9 ++++++++-
 be/src/olap/delta_writer.cpp            | 12 ++++++++++--
 be/src/olap/memtable.cpp                |  3 +++
 be/src/olap/memtable_flush_executor.cpp | 13 ++++++++++++-
 be/src/runtime/mem_tracker.cpp          |  6 +++++-
 be/src/runtime/mem_tracker.h            | 12 ++++++++++--
 6 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/be/src/common/config.h b/be/src/common/config.h
index 56e7382320..fd7ebdd395 100644
--- a/be/src/common/config.h
+++ b/be/src/common/config.h
@@ -635,7 +635,14 @@ CONF_Bool(track_new_delete, "true");
 
 // If true, switch TLS MemTracker to count more detailed memory,
 // including caches such as ExecNode operators and TabletManager.
-CONF_Bool(memory_verbose_track, "true");
+//
+// At present, there is a performance problem in the frequent switch thread mem tracker.
+// This is because the mem tracker exists as a shared_ptr in the thread local. Each time it is switched,
+// the atomic variable use_count in the shared_ptr of the current tracker will be -1, and the tracker to be
+// replaced use_count +1, multi-threading Frequent changes to the same tracker shared_ptr are slow.
+// TODO: 1. Reduce unnecessary thread mem tracker switches,
+//       2. Consider using raw pointers for mem tracker in thread local
+CONF_Bool(memory_verbose_track, "false");
 
 // Default level of MemTracker to show in web page
 // now MemTracker support two level:
diff --git a/be/src/olap/delta_writer.cpp b/be/src/olap/delta_writer.cpp
index 40f2851655..b088ef50cc 100644
--- a/be/src/olap/delta_writer.cpp
+++ b/be/src/olap/delta_writer.cpp
@@ -96,8 +96,10 @@ Status DeltaWriter::init() {
         return Status::OLAPInternalError(OLAP_ERR_TABLE_NOT_FOUND);
     }
 
-    _mem_tracker =
-            MemTracker::create_tracker(-1, "DeltaWriter:" + std::to_string(_tablet->tablet_id()));
+    // Only consume mem tracker manually in mem table. Using the virtual tracker can avoid
+    // frequent recursive consumption of the parent tracker, thereby improving performance.
+    _mem_tracker = MemTracker::create_virtual_tracker(
+            -1, "DeltaWriter:" + std::to_string(_tablet->tablet_id()));
     // check tablet version number
     if (_tablet->version_count() > config::max_tablet_version_num) {
         //trigger quick compaction
@@ -301,6 +303,11 @@ Status DeltaWriter::close_wait() {
     // return error if previous flush failed
     RETURN_NOT_OK(_flush_token->wait());
 
+    _mem_table.reset();
+    // In allocate/free of mem_pool, the consume_cache of _mem_tracker will be called,
+    // and _untracked_mem must be flushed first.
+    MemTracker::memory_leak_check(_mem_tracker.get());
+
     // use rowset meta manager to save meta
     _cur_rowset = _rowset_writer->build();
     if (_cur_rowset == nullptr) {
@@ -333,6 +340,7 @@ Status DeltaWriter::cancel() {
         // cancel and wait all memtables in flush queue to be finished
         _flush_token->cancel();
     }
+    MemTracker::memory_leak_check(_mem_tracker.get());
     _is_cancelled = true;
     return Status::OK();
 }
diff --git a/be/src/olap/memtable.cpp b/be/src/olap/memtable.cpp
index c8c3d78e2a..1b44d0ee25 100644
--- a/be/src/olap/memtable.cpp
+++ b/be/src/olap/memtable.cpp
@@ -103,6 +103,9 @@ void MemTable::_init_agg_functions(const vectorized::Block* block) {
 MemTable::~MemTable() {
     std::for_each(_row_in_blocks.begin(), _row_in_blocks.end(), std::default_delete<RowInBlock>());
     _mem_tracker->release(_mem_usage);
+    _buffer_mem_pool->free_all();
+    _table_mem_pool->free_all();
+    MemTracker::memory_leak_check(_mem_tracker.get(), true);
 }
 
 MemTable::RowCursorComparator::RowCursorComparator(const Schema* schema) : _schema(schema) {}
diff --git a/be/src/olap/memtable_flush_executor.cpp b/be/src/olap/memtable_flush_executor.cpp
index 30c8852904..29300c0cda 100644
--- a/be/src/olap/memtable_flush_executor.cpp
+++ b/be/src/olap/memtable_flush_executor.cpp
@@ -61,7 +61,18 @@ Status FlushToken::wait() {
 }
 
 void FlushToken::_flush_memtable(std::shared_ptr<MemTable> memtable, int64_t submit_task_time) {
-    SCOPED_ATTACH_TASK_THREAD(ThreadContext::TaskType::LOAD, memtable->mem_tracker());
+#ifndef BE_TEST
+    // The memtable mem tracker needs to be completely accurate,
+    // because DeltaWriter judges whether to flush memtable according to the memtable memory usage.
+    // The macro of attach thread mem tracker is affected by the destructuring order of local variables,
+    // so it cannot completely correspond to the number of new/delete bytes recorded in scoped,
+    // and there is a small range of errors. So direct track load mem tracker.
+    // TODO(zxy) After rethinking the use of switch thread mem tracker, choose the appropriate way to get
+    // load mem tracke here.
+    // DCHECK(memtable->mem_tracker()->parent_task_mem_tracker_no_own());
+    // SCOPED_ATTACH_TASK_THREAD(ThreadContext::TaskType::LOAD,
+    //                           memtable->mem_tracker()->parent_task_mem_tracker_no_own());
+#endif
     _stats.flush_wait_time_ns += (MonotonicNanos() - submit_task_time);
     SCOPED_CLEANUP({ memtable.reset(); });
     // If previous flush has failed, return directly
diff --git a/be/src/runtime/mem_tracker.cpp b/be/src/runtime/mem_tracker.cpp
index 824c8b8530..d53e975d64 100644
--- a/be/src/runtime/mem_tracker.cpp
+++ b/be/src/runtime/mem_tracker.cpp
@@ -162,9 +162,13 @@ MemTracker::MemTracker(int64_t byte_limit, const std::string& label,
 void MemTracker::init() {
     DCHECK_GE(_limit, -1);
     MemTracker* tracker = this;
-    while (tracker != nullptr && tracker->_virtual == false) {
+    while (tracker != nullptr) {
         _all_trackers.push_back(tracker);
         if (tracker->has_limit()) _limit_trackers.push_back(tracker);
+        // This means that it terminates when recursively consume/release from the current tracker up to the virtual tracker.
+        if (tracker->_virtual == true) {
+            break;
+        }
         tracker = tracker->_parent.get();
     }
     DCHECK_GT(_all_trackers.size(), 0);
diff --git a/be/src/runtime/mem_tracker.h b/be/src/runtime/mem_tracker.h
index 85a6550f7a..684f23ec9a 100644
--- a/be/src/runtime/mem_tracker.h
+++ b/be/src/runtime/mem_tracker.h
@@ -416,9 +416,17 @@ public:
 
     // If an ancestor of this tracker is a Task MemTracker, return that tracker. Otherwise return nullptr.
     MemTracker* parent_task_mem_tracker() {
-        MemTracker* tracker = this;
+        if (this->_level == MemTrackerLevel::TASK) {
+            return this;
+        } else {
+            return parent_task_mem_tracker_no_own().get();
+        }
+    }
+
+    std::shared_ptr<MemTracker> parent_task_mem_tracker_no_own() {
+        std::shared_ptr<MemTracker> tracker = this->_parent;
         while (tracker != nullptr && tracker->_level != MemTrackerLevel::TASK) {
-            tracker = tracker->_parent.get();
+            tracker = tracker->_parent;
         }
         return tracker;
     }


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org