[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Todd Lipcon has submitted this change and it was merged. Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Misc optimizations to BinaryPlainBlockDecoder Looking at a profile while running a YCSB read workload against a binary built with clang, I noticed that BinaryPlainDecoder had done a really poor job of inlining vector::push_back when building the offsets array. This switches away from using a vector there and instead uses a simple buffer/pointer view into the buffer. I also rearranged a bit of other code and added PREDICT_FALSEs to try to get the code into a tighter loop. Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Reviewed-on: http://gerrit.cloudera.org:8080/6159 Tested-by: Kudu Jenkins Reviewed-by: David Ribeiro Alves --- M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h 2 files changed, 43 insertions(+), 26 deletions(-) Approvals: David Ribeiro Alves: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
David Ribeiro Alves has posted comments on this change. Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Patch Set 4: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
David Ribeiro Alves has posted comments on this change. Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Patch Set 4: doh! yeah, wrong rev. sorry about that -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Todd Lipcon has posted comments on this change.
Change subject: Misc optimizations to BinaryPlainBlockDecoder
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6159/2/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:
PS2, Line 200: else {
: p = coding::DecodeGroupVarInt32_SlowButSafe(
: p, &dst_ptr[0], &dst_ptr[1], &dst_ptr[2], &dst_ptr[3]);
: if (PREDICT_FALSE(p > limit)) {
:
> mind adding the latter part of your answer to the comment on the slow path?
I did, did you look at the diff from r2 to r4?
--
To view, visit http://gerrit.cloudera.org:8080/6159
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
David Ribeiro Alves has posted comments on this change.
Change subject: Misc optimizations to BinaryPlainBlockDecoder
..
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6159/2/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:
PS2, Line 200: else {
: p = coding::DecodeGroupVarInt32_SlowButSafe(
: p, &dst_ptr[0], &dst_ptr[1], &dst_ptr[2], &dst_ptr[3]);
: if (PREDICT_FALSE(p > limit)) {
:
> per the comment above, in the fast path, the function can add at most 17 to
mind adding the latter part of your answer to the comment on the slow path?
--
To view, visit http://gerrit.cloudera.org:8080/6159
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Todd Lipcon has posted comments on this change.
Change subject: Misc optimizations to BinaryPlainBlockDecoder
..
Patch Set 4:
(2 comments)
woops, yes, I did.
http://gerrit.cloudera.org:8080/#/c/6159/2/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:
PS2, Line 200: else {
: p = coding::DecodeGroupVarInt32_SlowButSafe(
: p, &dst_ptr[0], &dst_ptr[1], &dst_ptr[2], &dst_ptr[3]);
: if (PREDICT_FALSE(p > limit)) {
:
> not sure I follow, why a DCHECK in the fast patch and just a warning in the
per the comment above, in the fast path, the function can add at most 17 to
'p', and because we know that p_orig + 16 < limit, then p_orig + 17 <= limit,
and thus 'p' after the decoding is still <= limit.
In the "slow path" case here, we know that 'p' was already close to the limit
before decoding, and therefore might have overrun it by the time we return.
http://gerrit.cloudera.org:8080/#/c/6159/2/src/kudu/cfile/binary_plain_block.h
File src/kudu/cfile/binary_plain_block.h:
PS2, Line 142: the o
> docs
Done
--
To view, visit http://gerrit.cloudera.org:8080/6159
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-HasComments: Yes
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
David Ribeiro Alves has posted comments on this change. Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Patch Set 4: I see that my comment on the DCHECK was not addressed. Did you forget to post the replies where you explain why not? -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-HasComments: No
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6159 to look at the new patch set (#4). Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Misc optimizations to BinaryPlainBlockDecoder Looking at a profile while running a YCSB read workload against a binary built with clang, I noticed that BinaryPlainDecoder had done a really poor job of inlining vector::push_back when building the offsets array. This switches away from using a vector there and instead uses a simple buffer/pointer view into the buffer. I also rearranged a bit of other code and added PREDICT_FALSEs to try to get the code into a tighter loop. Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd --- M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h 2 files changed, 43 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6159/4 -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 4 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6159 to look at the new patch set (#3). Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Misc optimizations to BinaryPlainBlockDecoder Looking at a profile while running a YCSB read workload against a binary built with clang, I noticed that BinaryPlainDecoder had done a really poor job of inlining vector::push_back when building the offsets array. This switches away from using a vector there and instead uses a simple buffer/pointer view into the buffer. I also rearranged a bit of other code and added PREDICT_FALSEs to try to get the code into a tighter loop. Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd --- M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h 2 files changed, 39 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6159/3 -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
David Ribeiro Alves has posted comments on this change.
Change subject: Misc optimizations to BinaryPlainBlockDecoder
..
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/6159/2/src/kudu/cfile/binary_plain_block.cc
File src/kudu/cfile/binary_plain_block.cc:
PS2, Line 200: if (PREDICT_FALSE(p > limit)) {
: // Only need to check 'p' overrun in the slow path - see
above DCHECK.
: LOG(WARNING) << "bad block: " << HexDump(data_);
: return Status::Corruption(StringPrintf("unable to decode
offsets in block"));
: }
not sure I follow, why a DCHECK in the fast patch and just a warning in the
slow path?
http://gerrit.cloudera.org:8080/#/c/6159/2/src/kudu/cfile/binary_plain_block.h
File src/kudu/cfile/binary_plain_block.h:
PS2, Line 142: offset
docs
--
To view, visit http://gerrit.cloudera.org:8080/6159
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Andrew Wong
Gerrit-Reviewer: David Ribeiro Alves
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Hello David Ribeiro Alves, Andrew Wong, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/6159 to look at the new patch set (#2). Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Misc optimizations to BinaryPlainBlockDecoder Looking at a profile while running a YCSB read workload against a binary built with clang, I noticed that BinaryPlainDecoder had done a really poor job of inlining vector::push_back when building the offsets array. This switches away from using a vector there and instead uses a simple buffer/pointer view into the buffer. I also rearranged a bit of other code and added PREDICT_FALSEs to try to get the code into a tighter loop. Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd --- M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h 2 files changed, 37 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6159/2 -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] Misc optimizations to BinaryPlainBlockDecoder
Hello David Ribeiro Alves, Andrew Wong, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/6159 to review the following change. Change subject: Misc optimizations to BinaryPlainBlockDecoder .. Misc optimizations to BinaryPlainBlockDecoder Looking at a profile while running a YCSB read workload, I noticed that BinaryPlainDecoder had done a really poor job of inlining vector::push_back when building the offsets array. This switches away from using a vector there and instead uses a simple buffer/pointer view into the buffer. I also rearranged a bit of other code and added PREDICT_FALSEs to try to get the code into a tighter loop. Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd --- M src/kudu/cfile/binary_plain_block.cc M src/kudu/cfile/binary_plain_block.h 2 files changed, 37 insertions(+), 26 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/6159/1 -- To view, visit http://gerrit.cloudera.org:8080/6159 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I5b5061818de36dc268cd5d4fc8553bceeca5dadd Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Andrew Wong Gerrit-Reviewer: David Ribeiro Alves
