You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/05/05 06:43:03 UTC
[kudu-CR] [util] use emplace() instead of insert()
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/6807
Change subject: [util] use emplace() instead of insert()
......................................................................
[util] use emplace() instead of insert()
Since we are using C++11-compiant compiler, it's possible to avoid
unnecessary copying and use emplace() instead of insert() for dictionary
and hash containers.
Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
---
M src/kudu/gutil/map-util.h
1 file changed, 10 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/07/6807/1
--
To view, visit http://gerrit.cloudera.org:8080/6807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] use emplace() instead of insert()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [util] use emplace() instead of insert()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6807/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:
Line 364: pair<typename Collection::iterator, bool> ret = collection->emplace(key, value);
> I think you are right, but that would be another level of optimization. As
hm, I'm not sure I believe that it makes a difference... I wrote this test program:
https://gist.github.com/075a7dfe3db157893af526078968ba25
which puts shared_ptr<>s in a map, so that there is a non-trivial copy constructor (incrementing ref count), and found that there was no difference between these options. The optimizer's probably smart enough to see where it can do these optimizations as is
--
To view, visit http://gerrit.cloudera.org:8080/6807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] use emplace() instead of insert()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [util] use emplace() instead of insert()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6807/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:
Line 364: pair<typename Collection::iterator, bool> ret = collection->emplace(key, value);
> don't we need to change 'key' and 'value' above to be by-value and then use
I think you are right, but that would be another level of optimization. As I understand, using emplace() instead of insert(), we are getting rid of extra value_type() (i.e. pair<key, mapped_value>) constructor.
I would prefer to address changing const-ref semantics to move semantics separately, because there might be some implications at call sites which would require modifying more code in many different places.
--
To view, visit http://gerrit.cloudera.org:8080/6807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] use emplace() instead of insert()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change.
Change subject: [util] use emplace() instead of insert()
......................................................................
Abandoned
not needed
--
To view, visit http://gerrit.cloudera.org:8080/6807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [util] use emplace() instead of insert()
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [util] use emplace() instead of insert()
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/6807/1/src/kudu/gutil/map-util.h
File src/kudu/gutil/map-util.h:
Line 364: pair<typename Collection::iterator, bool> ret = collection->emplace(key, value);
don't we need to change 'key' and 'value' above to be by-value and then use std::move here? Otherwise it's still going to be calling the copy constructor instead of an available move constructor
--
To view, visit http://gerrit.cloudera.org:8080/6807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [util] use emplace() instead of insert()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [util] use emplace() instead of insert()
......................................................................
Patch Set 1: Verified+1
Unrelated flake: ClientTest.TestFailedDnsResolution
--
To view, visit http://gerrit.cloudera.org:8080/6807
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic40b17d9315d7c42d589fd30ce55242540e7767f
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No