Add option to MemoryCacheOptions to specify maximum cache entries count #327
Conversation
| // Compact by 10 percent if we exceed the given maximum number of cache entries | ||
| if (_entries.Count > _maximumEntriesCount) | ||
| { | ||
| Compact(0.10); |
There was a problem hiding this comment.
Do this in the background and handle concurrent adds.
| @@ -148,6 +150,12 @@ private void SetEntry(CacheEntry entry) | |||
| if (entryAdded) | |||
| { | |||
| entry.AttachTokens(); | |||
|
|
|||
| // Compact by 10 percent if we exceed the given maximum number of cache entries | |||
| if (_entries.Count > _maximumEntriesCount) | |||
There was a problem hiding this comment.
I assume this always resolves to false for a nullable?
There was a problem hiding this comment.
when it's null yes. https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/nullable-types/using-nullable-types see Operators section
| @@ -13,6 +13,8 @@ public class MemoryCacheOptions : IOptions<MemoryCacheOptions> | |||
|
|
|||
| public TimeSpan ExpirationScanFrequency { get; set; } = TimeSpan.FromMinutes(1); | |||
|
|
|||
| public int? EntryCountLimit { get; set; } | |||
JunTaoLuo
force-pushed
the
johluo/max-entry-count
branch
from
1d1622d to
ef54a8c
Compare
| { | ||
| // Keep compacting until remaining entries are at 90% of maximum | ||
| // Stop compacting if no entries were removed, for example if all the remaining entries are pinned with NeverRemove priority | ||
| while (Compact(1 - ((0.9 * _entryCountLimit.Value) / _entries.Count)) && _entries.Count > _entryCountLimit.Value) { } |
There was a problem hiding this comment.
This was triggered by already reaching the max, so why not just ask to compact 10%?
No need for the loop, if it fills up again it will trigger compaction again.
There was a problem hiding this comment.
Stress testing it was failing. For example the test was using two threads to keep adding entries and the removal was not able to keep up. By the time 10% were trimmed, more than 10% were added. So instead, I'm trying to trim until 90% is left and keep doing this until 90% of max is achieved. Hmm some concurrency test are hard to write...
There was a problem hiding this comment.
yikes, this loop may never exit and your cache could still keep growing beyond the limit.
plan b: if we're at the limit when you try to add, discard the new item and invoke compaction. This should prevent you from almost ever going over the limit, let alone running to catch up. Make sure to run the normal callbacks for the new item.
There was a problem hiding this comment.
In that case, we are no longer trying to remove the least recently used entries. But I suppose that is still better than exceeding the memory limit.
There was a problem hiding this comment.
The compaction will try to remove the least recently used to make room for the next guy, but in the meantime add must fail or block, and silent fail is better for a cache.
There was a problem hiding this comment.
Trying this now. This gives the extra benefit that the signature of Compact doesn't need to change.
There was a problem hiding this comment.
Seems to work but the behaviour is very strange. When the capacity is hit, the newest item and the oldest items(s) are evicted, whereas I would expect the newest one should be added and only the oldest items are removed. I'll see if there are better alternatives.
| @@ -104,6 +105,9 @@ public void CompactWhenMaximumEntriesCountExceeded() | |||
|
|
|||
| cache.Set("key9", "value9"); | |||
|
|
|||
| // Wait 1 second for compaction to complete | |||
| Thread.Sleep(TimeSpan.FromSeconds(1)); | |||
There was a problem hiding this comment.
register an eviction callback?
|
Closed in favour of #332 |
Addresses #325