Tim Armstrong has posted comments on this change. Change subject: IMPALA-4123: Fast bit unpacking ......................................................................
Patch Set 12: (8 comments) http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing-test.cc File be/src/util/bit-packing-test.cc: Line 19: #include <cstdlib> > cstd... Done Line 132: > rand() is only guaranteed to have 15 non-zero bits - the high 17 bits may a Done but didn't seed it since flakiness would only happen if we have a bad product bug with incorrect results. http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing.h File be/src/util/bit-packing.h: PS11, Line 74: /// Implementation of Unpack32Values() that uses 32-bit integer loads to : /// unpack values with the > Did you check to see that this benchmarks to be actually faster than if BIT Looks like it might be slightly faster (although within the margin of error) to do the switch on bit_width within Unpack32Values() and UnpackUpTo32Values(). So I'll go with that. http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-packing.inline.h File be/src/util/bit-packing.inline.h: Line 57: return std::make_pair(in_pos, values_to_read); > After some reading, I think you can and should throw a static on these cons For classes it makes sense - if you have a non-static constexpr in a class definition then the compiler almost certainly has to allocate storage in the memory layout of the object (since you could pass the object into third-party code, which could take the address of the member). I don't think we want to do that for local variables. I don't see any benefit - the compiler shouldn't have any problem fully propagating the constants, and there's no reason it would allocate storage locally unless we take the address of the variable. Line 140: case i: return Unpack32Values<OutType, i>(in, in_bytes, out); > constexpr BYTES_TO_READ, if you mark BitUtil::RoundUpNumBytes constexpr, or Done. Marked a few other functions in BitUtil constexpr for consistency. http://gerrit.cloudera.org:8080/#/c/4494/11/be/src/util/bit-stream-utils.h File be/src/util/bit-stream-utils.h: Line 100: /// 'buffer' is the buffer to read from. The buffer's length is 'buffer_len'. > While you're here, can you add "Does not take ownership of the pointer"? Done Line 142: /// Maximum byte length of a vlq encoded int > While you're here, can you DISALLOW_COPY_AND_ASSIGN? I believe we copy them in a couple of places. It's actually harmless and maybe useful since you can fork the state of the reader. I added a comment to document that it was deliberately left defined. http://gerrit.cloudera.org:8080/#/c/4494/12/be/src/util/openssl-util-test.cc File be/src/util/openssl-util-test.cc: Line 27: using std::uniform_int_distribution; Switch from boost for consistency -- To view, visit http://gerrit.cloudera.org:8080/4494 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I12db69409483d208cd4c0f41c27a78aeb6cd3622 Gerrit-PatchSet: 12 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
