Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15767 )
Change subject: [util] Don't use static function ptrs for non-static member functions in BlockBloom filter ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/15767/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15767/1//COMMIT_MSG@25 PS1, Line 25: - Uses non-static member function pointers for Insert/Find that are initialized : in constructor(this is same as the change before introduction of OrEqual functions) : avoiding the branch in Insert/Find. : - Doesn't use a static function pointer for OrEqual functions and checks for AVX2 : capability at run-time on every invocation. Curious why you opted to use different approaches for these, when their requirements seem identical w.r.t. use of static functions. http://gerrit.cloudera.org:8080/#/c/15767/1/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15767/1/src/kudu/util/block_bloom_filter.h@270 PS1, Line 270: to nit: "in" http://gerrit.cloudera.org:8080/#/c/15767/1/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15767/1/src/kudu/util/block_bloom_filter.cc@258 PS1, Line 258: this-> nit: do we need 'this'? Same above? http://gerrit.cloudera.org:8080/#/c/15767/1/src/kudu/util/block_bloom_filter.cc@284 PS1, Line 284: #ifdef USE_AVX2 : if (has_avx2()) { : BlockBloomFilter::OrEqualArrayAVX2(n, in, out); : } else { : BlockBloomFilter::OrEqualArrayNoAVX2(n, in, out); : } : #else : BlockBloomFilter::OrEqualArrayNoAVX2(n, in, out); : #endif Why not define these in the constructor too? -- To view, visit http://gerrit.cloudera.org:8080/15767 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I39703fd1a7e256ff60ef86d0b370590fbb526380 Gerrit-Change-Number: 15767 Gerrit-PatchSet: 1 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Thu, 23 Apr 2020 05:32:37 +0000 Gerrit-HasComments: Yes
