kudu-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wong (Code Review)" <ger...@cloudera.org>
Subject [kudu-CR] [util] Don't use static function ptrs for non-static member functions in BlockBloom filter
Date Thu, 23 Apr 2020 06:27:03 GMT
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 <bankim@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <aserbin@cloudera.com>
Gerrit-Reviewer: Andrew Wong <awong@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <bankim@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Volodymyr Verovkin <verjovkin@cloudera.com>
Gerrit-Reviewer: Wenzhe Zhou <wzhou@cloudera.com>
Gerrit-Comment-Date: Thu, 23 Apr 2020 06:27:03 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message