Todd Lipcon has posted comments on this change. Change subject: Refactor cache interface to handle allocation of values and keys together ......................................................................
Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/2957/2/src/kudu/cfile/block_cache.h File src/kudu/cfile/block_cache.h: Line 62: } PACKED; > Is this necessary? With two uint64_ts, where will the compiler insert paddi true it's not likely to insert padding, but it serves as a sort of indicator that the in-memory layout of this thing is important. If for some reason a later compiler decided it wanted to add padding, this could break in a hard-to-debug way, so saying PACKED helps. Line 73: PendingEntry(PendingEntry&& other) : PendingEntry() { > Why is it important to chain to the default constructor if the move operato without the chaining, the 'cache_' and 'handle_' members might be arbitrary uninitialized data, in which case the 'reset()' call at the beginning of the assignment operator would try to call cache_->Free() and crash. Line 84: // Free the the pending entry back to the block cache. > Nit: double the Done Line 119: BlockCacheHandle *handle); > Nit: BlockCacheHandle* handle (while you're here). Done Line 146: PendingEntry Allocate(const CacheKey& key, size_t block_size); > Is this return by value because a PendingEntry is never long-lived enough t Yea, it's just a little "owned value wrapper" type thing, and it's movable, so return by value should make sense. Agreed that it would be nice if Cache worked that way too, but Cache has (for historical reasons) a somewhat C-like interface with opaque struct return values. Line 148: // Insert the given block into the cache. 'inserted' is set to refer to the > Maybe just return inserted, instead of treating it as an OUT param? No can do because BlockCacheHandle isn't movable. There's a TODO about that somewhere http://gerrit.cloudera.org:8080/#/c/2957/2/src/kudu/util/cache.cc File src/kudu/util/cache.cc: Line 64: uint8_t* val_ptr() { > Nit: shouldn't this one be called mutable_val_ptr()? Done Line 187: // Call the user's eviction callback and free. > Nit: "if it exists" Done Line 448: uint8_t* buf = new uint8_t[sizeof(LRUHandle) - 1 + key_len_padded + val_len]; > Add a comment explaining what the different elements in the size computatio Done Line 449: LRUHandle* handle = reinterpret_cast<LRUHandle*>(buf); > How about you use the placement new operator here, then you can put L450-L4 I don't think that's the case -- you still need to use delete[] on the original array if it was allocated as new uint8_t[], regardless of whether you did a placement-new into it or not. As for moving the stuff into the constructor, I dont think it really simplifies the code, because then we have a big 5-argument constructor which is harder to read, plus more total lines of code. 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 > Curious why you chose to use an interface here and not just typedef a kudu: A kudu::Callback is larger than a straight pointer (16 bytes vs 8) and we store one in every cache entry. Plus, it's non-POD (non-trivial copy constructor and dtor) so awkward to store inside of the POD LRUHandle structs inside the cache. I think one improvement we could make to all this cache stuff is to make the eviction callback a property of the cache instead of the individual cache entry. Then a larger non-POD Callback would make plenty of sense. Can I defer that change to after the NVM cache stuff is committed? Line 49: virtual void EvictedEntry(Slice key, Slice value) = 0; > Why not pass by cref? I think I remember you saying there's a reason for do yea, if the object is small enough it'll just get passed in registers so it's slightly faster to pass by value instead of by const ref (which is a pointer). Probably doesn't matter for this case, though, happy to change back if you prefer Line 121: // Handle* h = cache_->Insert(h, deleter); > Nit: the rhs here should reference ph, not h. Also it's not really a "delet Done Line 145: virtual uint8_t* MutableValue(PendingHandle* handle) = 0; > Why is this a Cache method and not a PendingHandle one? Seems more intuitiv yea, this class uses a sort of C style -- eg Cache::Value(Handle* h) rather than h->Value(). I think the reasoning here is that you want to minimize the size of the LRU Entry objects, and having them be classes with virtual methods means they'd all have vtables just to accomodate the Handle::Value() function. So, I stuck with that pattern here rather than making it an interface with a virtual method. http://gerrit.cloudera.org:8080/#/c/2957/2/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 541: handle = reinterpret_cast<LRUHandle*>(buf); > Again, placement new would be a nice improvement here. same response to the other comment -- 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
