You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Zach Amsden (Code Review)" <ge...@cloudera.org> on 2017/01/28 02:47:49 UTC
[Impala-ASF-CR] Impala ABM / LZCNT support
Zach Amsden has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5821
Change subject: Impala ABM / LZCNT support
......................................................................
Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/sse-util.h
4 files changed, 108 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/1
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#7).
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
IMPALA-5266 Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware. It isn't on a perf critical code path yet but
still seems like a decent idea.
PopcountNoHw was absolutely atrocious as it contains a totally
unpredictable loop that can be computed much more efficiently,
so I fixed that as well.
Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/codegen/CMakeLists.txt
M be/src/exprs/bit-byte-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
11 files changed, 377 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/7
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 323: static inline int Log2FloorNonZero(uint32_t n) {
This actually ended up unused, as the 32-bit version of the RoundUpToPowerOfTwoNoHw implementation was slower than the 64-bit one!
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/fixed-size-hash-table.h
File be/src/util/fixed-size-hash-table.h:
Line 53: DCHECK_EQ(capacity_, BitUtil::RoundUpToPowerOfTwo(static_cast<int64_t>(capacity_)));
Uses of unsigned integers actually get caught now.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#2).
Change subject: Impala ABM / LZCNT support
......................................................................
Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware. It isn't on a perf critical code path yet but
still seems like a decent idea.
Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
8 files changed, 280 insertions(+), 14 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/2
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#3).
Change subject: Impala ABM / LZCNT support
......................................................................
Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware. It isn't on a perf critical code path yet but
still seems like a decent idea.
PopcountNoHw was absolutely atrocious as it contains a totally
unpredictable loop that can be computed much more efficiently,
so I fixed that.
Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
8 files changed, 287 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/3
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/5/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 88: return (value >> bits) | (value << (64 - bits));
This is undefined behavior when bits is 0 or 64:
"The behavior is undefined if the right operand is negative, or greater than or equal to the length in bits of the promoted left operand."
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5821/1/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 98: static inline uint32_t RoundUpToPowerOfTwoNoHw32(uint32_t v) {
we don't do unsigned arithmetic, unless we're dealing with bit vectors (but then rounding doesn't make sense).
Line 119: static inline uint32_t RoundUpToPowerOfTwo32(uint32_t v) {
same here
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/3//COMMIT_MSG
Commit Message:
Line 7: Impala ABM / LZCNT support
> Can you file a tracking JIRA for this? We mostly have standardised on alway
Done - IMPALA-5266
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#5).
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
IMPALA-5266 Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware. It isn't on a perf critical code path yet but
still seems like a decent idea.
PopcountNoHw was absolutely atrocious as it contains a totally
unpredictable loop that can be computed much more efficiently,
so I fixed that as well.
Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
8 files changed, 287 insertions(+), 19 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/5
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#8).
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
IMPALA-5266 Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware. It isn't on a perf critical code path yet but
still seems like a decent idea.
PopcountNoHw was absolutely atrocious as it contains a totally
unpredictable loop that can be computed much more efficiently,
so I fixed that as well.
Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/codegen/CMakeLists.txt
M be/src/exprs/bit-byte-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
11 files changed, 378 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/8
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 1:
I don't know if this is a good idea or not but I wanted to learn more about the build and so here it is.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/5/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 88: return (value >> bits) | (value << (64 - bits));
> DCHECK that this isn't the case?
With a comment explaining why.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/5/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 88: return (value >> bits) | (value << (64 - bits));
> DCHECK that this isn't the case?
Can't DCHECK in a constexpr; is documenting the limitation sufficient?
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has abandoned this change. ( http://gerrit.cloudera.org:8080/5821 )
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Abandoned
Looks abandoned, can reopen if needed later
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-Change-Number: 5821
Gerrit-PatchSet: 8
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@gmail.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/1/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 98: static inline uint32_t RoundUpToPowerOfTwoNoHw32(uint32_t v) {
> we don't do unsigned arithmetic, unless we're dealing with bit vectors (but
One of the primary reasons I created this change was to probe exactly that topic. I completely agree that almost all arithmetic should be signed. There are special cases with bit vectors and hash functions that actually require unsigned math, the rotation below is one of them. This function itself could be int32_t without a problem if users are careful about bounds - which they need to be anyway.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/5/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 88: return (value >> bits) | (value << (64 - bits));
> This is undefined behavior when bits is 0 or 64:
DCHECK that this isn't the case?
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 372: return 1LL << BitUtil::Log2CeilingNonZero64(v);
I think this and line 379 below are incorrect when v > (1LL << 62), since (1LL << 63) is not representable as an int64_t. I think they both return INT64_MIN
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has uploaded a new patch set (#6).
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
IMPALA-5266 Impala ABM / LZCNT support
I recently added some code that wants to do upwards power of 2
calculation. Turns out this can be done much more quickly in
hardware. It isn't on a perf critical code path yet but
still seems like a decent idea.
PopcountNoHw was absolutely atrocious as it contains a totally
unpredictable loop that can be computed much more efficiently,
so I fixed that as well.
Testing: Added a perf test to verify this is faster (it is)
and updated the bit-util-test to add better test coverage.
Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
---
M be/src/benchmarks/CMakeLists.txt
A be/src/benchmarks/bit-intrinsics-benchmark.cc
M be/src/codegen/CMakeLists.txt
M be/src/exprs/bit-byte-functions-ir.cc
M be/src/exprs/string-functions-ir.cc
M be/src/util/bit-util-test.cc
M be/src/util/bit-util.h
M be/src/util/cpu-info.cc
M be/src/util/cpu-info.h
M be/src/util/fixed-size-hash-table.h
M be/src/util/sse-util.h
11 files changed, 376 insertions(+), 44 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/5821/6
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/1/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 98: static inline uint32_t RoundUpToPowerOfTwoNoHw32(uint32_t v) {
> One of the primary reasons I created this change was to probe exactly that
let's discuss in person, but i think the function should be careful about bounds, not the caller. also, impala doesn't support unsigned column types, so there isn't really a need for unsigned arithmetic on user data.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Attila Jeges (Code Review)" <ge...@cloudera.org>.
Attila Jeges has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
PS8, Line 300: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(7), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(8), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(65535), 65536);
Duplicate of L297-299. I guess the intent here was to call RoundUpToPowerOfTwo() with 7L, 8L and 65535L, right?
PS8, Line 308: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(7), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(8), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(65535), 65536);
same here
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 376: DCHECK(v > 0);
DCHECK on L389 asserts that RoundUpToPowerOfTwo(int32_t) never overflows. Do we need a similar DCHECK here?
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/3//COMMIT_MSG
Commit Message:
Line 7: Impala ABM / LZCNT support
Can you file a tracking JIRA for this? We mostly have standardised on always having a JIRA for tracking - it's useful as a place to discuss the issue.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/benchmarks/bit-intrinsics-benchmark.cc
File be/src/benchmarks/bit-intrinsics-benchmark.cc:
Line 33: // PopcountSlowNoHw 78.8 79.6 80.3 0.0782X 0.0788X 0.0787X
This should be the baseline for the popcount functions right? I think you'll get the right relative timings for those if you divide this up into multiple suites, one per function.
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/codegen/CMakeLists.txt
File be/src/codegen/CMakeLists.txt:
Line 67: COMMAND ${LLVM_CLANG_EXECUTABLE} ${CLANG_IR_CXX_FLAGS} "-msse4.2" "-mlzcnt" ${CLANG_INCLUDE_FLAGS} ${IR_INPUT_FILES} -o ${IR_SSE_TMP_OUTPUT_FILE}
I don't think this is safe - this tells clang it's safe to emit lzcnt anywhere. Ivy Bridge has SSE4.2 but not LZCNT from what I can tell and we only check for SSE4.2 before loading this version of the module (in LlvmCodeGen::CreateFromMemory()).
Currently we have two ways of dealing with these architectural differences - this compile-time approach where we generate different versions of the module, some of which have the unsupported instructions removed at compile-time (see be/src/util/sse-util.h) - and a run-time approach where we apply a target attribute to the individual function and have a runtime check before calling the function. I think both approaches have downsides so I'm not sure what's right to do here. For the compile-time approach we need an extra IR module for every configuration.
I wish we had a better solution for cases like this.
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 323: static inline int Log2FloorNonZero(uint32_t n) {
> This actually ended up unused, as the 32-bit version of the RoundUpToPowerO
Can we remove it?
Line 387: static inline int32_t RoundUpToPowerOfTwo(int32_t v) {
Do we need the 32-bit version? I think in most cases we should be doing the kind of calculations that use this (buffer sizes, etc) in 64-bit arithmetic - having the overloads could lead to subtle bugs.
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/fixed-size-hash-table.h
File be/src/util/fixed-size-hash-table.h:
Line 53: DCHECK_EQ(capacity_, BitUtil::RoundUpToPowerOfTwo(static_cast<int64_t>(capacity_)));
> Uses of unsigned integers actually get caught now.
This should really be DCHECK(BitUtil::IsPowerOfTwo()) - I think this was written before that function existed.
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/sse-util.h
File be/src/util/sse-util.h:
Line 190: #define POPCNT_popcnt_u32 _mm_popcnt_u32
I mentioned this elsewhere but I don't think these are always available when SSE4.2 is present, so it's not necessarilysafe to have these under the SSE4.2 ifdef.
It looks like POPCNT is (mostly?) available when SSE42 is available: https://en.wikipedia.org/wiki/SSE4#Supporting_CPUs but LZCNT definitely isn't.
I've also seen some cases where VMs report weird CPU info that doesn't match any real processor (e.g. "haswell" without AVX support), so we should be a little careful about what we assume. I can't think of a reason why SSE4.2 would be enabled but POPCNT disabled but who knows.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 8:
(3 comments)
Thanks for taking a look!
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util-test.cc
File be/src/util/bit-util-test.cc:
PS8, Line 300: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(7), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(8), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwo(65535), 65536);
> Duplicate of L297-299. I guess the intent here was to call RoundUpToPowerOf
Original implementation required user specified bit width, so this was testing 32 and 64 bit operators separately. Will get rid of it.
PS8, Line 308: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(7), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(8), 8);
: EXPECT_EQ(BitUtil::RoundUpToPowerOfTwoNoHw(65535), 65536);
> same here
Done
http://gerrit.cloudera.org:8080/#/c/5821/8/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 376: DCHECK(v > 0);
> DCHECK on L389 asserts that RoundUpToPowerOfTwo(int32_t) never overflows. D
I would argue we don't need that check for 64-bit math.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 6:
Updated perf results; these are as good as I could get. I removed some various 8 / 16 / 32 bit pieces that did not show any win.
Need to update tests and remove some dead code.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Jim Apple (Code Review)" <ge...@cloudera.org>.
Jim Apple has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/5/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 88: return (value >> bits) | (value << (64 - bits));
> Can't DCHECK in a constexpr; is documenting the limitation sufficient?
WFM
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5266 Impala ABM / LZCNT support
Posted by "Zach Amsden (Code Review)" <ge...@cloudera.org>.
Zach Amsden has posted comments on this change.
Change subject: IMPALA-5266 Impala ABM / LZCNT support
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5821/5/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 88: return (value >> bits) | (value << (64 - bits));
> Can't DCHECK in a constexpr; is documenting the limitation sufficient?
Turns out none of this matters. clang now refuses to define lzcnt unless target("lzcnt") feature is set, which means we can't use the intrinsics. Also, gutil/bits seems to have been upgraded in capabilities since, so some of this functionality is unnecessary.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Attila Jeges <at...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@apache.org>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] Impala ABM / LZCNT support
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: Impala ABM / LZCNT support
......................................................................
Patch Set 3:
(2 comments)
This got stuck in my review backlog, sorry to take so long to look at it. Based on the benchmarks, seems like a win.
http://gerrit.cloudera.org:8080/#/c/5821/3/be/src/benchmarks/bit-intrinsics-benchmark.cc
File be/src/benchmarks/bit-intrinsics-benchmark.cc:
Line 26: // RoundUpToPowerOfTwo 2.15e+03 2.17e+03 2.2e+03 1.75X 1.74X 1.75X
It's interesting that it's faster even with the runtime CpuInfo check.
http://gerrit.cloudera.org:8080/#/c/5821/3/be/src/util/bit-util.h
File be/src/util/bit-util.h:
Line 64
This implementation had been bugging me for a while, since this function gets used all over the place.
--
To view, visit http://gerrit.cloudera.org:8080/5821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9f6a465ab4a9ee4f582847f8e211a779bdede3d2
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden <za...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zach Amsden <za...@cloudera.com>
Gerrit-HasComments: Yes