Skip to content

Commit

Permalink
Replace Values with KeyValuePair enumerator in MemoryCache (#53762)
Browse files Browse the repository at this point in the history
fixes #53761
  • Loading branch information
ezgambac authored Jun 10, 2021
1 parent 8538436 commit 00ee1c1
Showing 1 changed file with 6 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -316,8 +316,11 @@ void ScheduleTask(DateTimeOffset utcNow)
private static void ScanForExpiredItems(MemoryCache cache)
{
DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow;
foreach (CacheEntry entry in cache._entries.Values)

foreach (KeyValuePair<object, CacheEntry> item in cache._entries)
{
CacheEntry entry = item.Value;

if (entry.CheckExpired(now))
{
cache.RemoveEntry(entry);
Expand Down Expand Up @@ -399,8 +402,9 @@ private void Compact(long removalSizeTarget, Func<CacheEntry, long> computeEntry

// Sort items by expired & priority status
DateTimeOffset now = _options.Clock.UtcNow;
foreach (CacheEntry entry in _entries.Values)
foreach (KeyValuePair<object, CacheEntry> item in _entries)
{
CacheEntry entry = item.Value;
if (entry.CheckExpired(now))
{
entriesToRemove.Add(entry);
Expand Down

4 comments on commit 00ee1c1

@zyo2012
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezgambac I'm curious to know the motivation behind this change? Is this a sort of optimization or safetly? Both previous and new code do the same...

@ezgambac
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zyo2012 Iterating over .Values in a concurrent dictionary has 2 issues, it acquires a lock over the whole dictionary, hence blocking new writes, and it copies the .Values to a new list, so a new memory allocation.
Doing it using the iterator over the whole dictionary prevents both issues, as no global lock is acquired, and no new memory is allocated.

@zyo2012
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ezgambac thanks for the explanation and quick response. Please confirm: So during the iteration of _entries NEW can be added (in a separate thread)?

@ezgambac
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaik, yes

Please sign in to comment.