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/24 20:27:05 UTC
[kudu-CR] Use popcnt instruction for Bits::Count
Hello Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15549
to review the following change.
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Use popcnt instruction for Bits::Count
I found Bits::Count to be a noticeable contributor to profiles in my
TSDB benchmarking (a couple percent, if I recall correctly). This
switches to using popcnt instead of a lookup table.
This is technically a new CPU requirement, since the popcnt flag is
separate from the SSE4.2 flag. In practice, however, it doesn't appear
that there are any SSE4.2-capable machines that aren't also capable of
POPCNT. So, this shouldn't actually change our hardware requirements.
Another bit of evidence here is that '-msse4.2' (which we use) in clang
also enables -mpopcnt by default, so it was already possible for clang
to emit popcnt instructions for its own optimizations.
In any case, this instruction was introduced about 10 years ago, so even
if I missed one case of a 2010-era server, it's unlikely to still be in
use for Kudu.
Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
---
M src/kudu/gutil/bits.cc
M src/kudu/gutil/cpu.cc
M src/kudu/gutil/cpu.h
M src/kudu/util/init.cc
4 files changed, 27 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/15549/1
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15549 )
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Patch Set 1: Code-Review+1
Nothing to add to Bankim's review
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
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-Comment-Date: Tue, 24 Mar 2020 21:40:49 +0000
Gerrit-HasComments: No
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Andrew Wong, Kudu Jenkins, Andrew Wong, Bankim Bhavsar,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15549
to look at the new patch set (#2).
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Use popcnt instruction for Bits::Count
I found Bits::Count to be a noticeable contributor to profiles in my
TSDB benchmarking (a couple percent, if I recall correctly). This
switches to using popcnt instead of a lookup table.
This is technically a new CPU requirement, since the popcnt flag is
separate from the SSE4.2 flag. In practice, however, it doesn't appear
that there are any SSE4.2-capable machines that aren't also capable of
POPCNT. So, this shouldn't actually change our hardware requirements.
Another bit of evidence here is that '-msse4.2' (which we use) in clang
also enables -mpopcnt by default, so it was already possible for clang
to emit popcnt instructions for its own optimizations.
In any case, this instruction was introduced about 10 years ago, so even
if I missed one case of a 2010-era server, it's unlikely to still be in
use for Kudu.
Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
---
M src/kudu/gutil/bits.cc
M src/kudu/gutil/cpu.cc
M src/kudu/gutil/cpu.h
M src/kudu/util/init.cc
4 files changed, 30 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/49/15549/2
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 2
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)
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15549 )
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15549/1/src/kudu/gutil/bits.cc
File src/kudu/gutil/bits.cc:
http://gerrit.cloudera.org:8080/#/c/15549/1/src/kudu/gutil/bits.cc@33
PS1, Line 33: int Bits::Count(
> This needs to be outside the #ifdef
Done
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 2
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: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Mar 2020 21:46:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15549 )
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15549/1/src/kudu/gutil/bits.cc
File src/kudu/gutil/bits.cc:
http://gerrit.cloudera.org:8080/#/c/15549/1/src/kudu/gutil/bits.cc@33
PS1, Line 33: int nbits = 0;
This needs to be outside the #ifdef
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 24 Mar 2020 20:37:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15549 )
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Patch Set 2: Verified+1 Code-Review+2
Failure of org.apache.kudu.backup.TestOptions seems unrelated.
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 2
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: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Mar 2020 22:46:21 +0000
Gerrit-HasComments: No
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15549 )
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 2
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: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Tue, 24 Mar 2020 21:49:51 +0000
Gerrit-HasComments: No
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15549 )
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Use popcnt instruction for Bits::Count
I found Bits::Count to be a noticeable contributor to profiles in my
TSDB benchmarking (a couple percent, if I recall correctly). This
switches to using popcnt instead of a lookup table.
This is technically a new CPU requirement, since the popcnt flag is
separate from the SSE4.2 flag. In practice, however, it doesn't appear
that there are any SSE4.2-capable machines that aren't also capable of
POPCNT. So, this shouldn't actually change our hardware requirements.
Another bit of evidence here is that '-msse4.2' (which we use) in clang
also enables -mpopcnt by default, so it was already possible for clang
to emit popcnt instructions for its own optimizations.
In any case, this instruction was introduced about 10 years ago, so even
if I missed one case of a 2010-era server, it's unlikely to still be in
use for Kudu.
Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Reviewed-on: http://gerrit.cloudera.org:8080/15549
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/gutil/bits.cc
M src/kudu/gutil/cpu.cc
M src/kudu/gutil/cpu.h
M src/kudu/util/init.cc
4 files changed, 30 insertions(+), 2 deletions(-)
Approvals:
Bankim Bhavsar: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 3
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: Todd Lipcon <to...@apache.org>
[kudu-CR] Use popcnt instruction for Bits::Count
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has removed a vote on this change.
Change subject: Use popcnt instruction for Bits::Count
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15549
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: Iad045e8b77e7baf65c42366eea3e107900eb4a64
Gerrit-Change-Number: 15549
Gerrit-PatchSet: 2
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: Todd Lipcon <to...@apache.org>