Skip to content

Remove some allocations related to storing CacheEntry scopes #45563

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.

Sign up for GitHub

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

Jump to bottom
Merged
adamsitnik merged 4 commits into from
Dec 9, 2020
Merged

Remove some allocations related to storing CacheEntry scopes #45563

adamsitnik merged 4 commits into from
Dec 9, 2020

Conversation

@adamsitnik
Copy link
Member

@adamsitnik adamsitnik commented Dec 3, 2020

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

@adamsitnik adamsitnik added this to the 6.0.0 milestone Dec 3, 2020
@ghost
Copy link

ghost commented Dec 3, 2020

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 Tratcher requested a review from davidfowl December 3, 2020 19:51
@Tratcher
Copy link
Member

Tratcher commented Dec 3, 2020

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

@adamsitnik adamsitnik mentioned this pull request Dec 4, 2020
Closed
eerhardt
eerhardt reviewed Dec 4, 2020
View reviewed changes
src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
}
}

CacheEntryHelper.ExitScope(this, _previous);
Copy link
Member

@eerhardt eerhardt Dec 4, 2020

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

@adamsitnik adamsitnik Dec 7, 2020

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.

eerhardt
eerhardt approved these changes Dec 8, 2020
View reviewed changes
Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. (I thought I had already approved this)

@adamsitnik
Copy link
Member Author

adamsitnik commented Dec 8, 2020

@Tratcher could you please review it?

@Tratcher
Copy link
Member

Tratcher commented Dec 8, 2020

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

@Tratcher Tratcher removed their request for review December 8, 2020 17:52
@adamsitnik
Copy link
Member Author

adamsitnik commented Dec 9, 2020

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.

@adamsitnik adamsitnik merged commit 50c2de9 into dotnet:master Dec 9, 2020
@adamsitnik adamsitnik deleted the memoryCacheCheaperScopes branch December 9, 2020 12:39
@davidfowl
Copy link
Member

davidfowl commented Dec 9, 2020

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.

This was referenced Dec 9, 2020
Closed
Merged
@adamsitnik
Copy link
Member Author

adamsitnik commented Dec 11, 2020

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.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@eerhardt eerhardt eerhardt approved these changes

@davidfowl davidfowl Awaiting requested review from davidfowl

+1 more reviewer

@maryamariyan maryamariyan maryamariyan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-Extensions-Caching tenet-performance Performance related issue

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@adamsitnik @Tratcher @davidfowl @maryamariyan @eerhardt