You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/03/19 22:20:18 UTC
[kudu-CR] client: micro-optimize getters by outlining cold path
Hello Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15503
to review the following change.
Change subject: client: micro-optimize getters by outlining cold path
......................................................................
client: micro-optimize getters by outlining cold path
This improved TSBS perf about 10% for one of the workloads.
Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
---
M src/kudu/client/scan_batch.cc
1 file changed, 8 insertions(+), 5 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/15503/1
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
[kudu-CR] client: micro-optimize result batch cell getters by outlining cold path
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize result batch cell getters by outlining cold path
......................................................................
Patch Set 2: Verified+1
failed tests seem unrelated
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 20 Mar 2020 19:01:52 +0000
Gerrit-HasComments: No
[kudu-CR] client: micro-optimize getters by outlining cold path
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize getters by outlining cold path
......................................................................
Patch Set 1: Code-Review+1
TIL: Outlining can improve performance!
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 01:53:41 +0000
Gerrit-HasComments: No
[kudu-CR] client: micro-optimize getters by outlining cold path
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize getters by outlining cold path
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15503/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15503/1//COMMIT_MSG@7
PS1, Line 7: client: micro-optimize getters by outlining cold path
I can't really wrap my head around why the outlining improves the perf. Can you explain the intuition, if there is any?
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 04:00:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: micro-optimize result batch cell getters by outlining cold path
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Andrew Wong, Kudu Jenkins, Andrew Wong, Adar Dembo, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15503
to look at the new patch set (#2).
Change subject: client: micro-optimize result batch cell getters by outlining cold path
......................................................................
client: micro-optimize result batch cell getters by outlining cold path
This improves performance in a couple ways:
- by outlining the Substitute call and Status construction, the
templated Get<TypeTraits> function becomes much smaller. This makes it
get inlined into the GetInt8(), GetInt32(), etc outer functions. So,
every call into those functions is just a single call instead of two
calls in short succession. This makes branch prediction easier.
- The addition of PREDICT_FALSE means the common path is now the
"straight line" path which means that a tight loop calling these
functions may be able to entirely fit within the micro-op cache, etc.
Details here are a bit black magic but it's generally considered good
hygiene for short loops to minimize the code footprint.
- The outlining of the call also means the stack frame size is a bit
smaller. The new code path has three "push" and "pop" instructions in
the function intro/outro whereas the old one had 6.
This improved TSBS perf about 10% for one of the workloads.
Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
---
M src/kudu/client/scan_batch.cc
1 file changed, 11 insertions(+), 6 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/03/15503/2
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] client: micro-optimize getters by outlining cold path
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize getters by outlining cold path
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15503/1/src/kudu/client/scan_batch.cc
File src/kudu/client/scan_batch.cc:
http://gerrit.cloudera.org:8080/#/c/15503/1/src/kudu/client/scan_batch.cc@243
PS1, Line 243: BadTypeStatus
nit: maybe add a comment that we're explicitly out-lining here? This seems trivial enough that I can imagine a future dev unknowingly reverting this change.
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 00:23:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: micro-optimize result batch cell getters by outlining cold path
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize result batch cell getters by outlining cold path
......................................................................
client: micro-optimize result batch cell getters by outlining cold path
This improves performance in a couple ways:
- by outlining the Substitute call and Status construction, the
templated Get<TypeTraits> function becomes much smaller. This makes it
get inlined into the GetInt8(), GetInt32(), etc outer functions. So,
every call into those functions is just a single call instead of two
calls in short succession. This makes branch prediction easier.
- The addition of PREDICT_FALSE means the common path is now the
"straight line" path which means that a tight loop calling these
functions may be able to entirely fit within the micro-op cache, etc.
Details here are a bit black magic but it's generally considered good
hygiene for short loops to minimize the code footprint.
- The outlining of the call also means the stack frame size is a bit
smaller. The new code path has three "push" and "pop" instructions in
the function intro/outro whereas the old one had 6.
This improved TSBS perf about 10% for one of the workloads.
Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Reviewed-on: http://gerrit.cloudera.org:8080/15503
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/client/scan_batch.cc
1 file changed, 11 insertions(+), 6 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Todd Lipcon: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] client: micro-optimize result batch cell getters by outlining cold path
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has removed a vote on this change.
Change subject: client: micro-optimize result batch cell getters by outlining cold path
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] client: micro-optimize getters by outlining cold path
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize getters by outlining cold path
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15503/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/15503/1//COMMIT_MSG@7
PS1, Line 7: client: micro-optimize getters by outlining cold path
> I can't really wrap my head around why the outlining improves the perf. Can
+1
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 04:11:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] client: micro-optimize result batch cell getters by outlining cold path
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15503 )
Change subject: client: micro-optimize result batch cell getters by outlining cold path
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15503
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5838576b3e18cbe8e093bf1e9ce6418ce922de63
Gerrit-Change-Number: 15503
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 20 Mar 2020 06:02:57 +0000
Gerrit-HasComments: No