[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12157 ) Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Reviewed-on: http://gerrit.cloudera.org:8080/12157 Tested-by: Adar Dembo Reviewed-by: Mike Percy --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/rowblock.h 3 files changed, 97 insertions(+), 36 deletions(-) Approvals: Adar Dembo: Verified Mike Percy: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 10 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12157 ) Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. Patch Set 9: Code-Review+2 (1 comment) http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc@217 PS6, Line 217: // Some entries are randomly deselected in order to exercise the selection > Think of the deselection as yet another predicate: besides omitting certain Thanks. -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 23 Jan 2019 23:04:00 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12157 ) Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. Patch Set 9: Verified+1 Overriding Jenkins, ASAN failure was known flake KUDU-1736. -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 14 Jan 2019 21:37:50 + Gerrit-HasComments: No
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Adar Dembo has removed a vote on this change. Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 9 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12157 to look at the new patch set (#8). Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc M src/kudu/common/rowblock.h 3 files changed, 97 insertions(+), 36 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/8 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 8 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/12157 )
Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock
..
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators-test.cc
File src/kudu/common/generic_iterators-test.cc:
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators-test.cc@235
PS4, Line 235: }
> would this work just using emplace_back(std::move(ints), std::move(sv))? or
Brace initialization apparently only works if I qualify the initializer-list
with List:
all_ints.emplace_back(List{ std::move(ints), std::move(sv) });
http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc
File src/kudu/common/generic_iterators-test.cc:
http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc@217
PS6, Line 217: VLOG(2) << Substitute("Row $0 with value $1 selected? $2",
> Would you mind adding a comment about this? I'm confused about what is bein
Think of the deselection as yet another predicate: besides omitting certain
rows from output of the iteration it also affects the list of ints we expect
(see L227).
Will add a comment.
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc@199
PS4, Line 199: // Seek next_row_ to the first selected row.
> maybe this isn't a bottleneck, but mind adding a TODO to use BitmapFindFirs
The only reason I didn't use BitmapFindFirst already is because a follow-up
patch needs to find the last selected row, and I didn't want to spend a bunch
of time writing a correct BitmapFindLast. And without BitmapFindLast, the
asymmetry bothered me. :/
But that's pretty silly, so I modified this to use BitmapFindFirst.
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc@207
PS4, Line 207: LOG(DFATAL) << "Unreachable code"; // guaranteed by the
short-circuit above
> wonder whether this should be a real FATAL - note that DFATAL still emits e
Moot point now; BitmapFindFirst alters the control flow so that this isn't
needed anymore.
--
To view, visit http://gerrit.cloudera.org:8080/12157
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b
Gerrit-Change-Number: 12157
Gerrit-PatchSet: 7
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 11 Jan 2019 23:52:49 +
Gerrit-HasComments: Yes
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12157 to look at the new patch set (#7). Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc 2 files changed, 84 insertions(+), 29 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/7 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/12157 ) Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc File src/kudu/common/generic_iterators-test.cc: http://gerrit.cloudera.org:8080/#/c/12157/6/src/kudu/common/generic_iterators-test.cc@217 PS6, Line 217: bool row_selected = prng.Uniform(8) > 0; Would you mind adding a comment about this? I'm confused about what is being tested and how, particularly why it's OK to randomly deselect rows in these tests given a predicate. -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 09 Jan 2019 22:29:42 + Gerrit-HasComments: Yes
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12157 to look at the new patch set (#6). Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc 2 files changed, 84 insertions(+), 28 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/6 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 6 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12157 to look at the new patch set (#5). Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc 2 files changed, 73 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/5 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 5 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/12157 )
Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock
..
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators-test.cc
File src/kudu/common/generic_iterators-test.cc:
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators-test.cc@235
PS4, Line 235: all_ints.emplace_back(std::move(l));
would this work just using emplace_back(std::move(ints), std::move(sv))? or
maybe similar but with a {} in there?
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc
File src/kudu/common/generic_iterators.cc:
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc@199
PS4, Line 199: // Seek next_row_ to the first selected row.
maybe this isn't a bottleneck, but mind adding a TODO to use BitmapFindFirst
here?
http://gerrit.cloudera.org:8080/#/c/12157/4/src/kudu/common/generic_iterators.cc@207
PS4, Line 207: LOG(DFATAL) << "Unreachable code"; // guaranteed by the
short-circuit above
wonder whether this should be a real FATAL - note that DFATAL still emits error
log messages in release builds
--
To view, visit http://gerrit.cloudera.org:8080/12157
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b
Gerrit-Change-Number: 12157
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mike Percy
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Wed, 09 Jan 2019 18:21:49 +
Gerrit-HasComments: Yes
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, Kudu Jenkins, Grant Henke, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12157 to look at the new patch set (#4). Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. There was a lack of selection vector coverage in the existing fuzzy merge tests, so I've modified them to randomly use one, and added a new test too. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators-test.cc M src/kudu/common/generic_iterators.cc 2 files changed, 73 insertions(+), 17 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/4 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 4 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Todd Lipcon
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12157 to look at the new patch set (#2). Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators.cc 1 file changed, 6 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/2 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 2 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy
[kudu-CR] generic iterators: short-circuit MergeIterState::PullNextBlock
Hello Mike Percy, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12157 to review the following change. Change subject: generic_iterators: short-circuit MergeIterState::PullNextBlock .. generic_iterators: short-circuit MergeIterState::PullNextBlock This is a micro-optimization to avoid testing every bit in the selection vector when we've already counted up the number of set bits. Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b --- M src/kudu/common/generic_iterators.cc 1 file changed, 6 insertions(+), 2 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/12157/1 -- To view, visit http://gerrit.cloudera.org:8080/12157 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I529e632f40943962d34c184b29b5cff7604ad59b Gerrit-Change-Number: 12157 Gerrit-PatchSet: 1 Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Mike Percy
