Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some allocations related to storing CacheEntry scopes #45563

Merged
merged 4 commits into from
Dec 9, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
Copy link
Member

Choose a reason for hiding this comment

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

Is there any way changing the ordering between popping the "scope" off the stack and calling _cache.SetEntry could break anything? Before the scope was removed before calling _cache.SetEntry. Now it happens after.

Copy link
Member Author

Choose a reason for hiding this comment

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

So far, when Dispose was called, we were first setting the CacheEntryHelper.Current to previous and then accessing CacheEntryHelper.Current again to get it. With my change, we take advantage of knowing "previous" and resetting it after using its value (one access to async local instead of two).

I don't think that this reordering can break anything.

_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
{
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