Todd Lipcon has posted comments on this change. Change subject: Refactor cache interface to handle allocation of values and keys together ......................................................................
Patch Set 2: (2 comments) http://gerrit.cloudera.org:8080/#/c/2957/2/src/kudu/util/cache.cc File src/kudu/util/cache.cc: Line 449: LRUHandle* handle = reinterpret_cast<LRUHandle*>(buf); > True, Free() won't be simpler. It'll actually be: I would agree normally except that LRUHandle is a struct rather than a full-fledged class, so the ctor just makes it less readable due to lack of named parameters in C++. Plus, it's scope creep since this is just moved code and this class has always worked this way. http://gerrit.cloudera.org:8080/#/c/2957/2/src/kudu/util/cache.h File src/kudu/util/cache.h: Line 49: virtual void EvictedEntry(Slice key, Slice value) = 0; > What's the threshold for "small enough"? Slices are 16 bytes, so that's two Yea, for Slice, you end up passing in two registers. Here's some comparison of generated assembly: https://godbolt.org/g/sF47YU You can see that the pass-by-reference one ends up requiring a slightly longer instruction in the implementation, and an extra instruction at the call site. The truth is it probably doesn't matter since anywhere it's perf sensitive you've probably gone and inlined things anyway. -- To view, visit http://gerrit.cloudera.org:8080/2957 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iedeb24c0233bf062a0a6450a9fd3a7c57499144f Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
