You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@doris.apache.org by GitBox <gi...@apache.org> on 2021/11/23 13:36:53 UTC
[GitHub] [incubator-doris] morningman commented on a change in pull request #7198: [Memory] Use TCMalloc Hook to count the real Process and Query MemTracker
morningman commented on a change in pull request #7198:
URL: https://github.com/apache/incubator-doris/pull/7198#discussion_r755108387
##########
File path: be/src/service/doris_main.cpp
##########
@@ -275,6 +276,9 @@ int main(int argc, char** argv) {
#if defined(LEAK_SANITIZER)
__lsan_do_leak_check();
#endif
+ LOG(INFO) << "Tcmalloc New number: " << new_hook_calls
Review comment:
Better not print log here, use metrics
##########
File path: be/src/exec/olap_scan_node.cpp
##########
@@ -1652,6 +1654,7 @@ void OlapScanNode::scanner_thread(OlapScanner* scanner) {
// and transfer thread
_scan_batch_added_cv.notify_one();
_scan_thread_exit_cv.notify_one();
+ current_thread.update_mem_tracker(nullptr);
Review comment:
The thread may be returned early.
##########
File path: be/src/runtime/thread_status.h
##########
@@ -0,0 +1,78 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#pragma once
+
+#include <string>
+
+#include "runtime/exec_env.h"
+#include "runtime/mem_tracker.h"
+
+namespace doris {
+
+class ThreadStatus {
Review comment:
Maybe `TheadContext` is better?
##########
File path: be/src/runtime/mem_tracker.cpp
##########
@@ -242,26 +243,38 @@ int64_t MemTracker::GetPoolMemReserved() {
return mem_reserved;
}
-std::shared_ptr<MemTracker> PoolMemTrackerRegistry::GetRequestPoolMemTracker(
- const string& pool_name, bool create_if_not_present) {
- DCHECK(!pool_name.empty());
- lock_guard<SpinLock> l(pool_to_mem_trackers_lock_);
- PoolTrackersMap::iterator it = pool_to_mem_trackers_.find(pool_name);
- if (it != pool_to_mem_trackers_.end()) {
- MemTracker* tracker = it->second.get();
- DCHECK(pool_name == tracker->pool_name_);
- return it->second;
+std::shared_ptr<MemTracker> QueryMemTrackerRegistry::RegisterQueryMemTracker(
+ const std::string& query_id, int64_t mem_limit) {
+ DCHECK(!query_id.empty());
+ if (mem_limit != -1) {
+ if (mem_limit > MemInfo::physical_mem()) {
+ LOG(WARNING) << "Memory limit " << PrettyPrinter::print(mem_limit, TUnit::BYTES)
+ << " exceeds physical memory of "
+ << PrettyPrinter::print(MemInfo::physical_mem(), TUnit::BYTES);
+ }
+ VLOG(2) << "Using query memory limit: " << PrettyPrinter::print(mem_limit, TUnit::BYTES);
}
- if (!create_if_not_present) return nullptr;
- // First time this pool_name registered, make a new object.
- std::shared_ptr<MemTracker> tracker = MemTracker::CreateTracker(
- -1, strings::Substitute(REQUEST_POOL_MEM_TRACKER_LABEL_FORMAT, pool_name),
- ExecEnv::GetInstance()->process_mem_tracker());
- tracker->pool_name_ = pool_name;
- pool_to_mem_trackers_.emplace(pool_name, std::shared_ptr<MemTracker>(tracker));
+
+ // First time this query_id registered, make a new object, otherwise do nothing.
+ _query_mem_trackers.try_emplace_l(
+ query_id, [](std::shared_ptr<MemTracker>) {},
+ MemTracker::CreateTracker(mem_limit,
+ strings::Substitute(QUERY_MEM_TRACKER_LABEL_FORMAT, query_id),
+ ExecEnv::GetInstance()->hook_process_mem_tracker(), false,
+ false, MemTrackerLevel::OVERVIEW, query_id));
+
+ std::shared_ptr<MemTracker> tracker = nullptr;
+ _query_mem_trackers.if_contains(query_id,
+ [&tracker](std::shared_ptr<MemTracker> v) { tracker = v; });
return tracker;
}
+void QueryMemTrackerRegistry::DeregisterQueryMemTracker(const std::string& query_id) {
+ DCHECK(!query_id.empty());
+ _query_mem_trackers.erase_if(query_id, [](std::shared_ptr<MemTracker>) { return true; });
+ LOG(WARNING) << "DeregisterQueryMemTracker " << query_id << " len " << _query_mem_trackers.size();
Review comment:
Why there is a warning log?
##########
File path: be/src/runtime/mem_tracker.cpp
##########
@@ -105,12 +105,14 @@ std::shared_ptr<MemTracker> MemTracker::CreateTracker(RuntimeProfile* profile, i
level > real_parent->_level ? level : real_parent->_level));
real_parent->AddChildTracker(tracker);
tracker->Init();
+ tracker->set_query_id(query_id);
return tracker;
}
std::shared_ptr<MemTracker> MemTracker::CreateTracker(int64_t byte_limit, const std::string& label,
- std::shared_ptr<MemTracker> parent, bool log_usage_if_zero, bool reset_label_name, MemTrackerLevel level) {
+ std::shared_ptr<MemTracker> parent, bool log_usage_if_zero, bool reset_label_name,
+ MemTrackerLevel level, std::string query_id) {
Review comment:
```suggestion
MemTrackerLevel level, const std::string& query_id) {
```
##########
File path: be/src/runtime/exec_env.h
##########
@@ -174,7 +176,8 @@ class ExecEnv {
ClientCache<TPaloBrokerServiceClient>* _broker_client_cache = nullptr;
ClientCache<TExtDataSourceServiceClient>* _extdatasource_client_cache = nullptr;
std::shared_ptr<MemTracker> _mem_tracker;
Review comment:
Add comments to explain these trackers.
##########
File path: be/src/runtime/mem_tracker.cpp
##########
@@ -78,7 +78,7 @@ void MemTracker::CreateRootTracker() {
std::shared_ptr<MemTracker> MemTracker::CreateTracker(RuntimeProfile* profile, int64_t byte_limit,
const std::string& label, const std::shared_ptr<MemTracker>& parent,
- bool reset_label_name, MemTrackerLevel level) {
+ bool reset_label_name, MemTrackerLevel level, std::string query_id) {
Review comment:
```suggestion
bool reset_label_name, MemTrackerLevel level, const std::string& query_id) {
```
##########
File path: be/src/runtime/mem_tracker.cpp
##########
@@ -242,26 +243,38 @@ int64_t MemTracker::GetPoolMemReserved() {
return mem_reserved;
}
-std::shared_ptr<MemTracker> PoolMemTrackerRegistry::GetRequestPoolMemTracker(
- const string& pool_name, bool create_if_not_present) {
- DCHECK(!pool_name.empty());
- lock_guard<SpinLock> l(pool_to_mem_trackers_lock_);
- PoolTrackersMap::iterator it = pool_to_mem_trackers_.find(pool_name);
- if (it != pool_to_mem_trackers_.end()) {
- MemTracker* tracker = it->second.get();
- DCHECK(pool_name == tracker->pool_name_);
- return it->second;
+std::shared_ptr<MemTracker> QueryMemTrackerRegistry::RegisterQueryMemTracker(
+ const std::string& query_id, int64_t mem_limit) {
+ DCHECK(!query_id.empty());
+ if (mem_limit != -1) {
+ if (mem_limit > MemInfo::physical_mem()) {
Review comment:
Is this check necessary? I think this can be done when memtracker is created?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@doris.apache.org
For additional commands, e-mail: commits-help@doris.apache.org