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<int64_t> stats = bytes_avg.GetStats<int64_t>(); : 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<int32_t>& 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<SpinLock> 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; : } : } : : { : lock_guard<SpinLock> l(agg_info_strings_lock_); : for (const auto& entry : agg_info_strings_) { : map<string, vector<int32_t>> distinct_vals = GroupDistinctInfoStrings(entry.second); : for (const auto& distinct_entry : distinct_vals) { : stream << prefix << " " << entry.first; : stream << "["; : PrettyPrintIndexRanges(s, distinct_entry.second); : stream << "]: " << distinct_entry.first << endl; : } : } : } : } > Nit: From what I understand, this doesn't print anything when it isn't the 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: 17 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 09 Sep 2020 19:03:47 +0000 Gerrit-HasComments: Yes
