Adar Dembo has posted comments on this change. Change subject: Refactor cache interface to handle allocation of values and keys together ......................................................................
Patch Set 2: (3 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); > I don't think that's the case -- you still need to use delete[] on the orig True, Free() won't be simpler. It'll actually be: h->~PendingHandle(); uint8_t* data = reinterpret_cast<uint8_t*>(h); delete [] data; At least, that's if I'm interpreting http://stackoverflow.com/questions/6783993/placement-new-and-delete correctly. But I like the use of a constructor because of separation of concerns: L450-454 are initialization of LRUHandle, which logically belongs in a constructor rather than in the class that allocated the LRUHandle. http://gerrit.cloudera.org:8080/#/c/2957/2/src/kudu/util/cache.h File src/kudu/util/cache.h: Line 45: // Callback interface which is called when an entry is evicted from the > A kudu::Callback is larger than a straight pointer (16 bytes vs 8) and we s Yeah, that's fine. Line 49: virtual void EvictedEntry(Slice key, Slice value) = 0; > yea, if the object is small enough it'll just get passed in registers so it What's the threshold for "small enough"? Slices are 16 bytes, so that's two registers? -- 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
