Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4123: Fast bit unpacking

Patch Set 12:


File be/src/util/bit-packing-test.cc:

Line 19: #include <cstdlib>
> cstd...

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.

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.

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.

File be/src/util/bit-stream-utils.h:

Line 100:   /// 'buffer' is the buffer to read from.  The buffer's length is 
> While you're here, can you add "Does not take ownership of the pointer"?

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.

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

Reply via email to