Mv filecache tuning2 #79

Merged
merged 9 commits into from May 24, 2013

Projects

None yet

3 participants

@matthewvon
Collaborator

Tuning to file/table cache to improve random read and iterator performance. See https://github.com/basho/leveldb/wiki/Mv-filecache-tuning2

About 50% improvement in 2i_slf_stressmix test, with 5x reduction in timeouts from long iterations.

@matthewvon matthewvon was assigned May 17, 2013
@chardan

I'll assume that it's okay to take no action if the reinterpret_cast<> fails (silently ignore unknown cache type being sent to the LRU cache).

Collaborator

I assume so too. The reinterpret_cast<> idiom was copied from another place in the code. See the "Unref(reinterpret_cast<LRUHandle*>(handle));" line just above. Google seems to use Cache::Handle * as an opaque pointer to hide the implementation of the cache. Google has no mechanism to validate the pointer is correct.

Collaborator

i.e. it is not "ok", it just "is".

@chardan

...and here, we don't check the result. (I'm betting we don't need to above either.) I suspect there will be no issue, but will point out that the next line will explode if h is nullptr.

Collaborator

Yeah, Google's code does not appear to be very defensive. See the Release(Handle * handle) function just below. Same weakness.

@evanmcc evanmcc and 1 other commented on an outdated diff May 21, 2013
include/leveldb/cache.h
@@ -90,6 +90,11 @@ class Cache {
// Allows more accurate tracking of "charge" against each cache item.
virtual size_t EntryOverheadSize() {return(0);};
+ // Riak specific: Add a reference to cache object to help hold it
+ // hold it in memory
@evanmcc
evanmcc May 21, 2013

comment nitpick: 'hold it' is repeated.

@matthewvon
matthewvon May 21, 2013 collaborator

corrected and checked in.

@evanmcc
evanmcc commented May 22, 2013

Jessie and I have been looking at some breakages in the test suite. There's a bug in the repair stuff because evict now takes two arguments, but regardless of what you set it to, it seems to introduce a non-deterministic double-release problem. It's possible to not explode when you do the second release, but the fact that it's started to happen now is worrying.

Also, master does not pass all of its tests, but haven't looked into that.

Jessie's take:
"Two issues: 1) tool needs patching because of API change; 2) this bug (workaround possible, but concern is that because of the way the assertions are used it might be The Wrong Way To Do It(TM))."

Jesse Willia... and others added some commits May 22, 2013
@chardan
chardan commented May 23, 2013

+1 (Matthew notes known issues in db_test and corruption_test that he will address later).

@matthewvon matthewvon merged commit df01dae into master May 24, 2013
@matthewvon matthewvon deleted the mv-filecache-tuning2 branch May 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment