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: Code-Review+1 (3 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. > Insert/Find are not static member functions whereas OrEqual functions are. Ah I see. In that case, could you update the preface to these bullets? The main difference between Insert/Find and OrEqualArray isn't how common they are. One is dependent on data members, the other is static. It'd be better to highlight that up front -- I misinterpreted this block and thought it meant that we are taking different approaches to fix the same problem because of differences in how hot the codepaths are. 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 > "to" is intended here. Ah, seems I misinterpreted this. I think mentioning static/non-static here is a bit confusing given these are now squarely non-static, and the pointed-to functions are also non-static. Consider removing the "Note:" entirely. 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@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 > OrEqual variant functions are static so will need to use a std::once_flag t Ack -- 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 06:27:03 +0000 Gerrit-HasComments: Yes
