You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Tim Armstrong (Code Review)" <ge...@cloudera.org> on 2017/07/18 23:31:38 UTC
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Tim Armstrong has uploaded a new change for review.
http://gerrit.cloudera.org:8080/7459
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
IMPALA-5676: avoid expensive consistency checks in BTSv2
Doing an O(n) consistency check every time the read or write
page was advanced results in O(n^2) overall runtime.
The fix is to separate the O(1) and O(n) checks and only do the
O(n) checks if:
* The function does an an O(n) pass over the pages anyway (e.g.
PinStream())
* The function is called only once per read or write pass over the
stream.
This should make the cost of the checks O(n) (if we make the reasonable
assumption that PrepareForWrite(), PrepareForRead(), PinStream() and
UnpinStream() are called a bounded number of times per stream).
Testing:
Ran BufferedTupleStreamV2Test.
Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
---
M be/src/runtime/buffered-tuple-stream-v2.cc
M be/src/runtime/buffered-tuple-stream-v2.h
2 files changed, 46 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/7459/1
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7459
to look at the new patch set (#4).
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
IMPALA-5676: avoid expensive consistency checks in BTSv2
Doing an O(n) consistency check every time the read or write
page was advanced results in O(n^2) overall runtime.
The fix is to separate the O(1) and O(n) checks and only do the
O(n) checks if:
* The function does an an O(n) pass over the pages anyway (e.g.
PinStream())
* The function is called only once per read or write pass over the
stream.
This should make the cost of the checks O(n) (if we make the reasonable
assumption that PrepareForWrite(), PrepareForRead(), PinStream() and
UnpinStream() are called a bounded number of times per stream).
Testing:
Ran BufferedTupleStreamV2Test.
Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
---
M be/src/runtime/buffered-tuple-stream-v2.cc
M be/src/runtime/buffered-tuple-stream-v2.h
2 files changed, 43 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/7459/4
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 4: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/895/
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#2).
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
IMPALA-5676: avoid expensive consistency checks in BTSv2
Doing an O(n) consistency check every time the read or write
page was advanced results in O(n^2) overall runtime.
The fix is to separate the O(1) and O(n) checks and only do the
O(n) checks if:
* The function does an an O(n) pass over the pages anyway (e.g.
PinStream())
* The function is called only once per read or write pass over the
stream.
This should make the cost of the checks O(n) (if we make the reasonable
assumption that PrepareForWrite(), PrepareForRead(), PinStream() and
UnpinStream() are called a bounded number of times per stream).
Testing:
Ran BufferedTupleStreamV2Test.
Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
---
M be/src/runtime/buffered-tuple-stream-v2.cc
M be/src/runtime/buffered-tuple-stream-v2.h
2 files changed, 43 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/7459/2
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 4:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/895/
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 3: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/7459/3/be/src/runtime/buffered-tuple-stream-v2.cc
File be/src/runtime/buffered-tuple-stream-v2.cc:
PS3, Line 503: CHECK_CONSISTENCY_FAST();
If I understand it correctly, this should happen after all pages have been read so it should be fine to use CHECK_CONSISTENCY_FULL(). Please feel free to leave it as-is though.
http://gerrit.cloudera.org:8080/#/c/7459/3/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:
PS3, Line 695: uccess from a public API call. The Fast version
: /// has constant runtime. The Full version includes additional checks that are O(n)
Why changed it back ?
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 4: Code-Review+2
(2 comments)
Carry +2
http://gerrit.cloudera.org:8080/#/c/7459/3/be/src/runtime/buffered-tuple-stream-v2.cc
File be/src/runtime/buffered-tuple-stream-v2.cc:
PS3, Line 503: CHECK_CONSISTENCY_FULL();
> If I understand it correctly, this should happen after all pages have been
Good point, this seems like a good place to check it.
http://gerrit.cloudera.org:8080/#/c/7459/3/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:
PS3, Line 695: uccess from a public API call. The Fast version
: /// has constant runtime and does not check all of 'pages_'. The Full version inclu
> Why changed it back ?
Too many branches :( I pushed a different draft patch based on an old branch that had the earlier version of the commit included.
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Hello Impala Public Jenkins, Michael Ho,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7459
to look at the new patch set (#5).
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
IMPALA-5676: avoid expensive consistency checks in BTSv2
Doing an O(n) consistency check every time the read or write
page was advanced results in O(n^2) overall runtime.
The fix is to separate the O(1) and O(n) checks and only do the
O(n) checks if:
* The function does an an O(n) pass over the pages anyway (e.g.
PinStream())
* The function is called only once per read or write pass over the
stream.
This should make the cost of the checks O(n) (if we make the reasonable
assumption that PrepareForWrite(), PrepareForRead(), PinStream() and
UnpinStream() are called a bounded number of times per stream).
Testing:
Ran BufferedTupleStreamV2Test.
Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
---
M be/src/runtime/buffered-tuple-stream-v2.cc
M be/src/runtime/buffered-tuple-stream-v2.h
2 files changed, 45 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/7459/5
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7459/1/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:
Line 696: /// has constant runtime. The Full version includes additional checks that are O(n)
> May help to have a one line explanation on the difference in areas in which
Done
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has uploaded a new patch set (#3).
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
IMPALA-5676: avoid expensive consistency checks in BTSv2
Doing an O(n) consistency check every time the read or write
page was advanced results in O(n^2) overall runtime.
The fix is to separate the O(1) and O(n) checks and only do the
O(n) checks if:
* The function does an an O(n) pass over the pages anyway (e.g.
PinStream())
* The function is called only once per read or write pass over the
stream.
This should make the cost of the checks O(n) (if we make the reasonable
assumption that PrepareForWrite(), PrepareForRead(), PinStream() and
UnpinStream() are called a bounded number of times per stream).
Testing:
Ran BufferedTupleStreamV2Test.
Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
---
M be/src/runtime/buffered-tuple-stream-v2.cc
M be/src/runtime/buffered-tuple-stream-v2.h
2 files changed, 46 insertions(+), 28 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/59/7459/3
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
IMPALA-5676: avoid expensive consistency checks in BTSv2
Doing an O(n) consistency check every time the read or write
page was advanced results in O(n^2) overall runtime.
The fix is to separate the O(1) and O(n) checks and only do the
O(n) checks if:
* The function does an an O(n) pass over the pages anyway (e.g.
PinStream())
* The function is called only once per read or write pass over the
stream.
This should make the cost of the checks O(n) (if we make the reasonable
assumption that PrepareForWrite(), PrepareForRead(), PinStream() and
UnpinStream() are called a bounded number of times per stream).
Testing:
Ran BufferedTupleStreamV2Test.
Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Reviewed-on: http://gerrit.cloudera.org:8080/7459
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/buffered-tuple-stream-v2.cc
M be/src/runtime/buffered-tuple-stream-v2.h
2 files changed, 45 insertions(+), 28 deletions(-)
Approvals:
Impala Public Jenkins: Verified
Tim Armstrong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Michael Ho (Code Review)" <ge...@cloudera.org>.
Michael Ho has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7459/1/be/src/runtime/buffered-tuple-stream-v2.h
File be/src/runtime/buffered-tuple-stream-v2.h:
Line 696: /// has constant runtime. The Full version includes additional checks that are O(n)
May help to have a one line explanation on the difference in areas in which these two functions cover.
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-5676: avoid expensive consistency checks in BTSv2
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-5676: avoid expensive consistency checks in BTSv2
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/899/
--
To view, visit http://gerrit.cloudera.org:8080/7459
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8b380fcd0568cb73b36a490954bcd316db969ede
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No