-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
few minor MemoryCache perf improvements #44797
few minor MemoryCache perf improvements #44797
Conversation
Tagging subscribers to this area: @eerhardt, @maryamariyan Issue Details
|
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Called by multiple actions to see how long it's been since we last checked for expired items. | ||
// If sufficient time has elapsed then a scan is initiated on a background task. | ||
private void StartScanForExpiredItems(DateTimeOffset? utcNow = null) | ||
[MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't inlined otherwise? Also, ScheduleTask looks pretty small; it's not inlined automatically (which would defeat your goal here)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
before my changes, the StartScanForExpiredItems
was not getting inlined:
now StartScanForExpiredItemsIfNeeded
does get inlined an ScheduleTask
does not:
I was simply not sure if _options.ExpirationScanFrequency < utcNow - _lastExpirationScan
won't get translated to too much IL and just to be sure I've applied the attribute
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was simply not sure if _options.ExpirationScanFrequency < utcNow - _lastExpirationScan won't get translated to too much IL and just to be sure I've applied the attribute
Ok, but the same argument goes for whether ScheduleTask may get inlined automatically even though you don't want it to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the same argument goes for whether ScheduleTask may get inlined automatically even though you don't want it to
It's quite big (from the IL perspective) and I would not expect it to get inlined. Do you want me to ensure it by using an attribute?
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Toub <stoub@microsoft.com>
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Outdated
Show resolved
Hide resolved
Perf lab data showing improvements: |
Related improvement link: DrewScoggins/performance-2#3516 |
I just took a quick look at the source code to see if we could get some gains in the Caching TechEmpower benchmark. This PR is a free lunch, there is definitely more low hanging fruits there and I hope that it becomes one of our .NET 6 goals.
The performance repo microbenchmarks (dotnet/performance#1598) show 5 to 15% perf improvement
The TechEmpower benchmark shows +6k RPS (from 234,210 240,778) to which translates to +2.75% RPS