Jim Apple has posted comments on this change. Change subject: Use AVX2 operations to speedup Bloom filters by 10-100%. ......................................................................
Patch Set 7: (7 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' Done 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 i Unfortunately, it isn't constexpr yet, and it's in a Google library. Line 176: if (CpuInfo::IsSupported(CpuInfo::AVX2)) { > Can you call CpuInfo::IsSupported once at Init or BloomFilter() and use a c I could call it in the constructor, but given how cheap it is to check, do you think it will save any cycles? https://github.com/cloudera/Impala/blob/cdh5-trunk/be/src/util/cpu-info.h#L61 PS7, Line 218: BLOOM_HASH_CONSTANTS > Maybe just undefine at the bottom of the header? It'll be easier to spot. O added IMPALA_, moved undef to bottom of file Line 244: inline bool BloomFilter::BucketFindAVX2( > nit: could cut down on vertical whitespace in this function. Slimmed down whitespace here and elsewhere. PS7, Line 250: is > nit: 'if' not 'is' Done Line 269: return false; > Don't need to fix, but I wonder if it would be more efficient to always che It was slower last time I tried. -- 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
