search

intel / CacheLib

Pluggable in-process caching engine to build and scale high performance services
https://www.cachelib.org
Apache License 2.0
17 stars 4 forks source link

fix race in promotion #71

Closed byrnedj closed 1 year ago

byrnedj commented 1 year ago

releaseBackToAllocator was being called before wakeUpWaiters - allowing for the condition where the candidate's key could be changed (since it may have been reallocated already).

I also edited the promoter to remove from memory container when marked moving (as is done in eviction) since we are still under the memory container lock.

The reason is that promotion (like background eviction) rarely fails and the performance improvement in terms of number of objects promoted increases significantly (30%-40%) in my test cases since we spend less time contending for memory container locks.

This change is Reviewable

vinser52 commented 1 year ago

Is it possible to add any tests? I understand it might be non-trivial, just asking.

byrnedj commented 1 year ago

Is it possible to add any tests? I understand it might be non-trivial, just asking.

It would be possible - I think it would have to be based on gdb like in previous race condition. However - this was an implementation error as wakeUpWaiters should always be called before we release a candidate if it had been marked moving.

I think it would be good to add a todo for more background worker tests - but I think we should merge this so that promotion no longer causes benchmarks to fail.