Adar Dembo 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 padding? Line 73: PendingEntry(PendingEntry&& other) : PendingEntry() { Why is it important to chain to the default constructor if the move operator overwrites cache_ and handle_ anyway? Line 84: // Free the the pending entry back to the block cache. Nit: double the Line 119: BlockCacheHandle *handle); Nit: BlockCacheHandle* handle (while you're here). Line 146: PendingEntry Allocate(const CacheKey& key, size_t block_size); Is this return by value because a PendingEntry is never long-lived enough to justify allocating on the heap? Would be nice if Cache worked the same way, but there the heap allocation is so you can hide the PendingHandle implementation? 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? 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()? Line 187: // Call the user's eviction callback and free. Nit: "if it exists" 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 computation represent? Line 449: LRUHandle* handle = reinterpret_cast<LRUHandle*>(buf); How about you use the placement new operator here, then you can put L450-L454 in the LRUHandle constructor? And it should simplify Free() too; you can just "delete h". 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::Callback with the appropriate args (or even std::function). 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 doing that with "small" objects, but I can't remember it. Line 121: // Handle* h = cache_->Insert(h, deleter); Nit: the rhs here should reference ph, not h. Also it's not really a "deleter" anymore, but an "evicter", right? Line 145: virtual uint8_t* MutableValue(PendingHandle* handle) = 0; Why is this a Cache method and not a PendingHandle one? Seems more intuitive to model it as the latter. 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. -- 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
