Remove some allocations related to storing CacheEntry scopes by adamsitnik · Pull Request #45563 · dotnet/runtime

adamsitnik · 2020-12-03T19:29:38Z

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

…ncLocal<CacheEntry>

… null

ghost · 2020-12-03T19:29:41Z

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 · 2020-12-03T19:54:13Z

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 · 2020-12-04T16:24:57Z

src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs

                    }
                }
+
+                CacheEntryHelper.ExitScope(this, _previous);


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.

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.

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

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

eerhardt

LGTM. (I thought I had already approved this)

adamsitnik · 2020-12-08T16:50:51Z

@Tratcher could you please review it?

Tratcher · 2020-12-08T17:51:57Z

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

adamsitnik · 2020-12-09T12:38:57Z

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 · 2020-12-09T12:46:54Z

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 · 2020-12-11T14:49:40Z

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.

adamsitnik added 3 commits December 3, 2020 19:23

CacheEntryStack._previous is never used so it can be removed

df8d001

remove CacheEntryStack and ScopeLease as it's enough to have just Asy…

8789cb5

…ncLocal<CacheEntry>

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

c4c1944

… null

adamsitnik added tenet-performance Performance related issue area-Extensions-Caching labels Dec 3, 2020

adamsitnik added this to the 6.0.0 milestone Dec 3, 2020

adamsitnik requested review from Tratcher, eerhardt and maryamariyan December 3, 2020 19:29

Tratcher requested a review from davidfowl December 3, 2020 19:51

adamsitnik mentioned this pull request Dec 4, 2020

AsyncLocal Memory Usage in MemoryCache #45436

Closed

eerhardt reviewed Dec 4, 2020

View reviewed changes

src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntryHelper.cs Outdated Show resolved Hide resolved

Update src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEnt…

e6fd38f

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

eerhardt approved these changes Dec 8, 2020

View reviewed changes

maryamariyan approved these changes Dec 8, 2020

View reviewed changes

Tratcher removed their request for review December 8, 2020 17:52

adamsitnik merged commit 50c2de9 into dotnet:master Dec 9, 2020

adamsitnik deleted the memoryCacheCheaperScopes branch December 9, 2020 12:39

This was referenced Dec 9, 2020

[Proposal] use Timer in MemoryCache #45842

Closed

revert ordering change to address PR feedback #45964

Merged

ghost locked as resolved and limited conversation to collaborators Jan 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove some allocations related to storing CacheEntry scopes#45563

Remove some allocations related to storing CacheEntry scopes#45563
adamsitnik merged 4 commits intodotnet:masterfrom
adamsitnik:memoryCacheCheaperScopes

adamsitnik commented Dec 3, 2020

Uh oh!

ghost commented Dec 3, 2020

Uh oh!

Tratcher commented Dec 3, 2020

Uh oh!

eerhardt Dec 4, 2020

Uh oh!

adamsitnik Dec 7, 2020

Uh oh!

Uh oh!

eerhardt left a comment

Uh oh!

adamsitnik commented Dec 8, 2020

Uh oh!

Tratcher commented Dec 8, 2020

Uh oh!

adamsitnik commented Dec 9, 2020

Uh oh!

davidfowl commented Dec 9, 2020

Uh oh!

adamsitnik commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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;
	}

Conversation

adamsitnik commented Dec 3, 2020

Uh oh!

ghost commented Dec 3, 2020

Uh oh!

Tratcher commented Dec 3, 2020

Uh oh!

eerhardt Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

adamsitnik Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eerhardt left a comment

Choose a reason for hiding this comment

Uh oh!

adamsitnik commented Dec 8, 2020

Uh oh!

Tratcher commented Dec 8, 2020

Uh oh!

adamsitnik commented Dec 9, 2020

Uh oh!

davidfowl commented Dec 9, 2020

Uh oh!

adamsitnik commented Dec 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants