Lower contention LruCache #6875
Conversation
|
Not sure about the design, wouldn't concurrent dictionary be better in terms of Set's? |
There was a problem hiding this comment.
I read through the PR and provided some comments.
As a general one, this PR makes the cleaning separate and results in potentially occasionally bigger dictionaries. This separation makes me wonder, whether we could drop the linkage of LinkedListNode altogether and
- create
Node<T>that would have aT Valueandlong Epoch. - on operation, you
var epoch = Interlocked.Increment(ref _counter)and set theEpochin the item - you enqueue
(node, epoch) - during cleanup, you pull enqueue items, and check
tuple.node.Epoch == tuple.epoch, if right and the map is to big you remove the thing. Otherwise you discard because something else touched this.
No allocations for LinkedListItem at all, as _accesses store now value tuples, in a similar way as _cacheMap. Yes, I'm aware that this does not come for free as the _accessess segments will have bigger items. Still, no obj header for the nodes I think.
This would make is similar to ConcurrentQueue a bit 😅 using the epoch thing.
WDYT @benaadams ?
| { | ||
| bool didGet; | ||
| LinkedListNode<LruCacheItem>? node; | ||
| using (var handle = _lock.AcquireWrite()) |
There was a problem hiding this comment.
AcquireWrite? Is it intentional?
| ref LinkedListNode<LruCacheItem>? node = ref Unsafe.NullRef<LinkedListNode<LruCacheItem>?>(); | ||
| using (var handle = _lock.AcquireWrite()) | ||
| { | ||
| node = ref CollectionsMarshal.GetValueRefOrAddDefault(_cacheMap, key, out _); |
There was a problem hiding this comment.
This is a ref to an array. If another thread resizes the array, this ref will point to an array no longer in use. Am I missing something or is it a bug?
| { | ||
| while (!ct.IsCancellationRequested && _accesses.TryDequeue(out LinkedListNode<LruCacheItem>? node)) | ||
| { | ||
| bool exists = _cacheMap.ContainsKey(node.Value.Key); |
There was a problem hiding this comment.
Can be broken as _cacheMap might be written to concurrently?
| } | ||
|
|
||
| bool exists = false; // Captured by lambda | ||
| LinkedListNode<LruCacheItem> node = _cacheMap.AddOrUpdate( |
There was a problem hiding this comment.
Do we always allocate an item on non-existence of the key here and not reuse the recent as thsi PR separates this two? Just asking, can be by design.
| break; | ||
| } | ||
|
|
||
| _cacheMap.Remove(leastRecentlyUsed.Value.Key, out leastRecentlyUsed); |
There was a problem hiding this comment.
I see that .Remove is called in two places only, one on Delete and second in here.
| { | ||
| using NonBlocking; | ||
|
|
||
| public sealed class NonBlockingLruCache<TKey, TValue> : IThreadPoolWorkItem, ICache<TKey, TValue> where TKey : notnull |
There was a problem hiding this comment.
I see how you split implementations now. Why not to hide them behind LruCache generic class and return one depending on the Unsafe.SizeOf<TKey> ?
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?