Tim Armstrong has posted comments on this change. Change subject: Use AVX2 operations to speedup Bloom filters by 10-100%. ......................................................................
Patch Set 7: Code-Review+1 (8 comments) http://gerrit.cloudera.org:8080/#/c/3338/7/be/src/util/bloom-filter-test.cc File be/src/util/bloom-filter-test.cc: PS7, Line 41: instructionshalf nit: missing space before 'half' Line 44: void BfInsert(BloomFilter& bf, uint32_t h) { Nice, I like this approach. http://gerrit.cloudera.org:8080/#/c/3338/7/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 This should be enabled now, so if you feel like trying, we could just fix it now. Line 176: if (CpuInfo::IsSupported(CpuInfo::AVX2)) { > Can you call CpuInfo::IsSupported once at Init or BloomFilter() and use a c We're going to optimise out CpuInfo::IsSupported() in codegen with a uniform approach once we've got a better constant-replacement infrastructure (IMPALA-3637) PS7, Line 218: BLOOM_HASH_CONSTANTS Maybe just undefine at the bottom of the header? It'll be easier to spot. Or make the macro name more unique and don't worry about undefining it, e.g. IMPALA_BLOOM_HASH_CONSTANTS Line 244: inline bool BloomFilter::BucketFindAVX2( nit: could cut down on vertical whitespace in this function. PS7, Line 250: is nit: 'if' not 'is' Line 269: return false; Don't need to fix, but I wonder if it would be more efficient to always check every word and move the branch outside the loop. That requires more bit manipulation but might be offset by reduced (unpredictable) branches. I guess it doesn't really matter since this is the slow path now. -- 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: 7 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
