Kurt Deschler has posted comments on this change. ( http://gerrit.cloudera.org:8080/21683 )
Change subject: IMPALA-13304: Include aggregate instance-level metrics within experimental profile(V2) ...................................................................... Patch Set 4: (4 comments) http://gerrit.cloudera.org:8080/#/c/21683/4/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/21683/4/be/src/util/runtime-profile.cc@2952 PS4, Line 2952: lock_guard<SpinLock> l(event_sequence_lock_); Given the number of potential memory allocations below, it is going to be better to pre-allocate a vector and copy the events into it for processing so that the spinlock is not held very long. http://gerrit.cloudera.org:8080/#/c/21683/4/be/src/util/runtime-profile.cc@2953 PS4, Line 2953: if (!event_sequence_map_.empty()) { Is the empty case common enough to warrant a branch? http://gerrit.cloudera.org:8080/#/c/21683/4/be/src/util/runtime-profile.cc@2960 PS4, Line 2960: vector<Value> labels(event_sequence_contents.labels.size()); Can these structures get reused? http://gerrit.cloudera.org:8080/#/c/21683/4/be/src/util/runtime-profile.cc@2982 PS4, Line 2982: vector<int64_t> min_ts_list(BUCKET_SIZE, max); Also try to re-use these vectors. -- To view, visit http://gerrit.cloudera.org:8080/21683 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I49e18a7a7e1288e3e674e15b6fc86aad60a08214 Gerrit-Change-Number: 21683 Gerrit-PatchSet: 4 Gerrit-Owner: Surya Hebbar <sheb...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Kurt Deschler <kdesc...@cloudera.com> Gerrit-Reviewer: Riza Suminto <riza.sumi...@cloudera.com> Gerrit-Reviewer: Surya Hebbar <sheb...@cloudera.com> Gerrit-Comment-Date: Mon, 23 Sep 2024 12:43:31 +0000 Gerrit-HasComments: Yes
