Skip to content

Commit

Permalink
Remove some allocations related to storing CacheEntry scopes (#45563)
Browse files Browse the repository at this point in the history
* CacheEntryStack._previous is never used so it can be removed

* remove CacheEntryStack and ScopeLease as it's enough to have just AsyncLocal<CacheEntry>

* the first Entry created has no previous entry, so the field is set to null

* Update src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
  • Loading branch information
adamsitnik and eerhardt authored Dec 9, 2020
1 parent d21fe17 commit 50c2de9
Show file tree
Hide file tree
Showing 4 changed files with 30 additions and 99 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ internal class CacheEntry : ICacheEntry
private TimeSpan? _absoluteExpirationRelativeToNow;
private TimeSpan? _slidingExpiration;
private long? _size;
private IDisposable _scope;
private CacheEntry _previous; // this field is not null only before the entry is added to the cache
private object _value;
private int _state; // actually a [Flag] enum called "State"

internal CacheEntry(object key, MemoryCache memoryCache)
{
Key = key ?? throw new ArgumentNullException(nameof(key));
_cache = memoryCache ?? throw new ArgumentNullException(nameof(memoryCache));
_scope = CacheEntryHelper.EnterScope(this);
_previous = CacheEntryHelper.EnterScope(this);
}

/// <summary>
Expand Down Expand Up @@ -145,23 +145,21 @@ public void Dispose()
{
IsDisposed = true;

// Ensure the _scope reference is cleared because it can reference other CacheEntry instances.
// This CacheEntry is going to be put into a MemoryCache, and we don't want to root unnecessary objects.
_scope.Dispose();
_scope = null;

// Don't commit or propagate options if the CacheEntry Value was never set.
// We assume an exception occurred causing the caller to not set the Value successfully,
// so don't use this entry.
if (IsValueSet)
{
_cache.SetEntry(this);

if (CanPropagateOptions())
if (_previous != null && CanPropagateOptions())
{
PropagateOptions(CacheEntryHelper.Current);
PropagateOptions(_previous);
}
}

CacheEntryHelper.ExitScope(this, _previous);
_previous = null; // we don't want to root unnecessary objects
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,65 +1,32 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Diagnostics;
using System.Threading;

namespace Microsoft.Extensions.Caching.Memory
{
internal class CacheEntryHelper
internal static class CacheEntryHelper
{
private static readonly AsyncLocal<CacheEntryStack> _scopes = new AsyncLocal<CacheEntryStack>();

internal static CacheEntryStack Scopes
{
get { return _scopes.Value; }
set { _scopes.Value = value; }
}
private static readonly AsyncLocal<CacheEntry> _current = new AsyncLocal<CacheEntry>();

internal static CacheEntry Current
{
get
{
CacheEntryStack scopes = GetOrCreateScopes();
return scopes.Peek();
}
get => _current.Value;
private set => _current.Value = value;
}

internal static IDisposable EnterScope(CacheEntry entry)
internal static CacheEntry EnterScope(CacheEntry current)
{
CacheEntryStack scopes = GetOrCreateScopes();

var scopeLease = new ScopeLease(scopes);
Scopes = scopes.Push(entry);

return scopeLease;
CacheEntry previous = Current;
Current = current;
return previous;
}

private static CacheEntryStack GetOrCreateScopes()
internal static void ExitScope(CacheEntry current, CacheEntry previous)
{
CacheEntryStack scopes = Scopes;
if (scopes == null)
{
scopes = CacheEntryStack.Empty;
Scopes = scopes;
}

return scopes;
}

private sealed class ScopeLease : IDisposable
{
private readonly CacheEntryStack _cacheEntryStack;

public ScopeLease(CacheEntryStack cacheEntryStack)
{
_cacheEntryStack = cacheEntryStack;
}

public void Dispose()
{
Scopes = _cacheEntryStack;
}
Debug.Assert(Current == current, "Entries disposed in invalid order");
Current = previous;
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -214,17 +214,23 @@ public void DisposingCacheEntryReleasesScope()
object GetScope(ICacheEntry entry)
{
return entry.GetType()
.GetField("_scope", BindingFlags.NonPublic | BindingFlags.Instance)
.GetField("_previous", BindingFlags.NonPublic | BindingFlags.Instance)
.GetValue(entry);
}

var cache = CreateCache();

ICacheEntry entry = cache.CreateEntry("myKey");
Assert.NotNull(GetScope(entry));
ICacheEntry first = cache.CreateEntry("myKey1");
Assert.Null(GetScope(first)); // it's the first entry, so it has no previous cache entry set

entry.Dispose();
Assert.Null(GetScope(entry));
ICacheEntry second = cache.CreateEntry("myKey2");
Assert.NotNull(GetScope(second)); // it's not first, so it has previous set
Assert.Same(first, GetScope(second)); // second.previous is set to first

second.Dispose();
Assert.Null(GetScope(second));
first.Dispose();
Assert.Null(GetScope(first));
}

[Fact]
Expand Down

0 comments on commit 50c2de9

Please sign in to comment.