Refine the lifecycle of memory_tracker for MPP query #6020
Conversation
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
added
release-note-none
| ++read_packet_index; | ||
| packets[read_packet_index - 1]->recomputeTrackedMem(); |
There was a problem hiding this comment.
recomputeTrackedMem may throw error, we need handle the exception properly.
There was a problem hiding this comment.
Yes, I move the try/catch part into recomputeTrackedMem
windtalker
force-pushed
the
memory_tracker
branch
from
e403b08
to
72c6b7a
Compare
| /// because after `write`, `async_tunnel_sender` can be destroyed at any time | ||
| /// so there is a risk that `res` is destructed after `aysnc_tunnel_sender` | ||
| /// is destructed which may cause the memory tracker in `res` become invalid | ||
| res->switchMemTracker(nullptr); |
There was a problem hiding this comment.
It may cause MemTracker incorrect... How about switching to RootMemTracker?
There was a problem hiding this comment.
Why? It just release the memory. Before this change, the memory is also released after write()
There was a problem hiding this comment.
I'm worried about an extreme case that a lot of threads are doing EstablishCallData::trySendOneMsg() and early free() of the MemTracker cause real memory usage too large than tracked
There was a problem hiding this comment.
But in before this pr, it also has the early free issue since async tunnel's write actually does not wait until write finish.
dbms/src/Flash/Mpp/MPPTask.cpp
Outdated
| @@ -336,6 +348,7 @@ void MPPTask::preprocess() | |||
| void MPPTask::runImpl() | |||
| { | |||
| CPUAffinityManager::getInstance().bindSelfQueryThread(); | |||
| current_memory_tracker = process_list_entry->get().getMemoryTrackerPtr().get(); | |||
There was a problem hiding this comment.
is it necessary? since newThreadManager()->scheduleThenDetach will propgate memTracker into runImpl
There was a problem hiding this comment.
Oh, yes, it is not necessary.
There was a problem hiding this comment.
Maybe change it to RUNTIME_ASSERT
| { | ||
| // note: no exception should be thrown rudely, since it's called by a GRPC poller. | ||
| for (size_t i = 0; i < read_packet_index; ++i) | ||
| { | ||
| auto & packet = packets[i]; | ||
| // We shouldn't throw error directly, since the caller works in a standalone thread. | ||
| try |
There was a problem hiding this comment.
I seems pushPacket can throw exception except recomputeTrackedMem such as fiu_do_on(FailPoints::random_receiver_async_msg_push_failure_failpoint, push_succeed = false;);
There was a problem hiding this comment.
This failpoint just set the return value(push_succeed) to false, it does not throw exception.
dbms/src/Flash/Mpp/MPPTask.cpp
Outdated
| #include <Flash/Coprocessor/DAGQuerySource.h> | ||
| #include <Flash/Coprocessor/DAGUtils.h> | ||
| #include <Flash/Mpp/ExchangeReceiver.h> | ||
| #include <Flash/Mpp/GRPCReceiverContext.h> | ||
| #include <Flash/Mpp/MPPTask.h> | ||
| #include <Flash/Mpp/MPPTunnelSet.h> | ||
| #include <Flash/Mpp/Utils.h> | ||
| #include <Flash/Planner/PlanQuerySource.h> |
ti-chi-bot
added
status/LGT2
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
windtalker
force-pushed
the
memory_tracker
branch
from
d45ad7c
to
90ad917
Compare
|
/merge |
|
@windtalker: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. 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 ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: 90ad917
|
ti-chi-bot
added
the
status/can-merge
Signed-off-by: xufei <xufeixw@mail.ustc.edu.cn>
ti-chi-bot
removed
the
status/can-merge
|
/merge |
|
@windtalker: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. 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 ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: 6f05860
|
ti-chi-bot
added
the
status/can-merge
ti-chi-bot
added
size/XL
|
/merge |
|
@windtalker: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. 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 ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: bf77528
|
ti-chi-bot
added
the
status/can-merge
ti-chi-bot
removed
the
status/can-merge
|
/merge |
|
@windtalker: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. 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 ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. Commit hash: 045c6dc
|
ti-chi-bot
added
the
status/can-merge
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Signed-off-by: xufei xufeixw@mail.ustc.edu.cn
What problem does this PR solve?
Issue Number: ref #5609
Problem Summary:
This is a refine pr for #5610
What is changed and how it works?
processlist/memory_trackerinMPPTask::prepare, so when creatingMPPTunnel/ExchangeReceiver, there is already a memory_tracker to use.TunnelSender, so eachTrackedMPPPacketdoes not need to hold the shared ptr of memory trackerCheck List
Tests
Side effects
Documentation
Release note