Todd Lipcon has posted comments on this change. Change subject: KUDU-1410 (part 2). rpc: clean up generated RPC service code ......................................................................
Patch Set 5: (3 comments) http://gerrit.cloudera.org:8080/#/c/2795/5/src/kudu/rpc/protoc-gen-krpc.cc File src/kudu/rpc/protoc-gen-krpc.cc: Line 487 > I'm trying to gauge the difference in dispatch expense. I ran rpc-bench and couldn't discern any noticeable difference. I think all the cross-thread communication (pthread_cond_signal) and attendant cache misses, wakeup costs, etc, washes all of this out - I've never seen 'Handle' show up at all in profiles of rpc-bench (which I've looked at a fair bit) Given that some of our RPC services have 10s of calls (eg master.proto has 14) I think a single hashtable lookup and indirect call is likely to be much cheaper than 14 string comparisons. The new code might have two branch misses, and the old code had on average 7. http://gerrit.cloudera.org:8080/#/c/2795/5/src/kudu/rpc/service_if.h File src/kudu/rpc/service_if.h: Line 61: std::function<void(const google::protobuf::Message* req, > Isn't it more conventional to pass 'req' as cref if it's read-only? this is a bit of a weird one because the actual handler could pass off the request handling to some other thread, etc, and the pointer actually lives until the RPC is responded to. So I think that's why we went with the const pointer originally, and this is just maintaining that Line 93: void Handle(InboundCall* incoming) override; > Shouldn't it also be defined as virtual? clang actually issues a warning now if you do virtual _and_ override, since it's apparently redundant. http://en.cppreference.com/w/cpp/language/override says that 'override' in itself ensures that the function must be virtual -- To view, visit http://gerrit.cloudera.org:8080/2795 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id7b42a5ba1de59315660a79b125e70938c3a1b2e Gerrit-PatchSet: 5 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
