impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4123: Fast bit unpacking
Date Mon, 17 Oct 2016 18:59:36 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message