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 2016/11/21 20:33:25 UTC
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Hello Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5166
to review the following change.
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
KUDU-1448. Enable AVX2 bitshuffle at runtime
This uses some trickery when building bitshuffle so that we build it
twice: once with normal flags and a second time with -mavx2. Enabling
AVX2 turns on some faster code paths which have been previously
benchmarked to yield a ~30% end-to-end improvement in some Impala scans.
In order to avoid multiply-defined symbols, the build uses 'objcopy' to
rename the symbols to add a '_avx2' suffix before linking. Then a new
wrapper file uses the 'ifunc' attribute to do runtime resolution of the
correct variant to call.
I'd previously attempted to do this upstream in the bitshuffle library
but it was somewhat difficult as the library makes heavy use of macros,
etc. In that prior attempt, I gave up after many hours of work, whereas
this was comparatively quite simple (only an hour of work!)
This also fixes a bug in the cpuid detection which we inherited
from Chromium. I filed the bug upstream here:
https://bugs.chromium.org/p/chromium/issues/detail?id=667457
I tested on my laptop which has AVX2 and verified that the AVX2 variant
functions were getting called.
Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
---
M src/kudu/cfile/CMakeLists.txt
A src/kudu/cfile/bitshuffle_arch_wrapper.cc
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/gutil/cpu.cc
M thirdparty/build-definitions.sh
6 files changed, 143 insertions(+), 10 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5166/1
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
Patch Set 1:
(3 comments)
Just passing through...
http://gerrit.cloudera.org:8080/#/c/5166/1/src/kudu/gutil/cpu.cc
File src/kudu/gutil/cpu.cc:
Line 61: : "a"(info_type)
My x86 knowledge is weak, but I'll ask the dumb question anyway: should the same change be applied here? Or maybe we should acknowledge that this is dead code as far as Kudu is concerned and remove it.
Line 71: : "a"(info_type), "c"(0)
Does Kudu use cpuid to selectively enable/disable other features? Or is this use of avx2 the first such case?
http://gerrit.cloudera.org:8080/#/c/5166/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 447: # iochain.c doesn't benefit from the architecture optimization.
Does it make sense to include iochain.c in the above if only to simplify the build logic a bit? Or would doing so impact runtime negatively?
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5166
to look at the new patch set (#3).
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
KUDU-1448. Enable AVX2 bitshuffle at runtime
This uses some trickery when building bitshuffle so that we build it
twice: once with normal flags and a second time with -mavx2. Enabling
AVX2 turns on some faster code paths which have been previously
benchmarked to yield a ~30% end-to-end improvement in some Impala scans.
In order to avoid multiply-defined symbols, the build uses 'objcopy' to
rename the symbols to add a '_avx2' suffix before linking. Then a new
wrapper file uses the 'ifunc' attribute to do runtime resolution of the
correct variant to call.
I'd previously attempted to do this upstream in the bitshuffle library
but it was somewhat difficult as the library makes heavy use of macros,
etc. In that prior attempt, I gave up after many hours of work, whereas
this was comparatively quite simple (only an hour of work!)
This also fixes a bug in the cpuid detection which we inherited
from Chromium. I filed the bug upstream here:
https://bugs.chromium.org/p/chromium/issues/detail?id=667457
I tested on my laptop which has AVX2 and verified that the AVX2 variant
functions were getting called.
Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
---
M src/kudu/cfile/CMakeLists.txt
A src/kudu/cfile/bitshuffle_arch_wrapper.cc
A src/kudu/cfile/bitshuffle_arch_wrapper.h
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/gutil/cpu.cc
M thirdparty/build-definitions.sh
7 files changed, 178 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5166/3
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5166
to look at the new patch set (#2).
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
KUDU-1448. Enable AVX2 bitshuffle at runtime
This uses some trickery when building bitshuffle so that we build it
twice: once with normal flags and a second time with -mavx2. Enabling
AVX2 turns on some faster code paths which have been previously
benchmarked to yield a ~30% end-to-end improvement in some Impala scans.
In order to avoid multiply-defined symbols, the build uses 'objcopy' to
rename the symbols to add a '_avx2' suffix before linking. Then a new
wrapper file uses the 'ifunc' attribute to do runtime resolution of the
correct variant to call.
I'd previously attempted to do this upstream in the bitshuffle library
but it was somewhat difficult as the library makes heavy use of macros,
etc. In that prior attempt, I gave up after many hours of work, whereas
this was comparatively quite simple (only an hour of work!)
This also fixes a bug in the cpuid detection which we inherited
from Chromium. I filed the bug upstream here:
https://bugs.chromium.org/p/chromium/issues/detail?id=667457
I tested on my laptop which has AVX2 and verified that the AVX2 variant
functions were getting called.
Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
---
M src/kudu/cfile/CMakeLists.txt
A src/kudu/cfile/bitshuffle_arch_wrapper.cc
A src/kudu/cfile/bitshuffle_arch_wrapper.h
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/gutil/cpu.cc
M thirdparty/build-definitions.sh
7 files changed, 178 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/5166/2
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
KUDU-1448. Enable AVX2 bitshuffle at runtime
This uses some trickery when building bitshuffle so that we build it
twice: once with normal flags and a second time with -mavx2. Enabling
AVX2 turns on some faster code paths which have been previously
benchmarked to yield a ~30% end-to-end improvement in some Impala scans.
In order to avoid multiply-defined symbols, the build uses 'objcopy' to
rename the symbols to add a '_avx2' suffix before linking. Then a new
wrapper file uses the 'ifunc' attribute to do runtime resolution of the
correct variant to call.
I'd previously attempted to do this upstream in the bitshuffle library
but it was somewhat difficult as the library makes heavy use of macros,
etc. In that prior attempt, I gave up after many hours of work, whereas
this was comparatively quite simple (only an hour of work!)
This also fixes a bug in the cpuid detection which we inherited
from Chromium. I filed the bug upstream here:
https://bugs.chromium.org/p/chromium/issues/detail?id=667457
I tested on my laptop which has AVX2 and verified that the AVX2 variant
functions were getting called.
Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Reviewed-on: http://gerrit.cloudera.org:8080/5166
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
---
M src/kudu/cfile/CMakeLists.txt
A src/kudu/cfile/bitshuffle_arch_wrapper.cc
A src/kudu/cfile/bitshuffle_arch_wrapper.h
M src/kudu/cfile/bshuf_block.cc
M src/kudu/cfile/bshuf_block.h
M src/kudu/gutil/cpu.cc
M thirdparty/build-definitions.sh
7 files changed, 178 insertions(+), 12 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-1448. Enable AVX2 bitshuffle at runtime
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1448. Enable AVX2 bitshuffle at runtime
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5166/1/src/kudu/cfile/bshuf_block.h
File src/kudu/cfile/bshuf_block.h:
Line 29: #include "kudu/cfile/bitshuffle_arch_wrapper.h"
> error: 'kudu/cfile/bitshuffle_arch_wrapper.h' file not found [clang-diagnos
woops
http://gerrit.cloudera.org:8080/#/c/5166/1/src/kudu/gutil/cpu.cc
File src/kudu/gutil/cpu.cc:
Line 61: : "a"(info_type)
> My x86 knowledge is weak, but I'll ask the dumb question anyway: should the
Yea, I guess it's dead code because we don't support i386, but don't think now's the time to shave that yak (we have 25 such places we check for i386)
As for the actual question, it isn't relevant because ecx is only used for the "extended CPUID flags" (info_type = 7), and that didn't exist on 32-bit afaik.
Line 71: : "a"(info_type), "c"(0)
> Does Kudu use cpuid to selectively enable/disable other features? Or is thi
This is the first case. We check other CPU flags in init.cc but only use them to do an early FATAL on unsupported cpus.
http://gerrit.cloudera.org:8080/#/c/5166/1/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 447: # iochain.c doesn't benefit from the architecture optimization.
> Does it make sense to include iochain.c in the above if only to simplify th
Done
--
To view, visit http://gerrit.cloudera.org:8080/5166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ied8887c0240a649158085676fdd08ab8628bf7d6
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes