You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2020/04/23 19:02:06 UTC
[kudu-CR] tablet: set up RowSetKeyProbe fields in initialization list
Hello Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15790
to review the following change.
Change subject: tablet: set up RowSetKeyProbe fields in initialization list
......................................................................
tablet: set up RowSetKeyProbe fields in initialization list
For some reason, setting these fields in the body of the constructor
generated a lot more code than setting them in the constructor list.
Namely, the setting of a unique_ptr<> generated a check whether the
pointer was already set, and code to deallocate the old version if it
was.
I'm not sure why clang couldn't figure out that it couldn't possibly be
set -- perhaps something to do with inheritance (eg maybe a subclass
would have set the field in its constructor before chaining upwards?).
Either way, this was taking 2-3% CPU and dropped after this change.
Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
---
M src/kudu/tablet/rowset.h
1 file changed, 3 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/90/15790/1
--
To view, visit http://gerrit.cloudera.org:8080/15790
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
Gerrit-Change-Number: 15790
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
[kudu-CR] tablet: set up RowSetKeyProbe fields in initialization list
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15790 )
Change subject: tablet: set up RowSetKeyProbe fields in initialization list
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15790/1/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/15790/1/src/kudu/tablet/rowset.h@319
PS1, Line 319: explicit
> still needed because it has only a single argument and we don't want it to
Aah, my bad :(
--
To view, visit http://gerrit.cloudera.org:8080/15790
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
Gerrit-Change-Number: 15790
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 24 Apr 2020 01:24:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: set up RowSetKeyProbe fields in initialization list
Posted by "Bankim Bhavsar (Code Review)" <ge...@cloudera.org>.
Bankim Bhavsar has posted comments on this change. ( http://gerrit.cloudera.org:8080/15790 )
Change subject: tablet: set up RowSetKeyProbe fields in initialization list
......................................................................
Patch Set 1: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15790/1/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/15790/1/src/kudu/tablet/rowset.h@319
PS1, Line 319: explicit
explicit no longer needed, right?
--
To view, visit http://gerrit.cloudera.org:8080/15790
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
Gerrit-Change-Number: 15790
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 23 Apr 2020 19:07:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] tablet: set up RowSetKeyProbe fields in initialization list
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15790 )
Change subject: tablet: set up RowSetKeyProbe fields in initialization list
......................................................................
tablet: set up RowSetKeyProbe fields in initialization list
For some reason, setting these fields in the body of the constructor
generated a lot more code than setting them in the constructor list.
Namely, the setting of a unique_ptr<> generated a check whether the
pointer was already set, and code to deallocate the old version if it
was.
I'm not sure why clang couldn't figure out that it couldn't possibly be
set -- perhaps something to do with inheritance (eg maybe a subclass
would have set the field in its constructor before chaining upwards?).
Either way, this was taking 2-3% CPU and dropped after this change.
Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
Reviewed-on: http://gerrit.cloudera.org:8080/15790
Reviewed-by: Bankim Bhavsar <ba...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/tablet/rowset.h
1 file changed, 3 insertions(+), 3 deletions(-)
Approvals:
Bankim Bhavsar: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15790
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
Gerrit-Change-Number: 15790
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] tablet: set up RowSetKeyProbe fields in initialization list
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/15790 )
Change subject: tablet: set up RowSetKeyProbe fields in initialization list
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15790/1/src/kudu/tablet/rowset.h
File src/kudu/tablet/rowset.h:
http://gerrit.cloudera.org:8080/#/c/15790/1/src/kudu/tablet/rowset.h@319
PS1, Line 319: explicit
> explicit no longer needed, right?
still needed because it has only a single argument and we don't want it to be an implicit type conversion constructor
--
To view, visit http://gerrit.cloudera.org:8080/15790
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe46e00f5ca8c13c7366df4bee04e7c6e840b3a0
Gerrit-Change-Number: 15790
Gerrit-PatchSet: 1
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Andrew Wong <an...@cloudera.com>
Gerrit-Reviewer: Bankim Bhavsar <ba...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 23 Apr 2020 20:03:09 +0000
Gerrit-HasComments: Yes