Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15439 )
Change subject: client: micro-optimizations to reduce CPU and allocations ...................................................................... Patch Set 1: (5 comments) http://gerrit.cloudera.org:8080/#/c/15439/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15439/1//COMMIT_MSG@26 PS1, Line 26: - Add a move constructor for KuduSchema Where does this actually get used? http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/resource_metrics-internal.h File src/kudu/client/resource_metrics-internal.h: http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/resource_metrics-internal.h@55 PS1, Line 55: std::list<std::string> owned_strings_; Isn't std::deque generally a better choice over std::list? If not here, could you add a comment explaining why? http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/resource_metrics.cc File src/kudu/client/resource_metrics.cc: http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/resource_metrics.cc@60 PS1, Line 60: owned_strings_.emplace_back(name); This suggests that owned_strings_ should be some sort of deduplicating data structure (like an unordered_set); why should two calls to Increment() with the same value in 'name' yield two different owned strings? http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/schema.h File src/kudu/client/schema.h: http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/schema.h@718 PS1, Line 718: explicit KuduSchema(Schema&& schema); How will a pre-C++11 compiler (which includes this header) respond to this? Also, should add a move-assignment operator for completeness. http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/schema.cc File src/kudu/client/schema.cc: http://gerrit.cloudera.org:8080/#/c/15439/1/src/kudu/client/schema.cc@826 PS1, Line 826: (Schema&& > nit: pass by value and move? Nah this actually makes sense for a move constructor. -- To view, visit http://gerrit.cloudera.org:8080/15439 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I705aef9f2e57d44f387b58650279130ff329666d Gerrit-Change-Number: 15439 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 16 Mar 2020 07:49:14 +0000 Gerrit-HasComments: Yes
