New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[opt](profile) Avoid unnecessary copies in the profile thrift. #34720
Conversation
Thank you for your contribution to Apache Doris. Since 2024-03-18, the Document has been moved to doris-website. |
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TPC-H: Total hot run time: 41331 ms
|
TPC-DS: Total hot run time: 188467 ms
|
TeamCity be ut coverage result: |
@@ -126,15 +134,16 @@ TReportExecStatusParams RuntimeQueryStatiticsMgr::create_report_exec_status_para | |||
} | |||
|
|||
TDetailedReportParams tmp; | |||
tmp.__set_profile(*pipeline_profile); | |||
tmp.__isset.profile = true; | |||
tmp.profile = std::move(*pipeline_profile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this move safe? the original object will be broken
@@ -151,15 +160,17 @@ TReportExecStatusParams RuntimeQueryStatiticsMgr::create_report_exec_status_para | |||
continue; | |||
} | |||
|
|||
load_channel_profiles_req.push_back(*load_channel_profile); | |||
load_channel_profiles_req.push_back(std::move(*load_channel_profile)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
original object will be broken
be/src/service/backend_service.cpp
Outdated
RuntimeProfile::Counter get_realtime_timer {TUnit::TIME_NS}; | ||
|
||
Defer _print_log([&]() { | ||
LOG_INFO("Getting realtime exec status cost {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG_WARNING
if we meet some threshold
run buildall |
TPC-H: Total hot run time: 42000 ms
|
TPC-DS: Total hot run time: 187641 ms
|
TeamCity be ut coverage result: |
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TeamCity be ut coverage result: |
clang-tidy review says "All clean, LGTM! 👍" |
run buildall |
TPC-H: Total hot run time: 41545 ms
|
TPC-DS: Total hot run time: 187370 ms
|
TeamCity be ut coverage result: |
run cloud_p1 |
} | ||
|
||
TReportExecStatusParams req; | ||
req.__set_query_profile(profile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check the thrift api have a better way?
likereq.__set_query_profile(std::move(profile)
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TeamCity be ut coverage result: |
run buildall |
TeamCity be ut coverage result: |
// This function will clear the data of fragment_id_to_profile and load_channel_profiles. | ||
RuntimeProfile::Counter timer {TUnit::TIME_NS}; | ||
Defer _print_log([&]() { | ||
LOG_INFO("Create report exec status params cost {}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we need log each profile report time?
be/src/util/thrift_client.h
Outdated
@@ -38,6 +38,11 @@ class TTransport; | |||
} // namespace apache | |||
|
|||
namespace doris { | |||
|
|||
#define SET_THRIFT_VALUES(thrift, member, value) \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not call MOVE_VALUE
run buildall |
clang-tidy review says "All clean, LGTM! 👍" |
TeamCity be ut coverage result: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
PR approved by at least one committer and no changes requested. |
PR approved by anyone and no changes requested. |
Proposed changes
need this pr
#34693
before
now
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...