You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2020/02/20 01:33:42 UTC

[Impala-ASF-CR] iMPALA-9399: optimise RuntimeProfile::ToThrift() 

Tim Armstrong has uploaded this change for review. ( http://gerrit.cloudera.org:8080/15251


Change subject: iMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................

iMPALA-9399: optimise RuntimeProfile::ToThrift()

The big win here is to preallocate the vector<TRuntimeProfileNode>
to the right size, to avoid copying the TRuntimeProfileNode objects
when reallocating the vector. The JIRA description on IMPALA-9399
explains why this happens.

Also various simplification/modernisation of code to avoid
unnecessary copies of vectors and redundant lock acquisitions.

Perf:
Added a microbenchmark that shows a 2.4x speedup of
RuntimeProfile::ToThrift() on a synthetic profile.

Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/runtime-profile-benchmark.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
4 files changed, 196 insertions(+), 71 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/15251/1
-- 
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: newchange
Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Gerrit-Change-Number: 15251
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................

IMPALA-9399: optimise RuntimeProfile::ToThrift()

The big win here is to preallocate the vector<TRuntimeProfileNode>
to the right size, to avoid copying the TRuntimeProfileNode objects
when reallocating the vector. The JIRA description on IMPALA-9399
explains why this happens.

Also various simplification/modernisation of code to avoid
unnecessary copies of vectors and redundant lock acquisitions.

Perf:
Added a microbenchmark that shows a 2.75x speedup of
RuntimeProfile::ToThrift() on a synthetic profile.

Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Reviewed-on: http://gerrit.cloudera.org:8080/15251
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/runtime-profile-benchmark.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
4 files changed, 192 insertions(+), 64 deletions(-)

Approvals:
  Impala Public Jenkins: Looks good to me, approved; Verified

-- 
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: merged
Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Gerrit-Change-Number: 15251
Gerrit-PatchSet: 8
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 2: Code-Review+1

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc@1204
PS2, Line 1204: e*
nit: It would be nice to use a reference to make the change less noisy. Maybe give the argument another name and create a "TRuntimeProfileNode& node" local variable if we want to keep the "arguments are not references" policy.


http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc@1218
PS2, Line 1218: str
Using a string* here could avoid the extra copies.


http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc@1218
PS2, Line 1218: Counter
Is it possible to make this const? It would make it clearer why it is ok the process this outside of counter_map_lock_



-- 
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: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 16:58:45 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc
File be/src/util/runtime-profile.cc:

http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc@1204
PS2, Line 1204: e*
> nit: It would be nice to use a reference to make the change less noisy. May
Done


http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc@1218
PS2, Line 1218: Counter
> Is it possible to make this const? It would make it clearer why it is ok th
Counters are thread-safe. I can make this const to be clearer.


http://gerrit.cloudera.org:8080/#/c/15251/2/be/src/util/runtime-profile.cc@1218
PS2, Line 1218: str
> Using a string* here could avoid the extra copies.
Good point, that should work because std::map entries are stable. I used a const string& instead.



-- 
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: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 18:05:30 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 4: Code-Review+2


-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 20:01:13 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15251

to look at the new patch set (#3).

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................

IMPALA-9399: optimise RuntimeProfile::ToThrift()

The big win here is to preallocate the vector<TRuntimeProfileNode>
to the right size, to avoid copying the TRuntimeProfileNode objects
when reallocating the vector. The JIRA description on IMPALA-9399
explains why this happens.

Also various simplification/modernisation of code to avoid
unnecessary copies of vectors and redundant lock acquisitions.

Perf:
Added a microbenchmark that shows a 2.4x speedup of
RuntimeProfile::ToThrift() on a synthetic profile.

Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/runtime-profile-benchmark.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
4 files changed, 192 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/15251/3
-- 
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: newpatchset
Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Gerrit-Change-Number: 15251
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 7: Code-Review+2


-- 
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: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 23:19:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5291/ : 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/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 <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 18:51:37 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5290/ : 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/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: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 18:51:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 6: Code-Review+2

(1 comment)

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: r (const SummaryStatsCounterMap
> I guess that there are only a few events so this probably wouldn't matter m
I think the typical length is 5-10 events, so optimising for long lists is not worthwhile.

I think gcc's std::sort() is O(n) in practice for short sorted lists - it does an intro-sort that does insertion sort for < 16 elements.

I did notice that I could avoid a string copy by moving the label into the output list.



-- 
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: 6
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 22:21:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 3: Code-Review+1

Looks like your suggested change made it even faster


-- 
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: 3
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 18:06:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15251

to look at the new patch set (#2).

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................

IMPALA-9399: optimise RuntimeProfile::ToThrift()

The big win here is to preallocate the vector<TRuntimeProfileNode>
to the right size, to avoid copying the TRuntimeProfileNode objects
when reallocating the vector. The JIRA description on IMPALA-9399
explains why this happens.

Also various simplification/modernisation of code to avoid
unnecessary copies of vectors and redundant lock acquisitions.

Perf:
Added a microbenchmark that shows a 2.4x speedup of
RuntimeProfile::ToThrift() on a synthetic profile.

Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/runtime-profile-benchmark.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
4 files changed, 196 insertions(+), 71 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/15251/2
-- 
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: newpatchset
Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Gerrit-Change-Number: 15251
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 1:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5281/ : 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/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: 1
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 01:50:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 7:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5382/ DRY_RUN=false


-- 
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: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 23:19:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/5293/ : 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/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: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 23:08:15 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 4: Code-Review+1


-- 
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 <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 18:42:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/15251 )

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................


Patch Set 7: Verified+1


-- 
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: 7
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 21 Feb 2020 04:09:18 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15251

to look at the new patch set (#5).

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................

IMPALA-9399: optimise RuntimeProfile::ToThrift()

The big win here is to preallocate the vector<TRuntimeProfileNode>
to the right size, to avoid copying the TRuntimeProfileNode objects
when reallocating the vector. The JIRA description on IMPALA-9399
explains why this happens.

Also various simplification/modernisation of code to avoid
unnecessary copies of vectors and redundant lock acquisitions.

Perf:
Added a microbenchmark that shows a 2.75x speedup of
RuntimeProfile::ToThrift() on a synthetic profile.

Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/runtime-profile-benchmark.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
4 files changed, 192 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/15251/5
-- 
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: newpatchset
Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Gerrit-Change-Number: 15251
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Csaba Ringhofer, Impala Public Jenkins, 

I'd like you to reexamine a change. Please visit

    http://gerrit.cloudera.org:8080/15251

to look at the new patch set (#4).

Change subject: IMPALA-9399: optimise RuntimeProfile::ToThrift()
......................................................................

IMPALA-9399: optimise RuntimeProfile::ToThrift()

The big win here is to preallocate the vector<TRuntimeProfileNode>
to the right size, to avoid copying the TRuntimeProfileNode objects
when reallocating the vector. The JIRA description on IMPALA-9399
explains why this happens.

Also various simplification/modernisation of code to avoid
unnecessary copies of vectors and redundant lock acquisitions.

Perf:
Added a microbenchmark that shows a 2.75x speedup of
RuntimeProfile::ToThrift() on a synthetic profile.

Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/runtime-profile-benchmark.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
4 files changed, 192 insertions(+), 64 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/51/15251/4
-- 
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: newpatchset
Gerrit-Change-Id: I1f85ef005ae3b793515b223d4c04bbd898336065
Gerrit-Change-Number: 15251
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-9399: optimise RuntimeProfile::ToThrift()

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
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 <ta...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 20 Feb 2020 20:01:03 +0000
Gerrit-HasComments: Yes