Remove some allocations related to storing CacheEntry scopes

[adamsitnik]

While looking at the source code to address #45436 I've realized that the _previous field of the CacheEntryStack type is never used:

runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs

Lines 8 to 38 in 4487088

	internal class CacheEntryStack
	{
	private readonly CacheEntryStack _previous;
	private readonly CacheEntry _entry;
	private CacheEntryStack()
	{
	}
	private CacheEntryStack(CacheEntryStack previous, CacheEntry entry)
	{
	if (previous == null)
	{
	throw new ArgumentNullException(nameof(previous));
	}
	_previous = previous;
	_entry = entry;
	}
	public static CacheEntryStack Empty { get; } = new CacheEntryStack();
	public CacheEntryStack Push(CacheEntry c)
	{
	return new CacheEntryStack(this, c);
	}
	public CacheEntry Peek()
	{
	return _entry;
	}

This led me to the conclusion, that there is no need to have this type if all it does is just being a wrapper around a single CacheEntry instance. So instead of AsyncLocal<CacheEntryStack> we can have AsyncLocal<CacheEntry>

The ScopeLease responsibility was to just reset the Current during Dispose. To save some allocation we can just "inline" this type and do what it did in CacheEntry.Dispose:

runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs

Lines 59 to 62 in 4487088

	public void Dispose()
	{
	Scopes = _cacheEntryStack;
	}

Doing this a little bit later than before allowed to avoid expensive access to CacheEntryHelper.Current in CacheEntry.Dispose

Benchmark numbers:

Method	Toolchain	Mean	Ratio	Allocated
AddThenRemove_NoExpiration	\after\CoreRun.exe	59.09 us	0.94	57 KB
AddThenRemove_NoExpiration	\before\CoreRun.exe	63.14 us	1.00	71 KB
AddThenRemove_AbsoluteExpiration	\after\CoreRun.exe	54.01 us	0.80	45 KB
AddThenRemove_AbsoluteExpiration	\before\CoreRun.exe	67.33 us	1.00	71 KB
AddThenRemove_RelativeExpiration	\after\CoreRun.exe	60.80 us	0.89	57 KB
AddThenRemove_RelativeExpiration	\before\CoreRun.exe	68.03 us	1.00	71 KB
AddThenRemove_SlidingExpiration	\after\CoreRun.exe	58.53 us	0.88	60 KB
AddThenRemove_SlidingExpiration	\before\CoreRun.exe	66.32 us	1.00	72 KB

Contributes to #45436

[ghost]

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

---

While looking at the source code to address #45436 I've realized that the _previous field of the CacheEntryStack type is never used:

runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryStack.cs

Lines 8 to 38 in 4487088

	internal class CacheEntryStack
	{
	private readonly CacheEntryStack _previous;
	private readonly CacheEntry _entry;
	private CacheEntryStack()
	{
	}
	private CacheEntryStack(CacheEntryStack previous, CacheEntry entry)
	{
	if (previous == null)
	{
	throw new ArgumentNullException(nameof(previous));
	}
	_previous = previous;
	_entry = entry;
	}
	public static CacheEntryStack Empty { get; } = new CacheEntryStack();
	public CacheEntryStack Push(CacheEntry c)
	{
	return new CacheEntryStack(this, c);
	}
	public CacheEntry Peek()
	{
	return _entry;
	}

This led me to the conclusion, that there is no need to have this type if all it does is just being a wrapper around a single CacheEntry instance. So instead of AsyncLocal<CacheEntryStack> we can have AsyncLocal<CacheEntry>

The ScopeLease responsibility was to just reset the Current during Dispose. To save some allocation we can just "inline" this type and do what it did in CacheEntry.Dispose:

runtime/src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs

Lines 59 to 62 in 4487088

	public void Dispose()
	{
	Scopes = _cacheEntryStack;
	}

Doing this a little bit later than before allowed to avoid expensive access to CacheEntryHelper.Current in CacheEntry.Dispose

Benchmark numbers:

Method	Toolchain	Mean	Ratio	Allocated
AddThenRemove_NoExpiration	\after\CoreRun.exe	59.09 us	0.94	57 KB
AddThenRemove_NoExpiration	\before\CoreRun.exe	63.14 us	1.00	71 KB
AddThenRemove_AbsoluteExpiration	\after\CoreRun.exe	54.01 us	0.80	45 KB
AddThenRemove_AbsoluteExpiration	\before\CoreRun.exe	67.33 us	1.00	71 KB
AddThenRemove_RelativeExpiration	\after\CoreRun.exe	60.80 us	0.89	57 KB
AddThenRemove_RelativeExpiration	\before\CoreRun.exe	68.03 us	1.00	71 KB
AddThenRemove_SlidingExpiration	\after\CoreRun.exe	58.53 us	0.88	60 KB
AddThenRemove_SlidingExpiration	\before\CoreRun.exe	66.32 us	1.00	72 KB

Contributes to #45436

Author:	adamsitnik
Assignees:	-
Labels:	area-Extensions-Caching, tenet-performance
Milestone:	6.0.0

[Tratcher]

This is going to require a very careful review, AsyncLocal's have a lot of surprising behaviors that often require special handling like this.

[eerhardt]

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.

[adamsitnik]

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.

[eerhardt]

LGTM. (I thought I had already approved this)

[adamsitnik]

@Tratcher could you please review it?

[Tratcher]

I've asked @davidfowl to review this one instead.

[adamsitnik]

I want to send one more PR that would conflict with this one. Since I have two approvals and @davidfowl seems to be busy, I am going to merge it now.

[davidfowl]

I had a comment but I never sent it. The use of async locals outside of async method is suspect but isn't new. The ordering change also concerns me but I haven't spend enough time coming up with hypothetical scenarios it might affect. I'd look to see what user code runs before and after our set/unset to see if there's any change more things are being captured now.

[adamsitnik]

The ordering change also concerns me but I haven't spend enough time coming up with hypothetical scenarios it might affect.

@davidfowl thank you for your feedback, I've sent #45964 to address it.