Todd Lipcon 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: (6 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? KuduSchema::FromSchema which gets called once per scan by ScanConfiguration: @ 0x7f42f9690339 kudu::client::KuduSchema::KuduSchema() @ 0x7f42f9690ad1 kudu::client::KuduSchema::FromSchema() @ 0x7f42f9664940 kudu::client::ScanConfiguration::ScanConfiguration() @ 0x7f42f967beae kudu::client::KuduScanner::Data::Data() 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@47 PS1, Line 47: void Increment(StringPiece name, int64_t amount); > nit: might be obvious, but maybe doc that this should only be used when we yep, forgot that i hadn't finished cleaning up tihs commit when I sent it. 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, cou went with a dense_hash_set instead 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 sure, that's fair. I'll use a set. I also want to mark this as deprecated since I don't think clients should be using this API anyway (it should have been private from the beginning) 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? added an ifdef around it for C++11. I think it's fine for it to just not show up in the header since it's only called by the implementation. Given this isn't a move constructor, but rather a constructor from a different class, I dont think it needs the move-assignment 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&& > Nah this actually makes sense for a move constructor. I could go either way on this -- this isn't a move constructor because it takes a Schema argument, not a KuduSchema argument. But I think this is more canonical for "move-like" constructors. -- 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-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 18 Mar 2020 22:17:22 +0000 Gerrit-HasComments: Yes
