Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )
Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift() ...................................................................... Patch Set 4: (2 comments) Thanks for the changes! I looked through the code one more time and I am confident enough for the +2. http://gerrit.cloudera.org:8080/#/c/15251/4/be/src/util/runtime-profile.h File be/src/util/runtime-profile.h: http://gerrit.cloudera.org:8080/#/c/15251/4/be/src/util/runtime-profile.h@603 PS4, Line 603: /// Collect this node and descendants into 'nodes'. The order is a pre-order traversal nit: missing . http://gerrit.cloudera.org:8080/#/c/15251/4/be/src/util/runtime-profile.cc File be/src/util/runtime-profile.cc: http://gerrit.cloudera.org:8080/#/c/15251/4/be/src/util/runtime-profile.cc@1263 PS4, Line 1263: val.second->GetEvents(&events); I guess that there are only a few events so this probably wouldn't matter much, but GetEvents() could be improved in 2 ways: 1. It always does a sort during GetEvents() - I think that we could be more optimistic and do this only if events turn out to be out of order 2. A function could be added that takes two vector pointers as parameters and push_backs to seq.labels/timestamps directly -- To view, visit http://gerrit.cloudera.org:8080/15251 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065 Gerrit-Change-Number: 15251 Gerrit-PatchSet: 4 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Thu, 20 Feb 2020 20:01:03 +0000 Gerrit-HasComments: Yes
