Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15450 )
Change subject: [util] Minor changes in BlockBloomFilter requested by Impala ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.h File src/kudu/util/block_bloom_filter.h: http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.h@263 PS2, Line 263: // Function pointers initialized just once to avoid run-time cost : // in hot-path of Find, Insert and Or operations. : static decltype(&BlockBloomFilter::BucketInsert) bucket_insert_func_ptr_; : static decltype(&BlockBloomFilter::BucketFind) bucket_find_func_ptr_; : static decltype(&BlockBloomFilter::OrEqualArrayNoAVX2) or_equal_array_func_ptr_; > Do these need to be declared in the header? I don't see them used. Datatype of function ptrs is derived from private member functions of this class. If these function ptrs are not member variables then in the .cc file can't access private member functions using decltype on defining/initializing them. I briefly explored using typedef/using to create separate public types for each of these function ptrs but couldn't get the matching types to work. In either case whether file static or class member static there is no runtime impact in terms of memory usage, so didn't dig deeper. http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.cc File src/kudu/util/block_bloom_filter.cc: http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/block_bloom_filter.cc@48 PS2, Line 48: // Flag used to initialize the static function pointers for the BlockBloomFilter class. : static std::once_flag g_init_func_ptrs_flag; > Move it into the anonymous namespace and you won't need to declare it stati This flag is not accessed from outside this .cc file and don't see a need in future, hence static. Inside the kudu namespace or outside doesn't make a difference. http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/hash_util.h File src/kudu/util/hash_util.h: http://gerrit.cloudera.org:8080/#/c/15450/2/src/kudu/util/hash_util.h@139 PS2, Line 139: // Can't use LOG(FATAL)/CHECK() since including glog/logging.h causes problems : // with code-gen in Impala. > Depending on whether this is on a hot path or not, you could also move it t This function is in the hot-path as it's accessed against every column value when checking against BF predicate. Need it to be inline. https://github.com/apache/kudu/blob/master/src/kudu/common/column_predicate.h#L333 https://github.com/apache/kudu/blob/master/src/kudu/util/block_bloom_filter.h#L123 -- To view, visit http://gerrit.cloudera.org:8080/15450 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5944f80f4c071ce787eded3f5b41d3bc56560cd0 Gerrit-Change-Number: 15450 Gerrit-PatchSet: 2 Gerrit-Owner: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Bankim Bhavsar <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Wenzhe Zhou <[email protected]> Gerrit-Comment-Date: Tue, 17 Mar 2020 01:36:36 +0000 Gerrit-HasComments: Yes
