Tim Armstrong has posted comments on this change. Change subject: Use AVX2 operations to speedup Bloom filters by 10-100%. ......................................................................
Patch Set 8: Code-Review+2 (3 comments) Looks good, upgrading to +2 since it's all in the utils folder. http://gerrit.cloudera.org:8080/#/c/3338/8/be/src/util/bloom-filter-test.cc File be/src/util/bloom-filter-test.cc: Line 33: uint64_t MakeRand() { Don't need to fix but we we could avoid having to work around rand's limitations by using a different rng. E.g. be/src/runtime/sorter.cc uses mersenne twister. http://gerrit.cloudera.org:8080/#/c/3338/8/be/src/util/bloom-filter.h File be/src/util/bloom-filter.h: Line 109: // TODO: Use Bits::Log2Ceiling64(numeric_limits<BucketWord>::digits) once we enable Maybe just remove this TODO, since I don't think it's actionable right now or all that important. Line 143: __attribute__((__target__("avx2"))); I spent a little time looking at how __attribute__((__target__ is handled in gcc and clang just to understand what happens here, since the gcc docs are unclear on how __attribute__((__target__ is handled. gcc can do function multi-versioning (https://gcc.gnu.org/wiki/FunctionMultiVersioning) if you have multiple declarations of the function with different attributes that are visible at the function callsite. If you do this, then the function call turns into an indirect call via a link table that's set up during dynamic linking. We avoid that here, since there's only a single version of the function declared. When compiling a release build, I only see a direct call in the assembly, so that is good. In the LLVM IR I also see a direct function call: tail call void @_ZN6impala11BloomFilter16BucketInsertAVX2Ejj(%"class.impala::BloomFilter"* nonnull %this, i32 %8, i32 %hash) So I think this pattern is good and doing what we're expecting. -- To view, visit http://gerrit.cloudera.org:8080/3338 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I6fef4f6652876f8fd7e3f0e41431702380418c98 Gerrit-PatchSet: 8 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Jim Apple <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
