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 2017/05/16 19:23:04 UTC
[kudu-CR] Enable move constructors for protobufs
Hello Adar Dembo, Grant Henke,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/6900
to review the following change.
Change subject: Enable move constructors for protobufs
......................................................................
Enable move constructors for protobufs
This enables an experimental option for protobuf to generate move
constructors for protos. This should make storing protobufs in
containers more performant, and also should allow us to avoid some
current patterns where we use ::Swap() instead of relying on more normal
looking C++11-style pass-by-value with std::move().
I didn't go through and update places to take advantage of this, except
for just trying it in one spot to make sure it compiles properly. I also
verified in the generated code that move constructors are being
generated.
Change-Id: I775e770799aec44cda79e641980e91259d19e650
---
M src/kudu/rpc/client_negotiation.cc
M thirdparty/build-definitions.sh
2 files changed, 2 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/6900/1
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 6: Code-Review+2
forwarding +2
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 343: # Do a clean build if the patchlevel changed.
> Neat. I think this would be a pretty useful thing to do in general. Would y
mind if I hold off and just wait until we port this all to python? (some day I'll revive that)
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.
Change subject: Enable move constructors for protobufs
......................................................................
Enable move constructors for protobufs
This enables an experimental option for protobuf to generate move
constructors for protos. This should make storing protobufs in
containers more performant, and also should allow us to avoid some
current patterns where we use ::Swap() instead of relying on more normal
looking C++11-style pass-by-value with std::move().
I didn't go through and update places to take advantage of this, except
for just trying it in one spot to make sure it compiles properly. I also
verified in the generated code that move constructors are being
generated.
Change-Id: I775e770799aec44cda79e641980e91259d19e650
Reviewed-on: http://gerrit.cloudera.org:8080/6900
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/rpc/client_negotiation.cc
M thirdparty/build-definitions.sh
M thirdparty/download-thirdparty.sh
3 files changed, 13 insertions(+), 2 deletions(-)
Approvals:
Todd Lipcon: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/1/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
Line 519: *pb.mutable_authn_token() = std::move(*authn_token_);
> Does SignedTokenPB need a move constructor to handle the move?
it generated one when I built locally after rm -Rf build/protobuf* from thirdparty, but I think the thirdparty didn't properly rebuild due to changed CXXFLAGS.
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/1/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
Line 519: *pb.mutable_authn_token() = std::move(*authn_token_);
> warning: passing result of std::move() as a const reference argument; no mo
Clang-tidy's suggestion implies that the assignment operator is being invoked, not the move assignment operator. Any idea why?
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 343: # Do a clean build if the patchlevel changed.
> mind if I hold off and just wait until we port this all to python? (some da
Sure.
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6900
to look at the new patch set (#3).
Change subject: Enable move constructors for protobufs
......................................................................
Enable move constructors for protobufs
This enables an experimental option for protobuf to generate move
constructors for protos. This should make storing protobufs in
containers more performant, and also should allow us to avoid some
current patterns where we use ::Swap() instead of relying on more normal
looking C++11-style pass-by-value with std::move().
I didn't go through and update places to take advantage of this, except
for just trying it in one spot to make sure it compiles properly. I also
verified in the generated code that move constructors are being
generated.
Change-Id: I775e770799aec44cda79e641980e91259d19e650
---
M src/kudu/rpc/client_negotiation.cc
M thirdparty/build-definitions.sh
M thirdparty/download-thirdparty.sh
3 files changed, 13 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/6900/3
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 4:
hrm, it seems once I fixed it to definitely rebuild, now it's failing tests, and I can reproduce locally. Hopefully this doesn't pollute the workspace for everyone else
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Enable move constructors for protobufs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/1/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
Line 519: *pb.mutable_authn_token() = std::move(*authn_token_);
> it generated one when I built locally after rm -Rf build/protobuf* from thi
Makes sense; the DEBUG console output shows that thirdparty was rebuilt, but the existing protobuf build output appears to have been reused (i.e. protobuf was not recompiled).
Is that something we need to address, perhaps by raising the protobuf patchlevel artificially? Or is it safe for this to land as-is?
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/4/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 142: # We use a patchlevel=2 here to force a rebuild after changing CXXFLAGS.
Why the change from 1 to 2? So that you can reuse existing workspaces that tested your patchlevel=1 patch and still have them rebuild?
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/2/thirdparty/build-definitions.sh
File thirdparty/build-definitions.sh:
Line 343: # Do a clean build if the patchlevel changed.
Neat. I think this would be a pretty useful thing to do in general. Would you mind applying it across the board?
(no good deed goes unpunished)
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/4/thirdparty/download-thirdparty.sh
File thirdparty/download-thirdparty.sh:
Line 142: # We use a patchlevel=2 here to force a rebuild after changing CXXFLAGS.
> Why the change from 1 to 2? So that you can reuse existing workspaces that
yea
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/6900
to look at the new patch set (#2).
Change subject: Enable move constructors for protobufs
......................................................................
Enable move constructors for protobufs
This enables an experimental option for protobuf to generate move
constructors for protos. This should make storing protobufs in
containers more performant, and also should allow us to avoid some
current patterns where we use ::Swap() instead of relying on more normal
looking C++11-style pass-by-value with std::move().
I didn't go through and update places to take advantage of this, except
for just trying it in one spot to make sure it compiles properly. I also
verified in the generated code that move constructors are being
generated.
Change-Id: I775e770799aec44cda79e641980e91259d19e650
---
M src/kudu/rpc/client_negotiation.cc
M thirdparty/build-definitions.sh
M thirdparty/download-thirdparty.sh
3 files changed, 13 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/00/6900/2
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Enable move constructors for protobufs
Posted by "Grant Henke (Code Review)" <ge...@cloudera.org>.
Grant Henke has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/1/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
Line 519: *pb.mutable_authn_token() = std::move(*authn_token_);
> Clang-tidy's suggestion implies that the assignment operator is being invok
Does SignedTokenPB need a move constructor to handle the move?
SignedTokenPB(SignedTokenPB&&);
I am not sure protobuf is generating one. Do we pass LANG_CXX11?:
https://github.com/google/protobuf/blob/2f4489a3e504e0a4aaffee69b551c6acc9e08374/src/google/protobuf/compiler/cpp/cpp_message.cc#L1094
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] Enable move constructors for protobufs
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Enable move constructors for protobufs
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6900/1/src/kudu/rpc/client_negotiation.cc
File src/kudu/rpc/client_negotiation.cc:
Line 519: *pb.mutable_authn_token() = std::move(*authn_token_);
> Makes sense; the DEBUG console output shows that thirdparty was rebuilt, bu
yea... I think it's safe to land, but if we want to be able to start coding against it and see accurate perf, we should probably bump the patchlevel. I'll try that.
--
To view, visit http://gerrit.cloudera.org:8080/6900
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I775e770799aec44cda79e641980e91259d19e650
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes