Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4123: Fast bit unpacking
Patch Set 12:
Line 19: #include <cstdlib>
> 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.
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.
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.
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.
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-Owner: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbap...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>