KVStore: Expand the usage of thread-wise alloc/dealloc trace #9003
Conversation
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
Signed-off-by: CalvinNeo <calvinneo1995@gmail.com>
ti-chi-bot
bot
added
release-note-none
|
/pull-integration-test |
|
/pull-unit-test |
|
/pull-unit-test |
|
|
|
/retest |
| std::unordered_map<std::string, uint64_t> agg_deallocate; | ||
| for (const auto & [k, v] : kvstore_map) | ||
| { | ||
| auto agg_thread_name = getThreadNameAggPrefix(std::string_view(k.data(), k.size())); |
There was a problem hiding this comment.
| auto agg_thread_name = getThreadNameAggPrefix(std::string_view(k.data(), k.size())); | |
| auto agg_thread_name = getThreadNameAggPrefix(k); |
| bool is_terminated{false}; | ||
| mutable std::mutex monitoring_mut; | ||
| std::condition_variable monitoring_cv; | ||
| std::thread * monitoring_thread{nullptr}; |
There was a problem hiding this comment.
no specific reason, just follows the original code...
… into monitor-memory-of-tiflash
| LOG_INFO(DB::Logger::get(), "Stop collecting thread alloc metrics"); | ||
| { | ||
| std::unique_lock lk(monitoring_mut); | ||
| if (monitoring_thread == nullptr) |
There was a problem hiding this comment.
Under what circumstances will this pointer be empty?
There was a problem hiding this comment.
when stopThreadAllocInfo may be called more than one time
| tiflash_metrics.setProxyThreadMemory("dealloc_" + k, data.dealloc); | ||
| } | ||
|
|
||
| void JointThreadInfoJeallocMap::stopThreadAllocInfo() |
There was a problem hiding this comment.
We should call it in ContextShared::shutdown explicitly to make sure the monitoring_thread is stopped before TiFlashMetrics::instance is released
There was a problem hiding this comment.
Then I shall move joint_memory_allocation_map from Context to ContextShared?
There was a problem hiding this comment.
Seems TiFlashMetrics::instance() is a singleton, I think maybe it actually overlives Context
TiFlashMetrics & TiFlashMetrics::instance()
{
static TiFlashMetrics inst; // Instantiated on first use.
return inst;
}
There was a problem hiding this comment.
Then I shall move joint_memory_allocation_map from Context to ContextShared?
Yes, Context is a session level instance, the long-live instances should be in ContextShared
ti-chi-bot
bot
added
size/XXL
… into monitor-memory-of-tiflash
|
/retest |
/test pull-unit-test |
|
@CalvinNeo: The specified target(s) for
Use In response to this:
Instructions 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. |
|
/retest |
| @@ -32,7 +32,6 @@ | |||
| #include <IO/BaseFile/fwd.h> | |||
| #include <IO/Buffer/ReadBufferFromFile.h> | |||
| #include <IO/FileProvider/FileProvider.h> | |||
| #include <Interpreters/Context.h> | |||
There was a problem hiding this comment.
why this line is removed?
There was a problem hiding this comment.
I don't know, I will add that back. however to my surprise, it compiles
Co-authored-by: JaySon <tshent@qq.com>
Co-authored-by: JaySon <tshent@qq.com>
|
/retest |
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, Lloyd-Pottiger The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
What problem does this PR solve?
Issue Number: close #8835
Problem Summary:
We put this utility in KVStore/FFI, because it is bound to FFI interface which is between C++ and Rust. That is also why we call is a joint.
What is changed and how it works?
Check List
Tests
Side effects
Documentation
Release note