Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15539 )
Change subject: bitmap: faster implementation of iteration over a bitmap ...................................................................... Patch Set 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/15539/3/src/kudu/util/bitmap.h File src/kudu/util/bitmap.h: http://gerrit.cloudera.org:8080/#/c/15539/3/src/kudu/util/bitmap.h@178 PS3, Line 178: : // Iterate over the bits in 'bitmap' and call 'func' for each set bit. : // 'func' should take a single argument which is the bit's index. : template<class F> : void ForEachSetBit(const uint8_t* __restrict__ bitmap, : int n_bits, : const F& func); : : // Iterate over the bits in 'bitmap' and call 'func' for each unset bit. : // 'func' should take a single argument which is the bit's index. : template<class F> : void ForEachUnsetBit(const uint8_t* __restrict__ bitmap, : int n_bits, : const F& func); > Why not define these down below? wanted to keep this part simple without implementation details for easy reading of the header http://gerrit.cloudera.org:8080/#/c/15539/3/src/kudu/util/bitmap.h@223 PS3, Line 223: base_idx += 64; > Could we do: but base_idx is used on line 220 http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap.h File src/kudu/util/bitmap.h: http://gerrit.cloudera.org:8080/#/c/15539/1/src/kudu/util/bitmap.h@181 PS1, Line 181: template<class F> > Wow, that is a big difference. I think using std::function is fine so long as it's not a perf-sensitive call site where inlining is important. The overhead should be similar to a virtual call. Not sure why no compilers seem to be able to inline it out such a simple case. It seems if I make the 'Foo' function in my godbolt example above not have any loop, it can do it -- To view, visit http://gerrit.cloudera.org:8080/15539 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ibe772a7dd92faf9f99115148ad4cc7df542d1c76 Gerrit-Change-Number: 15539 Gerrit-PatchSet: 3 Gerrit-Owner: Todd Lipcon <t...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Bankim Bhavsar <ban...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 24 Mar 2020 16:57:46 +0000 Gerrit-HasComments: Yes
