Sarah Jelinek has posted comments on this change. Change subject: Add implementation of the block cache on NVM running in persistent mode. ......................................................................
Patch Set 6: (31 comments) http://gerrit.cloudera.org:8080/#/c/1347/6/src/kudu/cfile/block_cache.h File src/kudu/cfile/block_cache.h: Line 77: bool Lookup(const Slice& key, Cache::CacheBehavior behavior, > since you've moved the CacheKey struct to be a part of the public interface Looks like you already made this change. Line 87: bool Insert(void* key, size_t key_size, Slice &block_data, > this API also now looks pretty strange, since we expect the key is supposed You changed this. Line 111: void Free(const Slice& key, void* p); > odd that you have to pass a key here, when Allocate doesn't either take or You fixed this. http://gerrit.cloudera.org:8080/#/c/1347/6/src/kudu/util/nvm_cache.cc File src/kudu/util/nvm_cache.cc: Line 91: struct KeyVal { > add comment explaining that this is a variable-length structure which store Done Line 94: uint64_t key_size; > given that this is persistent memory, we should try to be space-efficient, I could do that. I have changed these values to uint32_t. The kv_size allows me to determine the charge for the memory I am allocating. I think it's easier to do it my way but if it's a big deal I will change it. Line 104: void KvConstructor(PMEMobjpool* pop, void* ptr, void* arg) { > should be in an anonymous namespace (because this symbol isn't used outside Done Line 110: pmemobj_persist(pop, &kv->valid, sizeof(kv->valid)); > I spent some time reading the libpmemobj source, and it wasn't clear whethe I modified this to do: // Persist only what's necessary to reduce the amount of flush and commit size. // The key_len, value_len and kv_data have not been populated at this point. pmemobj_persist(pop, kv, sizeof(kv->valid) + sizeof(kv->kv_size)); Since kv->valid and kv->kv_size are next to each other I can persist that range of data with the least amount of penalty. Flushing the whole data structure results in more flushes and drains that are necessary. I would be flushing fields key_len, value_len and kv_data before they were valid. Since kv->valid and kv->kv_size are Line 113: // Finds the persistent memory object id for the give ptr and offset. > typo: 'given' Done Line 115: static PMEMoid FindOid(const uint8_t* ptr, size_t offset, PMEMobjpool* pop, > - 'offset' here is always offsetof(KeyValue, key_value), so shouldn't be a At this point is is always offsetof(KeyValue, kv_data) however I want this to be more generic. Line 120: uintptr_t kv_ptr = (uintptr_t)(ptr - offset); > - a DCHECK_GT(ptr, offset) is probably useful here Done. Line 122: newoid.pool_uuid_lo = root.pool_uuid_lo; > here you're using internals of the PMEMoid type. Is there no API in libpmem I am using the internals :-). There is no API in libpmemobj to do this. It isn't expected that this is needed when doing the non-transactional memory allocation. AFAIK there are no other consumers of libpmemobj that allocate pmem, populate it later and then have to find the OID without the original object. I am not sure it's a huge requirement. I will file a feature request. Line 129: POBJ_LAYOUT_BEGIN(kudublockcache); > style: kudu_block_cache (we try to avoid runningwordstogether) This isn't the style of the libpmemobj interfaces. I would prefer to leave it as it is. Line 163: uint8_t* key_ptr; // Only used in persistent mode. > seems potentially redundant - in persistent mode, aren't the key and value I changed this based: uint8_t kv_data[]; I can certainly add diagrams to the top of this file. Line 373: FreeEntry(e, true); > nit: indentation of above four lines is off Done Line 398: ScopedLeakCheckDisabler disabler; > I tried removing the ScopedLeakCheckDisabler and I dont see any leaks. Stil Done. Don't need it. Line 405: struct KeyVal *tmp = static_cast<struct KeyVal*>(pmemobj_direct(oid)); > - under what circumstances would pmemobj_direct fail on a just-allocated pm It should never happen. Added CHECK. Fixed use of 'struct'. Line 429: e->deleter->Delete(e->key(), e->value); > nit: indented too much Done Line 453: void (*constructor)(PMEMobjpool*, void*, void*)) { > use a typedef for this function pointer type Done Line 584: FreeEntry(e, false); > Not following this. If the last reference is held by a user, this means it' What is the difference between Release and Erase? When the ShardedLRUCache deconstructor is called which of these methods is called? Line 642: pmemobj_persist(pop_, &kv->key_value, kv->value_size); > instead of separately persisting each subfield, probably more efficient to Done Line 644: pmemobj_persist(pop_, &kv->valid, sizeof(kv->valid)); > Here we only care about an ordering property that all of the KeyValue field It is stricter than necessary. I have no problem changing this. What I changed it to is: pmemobj_persist(pop_, &kv->key_len, sizeof(kv->key_len) + sizeof(kv->value_len) + sizeof(kv>kv_data)); kv->valid = 1; pmemobj_persist(pop_, &kv->valid, sizeof(kv->valid)); This gives us pmem_flush, pmem_drain for each. What I would like to do is run this through an internal perf checker we have to see if I can reconfigure the code to get better performance. Going to leave it as I described above for now. Line 683: FreeEntry(e, false); > also doesn't seem right - if you explicitly erase something from the cache, Clearly I don't understand the the difference between Erase and Release. Line 776: for (int i = 0; i < num; i++) { > this loop looks like a lot of repeated code from above... why can't we just I changed this to be one loop through. See updated code. Line 792: // Volatile mode > this is mostly repeated code from up above, can't it be moved out of the co Fixed. Line 914: PLOG_IF(FATAL, pop == NULL) > you dont need the PLOG_IF since you just checked the same condition above I don't remember adding PLOG_IF :-). I will remove the if (pop == NULL), etc. Line 916: << FLAGS_nvm_cache_path.c_str(); > no need to call c_str() (you can concat the std::string) Done Line 926: << FLAGS_nvm_cache_path.c_str(); > same Done Line 943: << FLAGS_nvm_cache_path.c_str(); > same Done http://gerrit.cloudera.org:8080/#/c/1347/6/src/kudu/util/nvm_cache.h File src/kudu/util/nvm_cache.h: Line 24: #include <libvmem.h> > shouldn't need to include any of these in the header (since users dont need Removed. http://gerrit.cloudera.org:8080/#/c/1347/6/thirdparty/build-definitions.sh File thirdparty/build-definitions.sh: Line 312: for LIB in libvmem libpmem libpmemobj; do > nit: tab Fixed. Line 313: EXTRA_CFLAGS="$DEBUG_CFLAGS" make -j$PARALLEL $LIB DEBUG=0 > should say EXTRA_CFLAGS=$EXTRA_CFLAGS Done -- To view, visit http://gerrit.cloudera.org:8080/1347 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2dcf474bce9f2375b0ddde38312db7d82a81835c Gerrit-PatchSet: 6 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Anonymous Coward #90 Gerrit-Reviewer: Internal Jenkins Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Sarah Jelinek <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
