[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile subclasses. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also now subtracted from local time in the averaged profile, which fixes IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Reviewed-on: http://gerrit.cloudera.org:8080/15798 Reviewed-by: Impala Public Jenkins Tested-by: Impala Public Jenkins --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,633 insertions(+), 687 deletions(-) Approvals: Impala Public Jenkins: Looks good to me, approved; Verified -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 25 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 24: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 24 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Sep 2020 22:02:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 24: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6456/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 24 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Sep 2020 16:43:24 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 24: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 24 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 21 Sep 2020 16:43:23 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 23: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6449/ -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 23 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 19 Sep 2020 04:45:40 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 22: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7207/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 22 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 18 Sep 2020 23:45:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 23: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6449/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 23 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 18 Sep 2020 23:25:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 22: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 22 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 18 Sep 2020 23:24:53 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 22: The bugfix was trivial -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 22 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 18 Sep 2020 23:25:08 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 23: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 23 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 18 Sep 2020 23:25:21 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Kurt Deschler, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#22). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile subclasses. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also now subtracted from local time in the averaged profile, which fixes IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,633 insertions(+), 687 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/22 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 22 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 21: Looks like the failure may be a real problem with the profiles - some of the RUNTIME_PROFILE tests failed on the dockerized cluster. Unclear why they would fail in that config but not in the other tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 21 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 17 Sep 2020 18:55:36 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 21: Verified-1 Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/6436/ -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 21 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 17 Sep 2020 08:52:42 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 21: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/6436/ DRY_RUN=false -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 21 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 17 Sep 2020 03:30:29 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 21: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 21 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 17 Sep 2020 03:30:28 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 20: Code-Review+2 This is looking good to me. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 20 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 16 Sep 2020 23:13:13 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 20: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7168/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 20 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 12 Sep 2020 06:12:30 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype
..
Patch Set 17:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15798/19/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/15798/19/be/src/util/runtime-profile-counters.h@475
PS19, Line 475: RuntimeProfile::Counter
> I think I would prefer it to inherit from RuntimeProfileBase::Counter, sinc
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1892
PS17, Line 1892: RuntimeProfile::SummaryStatsCounter::ToThrift
> I agree that we don't want to disrupt the rest of the codebase.
This is a good point. I fixed up the erroneous references to RuntimeProfile::
classes in the runtime-profile{.h,.cc,-counters.h} files
--
To view, visit http://gerrit.cloudera.org:8080/15798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a
Gerrit-Change-Number: 15798
Gerrit-PatchSet: 17
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Sat, 12 Sep 2020 05:52:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Kurt Deschler, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#20). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile subclasses. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also now subtracted from local time in the averaged profile, which fixes IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,633 insertions(+), 687 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/20 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 20 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 19: Code-Review+1 (2 comments) I'm basically good to approve this. Unless someone else wants to review, I will bump this to +2. http://gerrit.cloudera.org:8080/#/c/15798/19/be/src/util/runtime-profile-counters.h File be/src/util/runtime-profile-counters.h: http://gerrit.cloudera.org:8080/#/c/15798/19/be/src/util/runtime-profile-counters.h@475 PS19, Line 475: RuntimeProfile::Counter I think I would prefer it to inherit from RuntimeProfileBase::Counter, since SummaryStatsCounter is a class declared in RuntimeProfileBase. http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1892 PS17, Line 1892: counter->total_num_values[i] = counter->total > Yeah I was trying to minimize disruption to the rest of the codebase. We co I agree that we don't want to disrupt the rest of the codebase. I'm thinking more about a few mismatches of the declaration vs the definition in runtime-profile.h/runtime-profile.cc/runtime-profile-counters.h. For example, we declare RuntimeProfileBase::AveragedCounter, but we define the implementations of its functions with RuntimeProfile::AveragedCounter. The compiler can figure it out, and it is not a big deal, but I think it is slightly clearer for the definition to match the declaration. Specifically, if the class is declared in RuntimeProfileBase, then we use that for all its function definitions (even if it has an alias in RuntimeProfile). So, the SummaryStatsCounter definitions are RuntimeProfileBase::SummaryStatsCounter rather than RuntimeProfile::SummaryStatsCounter. I think this would only impact Counter, SummaryStatsCounter, and AveragedCounter, and it shouldn't impact anything outside the files we are already touching. None of this would impact the classes declared in RuntimeProfile. Does that make sense? -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 19 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 10 Sep 2020 23:54:39 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 18: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/7143/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 09 Sep 2020 19:25:39 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype
..
Patch Set 17:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile-test.cc@626
PS17, Line 626: RuntimeProfile::AveragedCounter bytes_avg(TUnit::BYTES,
NUM_COUNTERS);
: for (int i = 0; i < NUM_COUNTERS; ++i) {
: bytes_avg.UpdateCounter(counters[i], i);
: }
: RuntimeProfile::AveragedCounter::Stats stats =
bytes_avg.GetStats();
: EXPECT_EQ(NUM_COUNTERS, stats.num_vals);
: EXPECT_EQ(100, stats.min);
: EXPECT_EQ(199, stats.max);
: EXPECT_EQ(149, stats.mean);
: EXPECT_EQ(149, stats.p50);
: EXPECT_EQ(174, stats.p75);
: EXPECT_EQ(189, stats.p90);
: EXPECT_EQ(194, stats.p95);
> Something else we could test is to/from thrift. AveragedCounter -> TAggCoun
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@211
PS17, Line 211: thread
> Should be "thrift"?
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@247
PS17, Line 247: RuntimeProfile::CreateFromThriftHelper
> I think this should be "RuntimeProfileBase::CreateFromThriftHelper"
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@356
PS17, Line 356: auto& entry = summary_stats_map_[src_entry.first];
> Nit: One thing that I found confusing is that the source entry coming from
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@380
PS17, Line 380: Update()/Update()
> Redundant? (Or needs some other change?)
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@526
PS17, Line 526: Update()/Update()
> Redundant? (Or needs some other change?)
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@936
PS17, Line 936: ""
> Nit: Not your change, but it might be clearer to use ROOT_COUNTER here rath
Done here and elsewhere
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1080
PS17, Line 1080: // Print a sorted vector of indices in compressed form with
subsequent indices
: // printed as ranges. E.g. [1, 2, 3, 4, 6] would result in
"1-4,6".
: static void PrettyPrintIndexRanges(ostream* s, const
vector& indices) {
: if (indices.empty()) return;
: ostream& stream = *s;
: int32_t start_idx = indices[0];
: int32_t prev_idx = indices[0];
: for (int i = 0; i < indices.size(); ++i) {
: int32_t idx = indices[i];
: if (idx > prev_idx + 1) {
: // Start of a new range. Print the previous range of
values.
: stream << start_idx;
: if (start_idx < prev_idx) stream << "-" << prev_idx;
: stream << ",";
: start_idx = idx;
: }
: prev_idx = idx;
: }
: // Print the last range of values.
: stream << start_idx;
: if (start_idx < prev_idx) stream << "-" << prev_idx;
: }
> Nit: Should this be closer to the code that calls it (down in AggregatedRun
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1892
PS17, Line 1892: RuntimeProfile::SummaryStatsCounter::ToThrift
> Do we care about whether all these declarations use RuntimeProfile vs Runti
Yeah I was trying to minimize disruption to the rest of the codebase. We could
try to clean it up later?
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@2066
PS17, Line 2066: ""
> Nit: Might consider using ROOT_COUNTER rather than ""
Done
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@2128
PS17, Line 2128: void AggregatedRuntimeProfile::PrettyPrintInfoStrings(
: ostream* s, const string& prefix) const {
: ostream& stream = *s;
: if (FLAGS_gen_experimental_profile) {
: lock_guard l(input_profile_name_lock_);
: if (!input_profile_names_.empty()) {
: // TODO: IMPALA-9382: improve pretty-printing here
: stream << prefix
: << "Instances: " <<
boost::algorithm::join(input_profile_names_, ", ")
: << endl;
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Kurt Deschler, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#18). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile subclasses. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also now subtracted from local time in the averaged profile, which fixes IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,568 insertions(+), 623 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/18 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 18 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Joe McDonnell has posted comments on this change. (
http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype
..
Patch Set 17:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile-test.cc
File be/src/util/runtime-profile-test.cc:
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile-test.cc@626
PS17, Line 626: RuntimeProfile::AveragedCounter bytes_avg(TUnit::BYTES,
NUM_COUNTERS);
: for (int i = 0; i < NUM_COUNTERS; ++i) {
: bytes_avg.UpdateCounter(counters[i], i);
: }
: RuntimeProfile::AveragedCounter::Stats stats =
bytes_avg.GetStats();
: EXPECT_EQ(NUM_COUNTERS, stats.num_vals);
: EXPECT_EQ(100, stats.min);
: EXPECT_EQ(199, stats.max);
: EXPECT_EQ(149, stats.mean);
: EXPECT_EQ(149, stats.p50);
: EXPECT_EQ(174, stats.p75);
: EXPECT_EQ(189, stats.p90);
: EXPECT_EQ(194, stats.p95);
Something else we could test is to/from thrift. AveragedCounter -> TAggCounter
-> AveragedCounter
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@211
PS17, Line 211: thread
Should be "thrift"?
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@247
PS17, Line 247: RuntimeProfile::CreateFromThriftHelper
I think this should be "RuntimeProfileBase::CreateFromThriftHelper"
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@356
PS17, Line 356: auto& entry = summary_stats_map_[src_entry.first];
Nit: One thing that I found confusing is that the source entry coming from the
RuntimeProfile has a different type than the destination entry, even though
they are both from their corresponding summary_stats_map_ fields. It made sense
once I looked at it again, but it would be good to make it clearer. One option
is to tweak the variable names (don't use 'entry' for both or make the names
more specific to the structure e.g. src_entry -> src_stats_counter or entry ->
agg_entry). Another thought is that maybe this 'auto' shouldn't be an 'auto'.
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@380
PS17, Line 380: Update()/Update()
Redundant? (Or needs some other change?)
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@526
PS17, Line 526: Update()/Update()
Redundant? (Or needs some other change?)
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@936
PS17, Line 936: ""
Nit: Not your change, but it might be clearer to use ROOT_COUNTER here rather
than empty string.
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1080
PS17, Line 1080: // Print a sorted vector of indices in compressed form with
subsequent indices
: // printed as ranges. E.g. [1, 2, 3, 4, 6] would result in
"1-4,6".
: static void PrettyPrintIndexRanges(ostream* s, const
vector& indices) {
: if (indices.empty()) return;
: ostream& stream = *s;
: int32_t start_idx = indices[0];
: int32_t prev_idx = indices[0];
: for (int i = 0; i < indices.size(); ++i) {
: int32_t idx = indices[i];
: if (idx > prev_idx + 1) {
: // Start of a new range. Print the previous range of
values.
: stream << start_idx;
: if (start_idx < prev_idx) stream << "-" << prev_idx;
: stream << ",";
: start_idx = idx;
: }
: prev_idx = idx;
: }
: // Print the last range of values.
: stream << start_idx;
: if (start_idx < prev_idx) stream << "-" << prev_idx;
: }
Nit: Should this be closer to the code that calls it (down in
AggregatedRuntimeProfile below)?
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@1892
PS17, Line 1892: RuntimeProfile::SummaryStatsCounter::ToThrift
Do we care about whether all these declarations use RuntimeProfile vs
RuntimeProfileBase?
It makes sense to me that we expose these types via RuntimeProfile so the rest
of the code doesn't need to change. Technically, the underlying type is on
RuntimeProfileBase.
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@2066
PS17, Line 2066: ""
Nit: Might consider using ROOT_COUNTER rather than ""
http://gerrit.cloudera.org:8080/#/c/15798/17/be/src/util/runtime-profile.cc@2128
PS17, Line 2128: void
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype
..
Patch Set 16:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift
File common/thrift/RuntimeProfile.thrift:
http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift@249
PS16, Line 249: an averaged profile
: // for the fragment is also included with averaged counter
values.
> Does it increase the serialized size for V1 by a noticeable amount? That wo
It shouldn't change the serialized size cause of how the thrift encoding works
- it just doesn't include unset fields. From the generated code
if (this->__isset.aggregated) {
xfer += oprot->writeFieldBegin("aggregated",
::apache::thrift::protocol::T_STRUCT, 13);
xfer += this->aggregated.write(oprot);
xfer += oprot->writeFieldEnd();
}
--
To view, visit http://gerrit.cloudera.org:8080/15798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a
Gerrit-Change-Number: 15798
Gerrit-PatchSet: 16
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Kurt Deschler
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 01 Sep 2020 04:54:42 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 16: (1 comment) http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift@249 PS16, Line 249: an averaged profile : // for the fragment is also included with averaged counter values. > It does. It will make the deserialized objects larger because of the extra Does it increase the serialized size for V1 by a noticeable amount? That would be my main concern, since that corresponds to disk usage for the profile log. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 31 Aug 2020 19:52:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 17: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6973/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 17 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 24 Aug 2020 22:20:22 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 16: (1 comment) http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift@249 PS16, Line 249: an averaged profile : // for the fragment is also included with averaged counter values. > Does the thrift for V1's average profile now also contain the TAggregatedRu It does. It will make the deserialized objects larger because of the extra members, particularly the C++ objects. I think more of the Java objects use references instead of inline objects. In C++ TAggregatedRuntimeProfileNode is 144 bytes and TRuntimeProfileNode is 456 bytes so a 31% increase. Although that will get a bit higher with later patches. But I think it is small in comparison to the counters - each TCounter is 56 bytes and each string is 32 bytes, so it's probably going to use the same memory as 2-3 counter entries and is negligible in the scheme of things. The alternative could be to have a separate list that only contains the necessary entries. My feeling is that that would add a bit of complexity to all of the profile readers and is not worth the memory savings. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 24 Aug 2020 21:59:48 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Kurt Deschler, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#17). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile subclasses. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also now subtracted from local time in the averaged profile, which fixes IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,542 insertions(+), 619 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/17 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 17 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 16: (3 comments) I made it through most of the runtime-profile.h. The transformation of the averaged profile to the aggregated profile and resulting class hierarchy seems reasonable to me. http://gerrit.cloudera.org:8080/#/c/15798/16//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15798/16//COMMIT_MSG@30 PS16, Line 30: with RuntimeProfile and AggregatedRuntimeProfile : base classes "base classes" -> "subclasses"? http://gerrit.cloudera.org:8080/#/c/15798/16//COMMIT_MSG@44 PS16, Line 44: These stats can now be : automatically as aggregations of counters. Some words seem to be missing. http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/15798/16/common/thrift/RuntimeProfile.thrift@249 PS16, Line 249: an averaged profile : // for the fragment is also included with averaged counter values. Does the thrift for V1's average profile now also contain the TAggregatedRuntimeProfileNode struct? If so, are there any size concerns? -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Sat, 15 Aug 2020 00:44:59 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 16: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/6901/ : Initial code review checks failed. See linked job for details on the failure. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 13 Aug 2020 01:06:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Kurt Deschler, Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#16). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,543 insertions(+), 619 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/16 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 16 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 15: (4 comments) http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@62 PS15, Line 62: 3: required list has_value : 4: required list values > It would be good to have explicit comments about the cardinality here and h I added comments to the TAgg* structs to make it more explicit. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@113 PS15, Line 113: 3: required list has_value I'm realising that I'm a little inconsistent about naming these lists, and particularly about whether they're plural or not. I fixed the has_value/have_values inconsistency in favour of the former, cause it sounds less awkward to me. I didn't see anyhting else that would obviously improve clarity but lmk if you think I could clean any of this up. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@142 PS15, Line 142: The first map key is the info string : // key. The second map key is a distinct value of that key. > Just for clarity, maybe include an example key and example distinct value. This is a good point. I added a realistic example that motivates it. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@144 PS15, Line 144: This means that the common case, where all > Missing the rest of this sentence. Done -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 13 Aug 2020 00:54:44 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 15: (3 comments) Looked at the thrift changes, and it makes sense to me. I'm looking at the rest and I'll post comments once I make it through runtime-profile.h http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift File common/thrift/RuntimeProfile.thrift: http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@62 PS15, Line 62: 3: required list has_value : 4: required list values It would be good to have explicit comments about the cardinality here and how the indices line up with the instances that are included. This applies in several places, so the comment may not be right here. If you wanted to back out the value for a specific instance, what would you do? i.e. the i'th index here corresponds to the i'th index of input_profiles, etc. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@142 PS15, Line 142: The first map key is the info string : // key. The second map key is a distinct value of that key. Just for clarity, maybe include an example key and example distinct value. http://gerrit.cloudera.org:8080/#/c/15798/15/common/thrift/RuntimeProfile.thrift@144 PS15, Line 144: This means that the common case, where all Missing the rest of this sentence. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Kurt Deschler Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Aug 2020 22:45:21 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 15: This is ready for review. There are some big follow-on changes to finish this off, but I think this covers the main refactoring of RuntimeProfile, and also fleshes out enough of the thrift representation to give you the general idea of where it's going. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 15 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 12 Aug 2020 00:45:15 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 14: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6867/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 14 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 11 Aug 2020 06:21:59 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#14). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,519 insertions(+), 619 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/14 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 14 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 13: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6304/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Fri, 12 Jun 2020 16:32:33 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#13). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests * Ran some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/runtime/fragment-instance-state.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 13 files changed, 1,515 insertions(+), 617 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/13 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/15798/12/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15798/12/be/src/util/runtime-profile.cc@1842 PS12, Line 1842: (*s) << "mean=" << PrettyPrinter::Print(BitcastToInt64(stats.mean), unit_, true) I think including the sum would actually be useful in a lot of cases. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 12 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 11 Jun 2020 18:51:38 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/15798/12/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15798/12/be/src/util/runtime-profile.cc@1844 PS12, Line 1844:<< " p50=" << PrettyPrinter::Print(BitcastToInt64(stats.p50), unit_, true) We can omit the percentiles to reduce noise if min = max (or if there are <= 2 values, which is likely pretty rare). -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 12 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Thu, 11 Jun 2020 18:50:10 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 12: (1 comment) http://gerrit.cloudera.org:8080/#/c/15798/12/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15798/12/be/src/util/runtime-profile.cc@1810 PS12, Line 1810: 0 There is a subtle bug here, where it uses an int as the intermediate value because of the int literal. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 12 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Wed, 10 Jun 2020 23:26:22 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 11: (1 comment) http://gerrit.cloudera.org:8080/#/c/15798/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15798/11//COMMIT_MSG@49 PS11, Line 49: Also make TotalTime and InactiveTime behave like other counters - This is IMPALA-6147 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 19 May 2020 20:14:15 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 11: The latest patches reworked AveragedCounter, which wasn't strictly thread-safe previously in a probably-benign way, to be unambiguously thread-safe. It's simplified by only storing the values and computing the mean on-demand. I also fixed some challenges with maintaining min/max/avg, etc and added percentile stats. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 19 May 2020 02:35:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype
..
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15798/6/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/15798/6/be/src/util/runtime-profile-counters.h@416
PS6, Line 416: int64_t min_value() const { return min_value_; }
> I think maintaining the min and max value on each update is a mistake - it
Done
http://gerrit.cloudera.org:8080/#/c/15798/7/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/7/be/src/util/runtime-profile.cc@1795
PS7, Line 1795: }
Long line.
--
To view, visit http://gerrit.cloudera.org:8080/15798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a
Gerrit-Change-Number: 15798
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Tue, 19 May 2020 02:34:09 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 11: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6094/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Tue, 19 May 2020 01:45:54 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#11). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. * exhaustive tests * ASAN tests TODO * Run some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 12 files changed, 1,517 insertions(+), 619 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/11 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 10: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6087/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 18 May 2020 07:47:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#10). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. TODO * exhaustive tests * ASAN * Run some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 12 files changed, 1,517 insertions(+), 619 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/10 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Hello Joe McDonnell, Impala Public Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/15798 to look at the new patch set (#9). Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter in the profile? Possible extensions: * We could better highlight outliers when pretty-printing the profile. Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. * Added unit test for stats computation in AveragedCounter. * Passed core tests. TODO * exhaustive tests * ASAN * Run some tests locally with TSAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 12 files changed, 1,501 insertions(+), 617 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/9 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 9 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Joe McDonnell Gerrit-Reviewer: Tim Armstrong
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has posted comments on this change. (
http://gerrit.cloudera.org:8080/15798 )
Change subject: IMPALA-9382: part 1: transposed profile prototype
..
Patch Set 6:
(15 comments)
http://gerrit.cloudera.org:8080/#/c/15798/6/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/15798/6/be/src/util/runtime-profile-counters.h@416
PS6, Line 416: int64_t min_value() const { return min_value_; }
I think maintaining the min and max value on each update is a mistake - it is
not correct if the counter is updated more than once, and it adds complexity to
the code. I think we should instead compute these stats on demand when
pretty-printing the profile.
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile-counters.h
File be/src/util/runtime-profile-counters.h:
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile-counters.h@392
PS4, Line 392: /// TODO: IMPALA-9382: rename counter. CounterVector?
AggregatedCounter?
> Update comment
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile-counters.h@499
PS4, Line 499:
> This
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile.h@370
PS4, Line 370:
> Update this
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile.h@744
PS4, Line 744: int idx);
> pluralise
Done
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.h
File be/src/util/runtime-profile.h:
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.h@193
PS2, Line 193: /// Return the number of input instances that contributed to
this profile.
> Do this
Done
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.h@349
PS2, Line 349: };
> typo
Done
http://gerrit.cloudera.org:8080/#/c/15798/1/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/1/be/src/util/runtime-profile.cc@2203
PS1, Line 2203: return distinct_vals;
> Move aggregated logic down here?
Done
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.cc@222
PS2, Line 222: bool indent = nodes[*idx].indent;
> bad comment
Done
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.cc@570
PS2, Line 570: double local_time_frac =
> Why was it the old way?
Done
http://gerrit.cloudera.org:8080/#/c/15798/2/be/src/util/runtime-profile.cc@1056
PS2, Line 1056: profile->PrettyPrint(s, prefix + (indent ? " " : ""));
> ADd todo
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile.cc@541
PS4, Line 541: for (int i = 0; i < children_.size(); ++i) {
> remove logs
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/be/src/util/runtime-profile.cc@1871
PS4, Line 1871: tcounter->min_value[i] = counter->min_;
> thisnn
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/common/thrift/RuntimeProfile.thrift
File common/thrift/RuntimeProfile.thrift:
http://gerrit.cloudera.org:8080/#/c/15798/4/common/thrift/RuntimeProfile.thrift@183
PS4, Line 183: // These fields are not used in aggregated profile nodes, i.e.
profile V2,
> This isn't really accurate
Done
http://gerrit.cloudera.org:8080/#/c/15798/4/common/thrift/RuntimeProfile.thrift@226
PS4, Line 226: // for the fragment is also included with averaged counter
values.
> Mention this is experimental
Done
--
To view, visit http://gerrit.cloudera.org:8080/15798
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a
Gerrit-Change-Number: 15798
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell
Gerrit-Reviewer: Tim Armstrong
Gerrit-Comment-Date: Sat, 16 May 2020 01:08:53 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. Patch Set 6: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/6029/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Comment-Date: Mon, 11 May 2020 21:11:02 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9382: part 1: transposed profile prototype
Tim Armstrong has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/15798 ) Change subject: IMPALA-9382: part 1: transposed profile prototype .. IMPALA-9382: part 1: transposed profile prototype This adds an experimental profile representation that is denser than the traditional representation. Counters, info strings and other information for all instances of a fragment are merged into a single tree. Descriptive stats (min, max, mean) are shown for each counter, along with the values for each instance. It can be enabled by setting --gen_experimental_profile=true. The default behaviour is unchanged, aside from including a few extra counters in existing profiles. An example of the pretty-printed profile is attached to the JIRA. The thrift representation of the profile is extended so that all instances of a fragment can be merged together into a single "aggregated" fragment, with vectors of counters. The in-memory representation is transformed in a similar way. The RuntimeProfile class is restructured so that there is a common RuntimeProfileBase class, with RuntimeProfile and AggregatedRuntimeProfile base classes. Execution fills in counters in RuntimeProfile for each instances, then these are aggregated together into an AggregatedRuntimeProfile on the coordinator. This replaces the "averaged" profile concept with an abstraction that more clearly distinguishes what operations apply to aggregated and unaggregated profiles. In a future change, we could use AggregatedRuntimeProfile for status reports so that less data needs to be sent to the coordinator, and the coordinator needs to do less processing. The new profile removes the bad practice of including aggregated stats as strings from the new profile. These stats can now be automatically as aggregations of counters. The legacy uses of InfoString are preserved so as to not lose information but can be removed when we switch to the transposed profile. Also make TotalTime and InactiveTime behave like other counters - they are pretty-printed the same as other counters. Inactive time is also included in the local time calculation, fixing IMPALA-2794. TODO in later patches for IMPALA-9382: These will need to be fixed before this can be considered production ready. * The JSON profile generation is not fully implemented for aggregated profiles. * Not all counter times are included in aggregated profile, e.g. time series counters. * The pretty-printing of the various profile counters will need to be improved to be more readable, e.g. grouping by host, improving formatting. * The aggregated profile is only updated at the end of the query. We need to support live updating. * Consider how to show local time per instance - make it a first-class counter? Possible extensions: * We could included more general summary stats for each counter, e.g. median, p95, etc. * We could better highlight outliers Testing: * I diffed the text profile of TPC-DS Q1 to make sure there were no unexpected changes. TODO * exhaustive tests * TSAN/ASAN Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a --- M be/src/runtime/bufferpool/buffer-pool-test.cc M be/src/runtime/coordinator-backend-state.cc M be/src/runtime/coordinator-backend-state.h M be/src/runtime/coordinator.cc M be/src/util/dummy-runtime-profile.h M be/src/util/pretty-printer-test.cc M be/src/util/pretty-printer.h M be/src/util/runtime-profile-counters.h M be/src/util/runtime-profile-test.cc M be/src/util/runtime-profile.cc M be/src/util/runtime-profile.h M common/thrift/RuntimeProfile.thrift 12 files changed, 1,383 insertions(+), 607 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/98/15798/6 -- To view, visit http://gerrit.cloudera.org:8080/15798 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I0838c6a0872f57c696267ff4e92d29c08748eb7a Gerrit-Change-Number: 15798 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong Gerrit-Reviewer: Tim Armstrong
