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

More MemoryCache perf improvements #45280

Merged

adamsitnik merged 5 commits into dotnet:master from adamsitnik:anotherMemoryCachePerfImprovement

Dec 1, 2020

Member

adamsitnik commented Nov 27, 2020 •

edited

Loading

don't acces expensive CacheEntryHelper.Current if options can't be propagated anyway

CacheEntryHelper.Current uses expensive AsyncLocal. Before accessing it, we can just check if the options can be propagated and don't use it at all if not.

+7.5% (+18k) RPS for TechEmpower benchmark!

load	before	after
CPU Usage (%)	19	21
Cores usage (%)	528	574
Working Set (MB)	48	48
Build Time (ms)	4,500	4,541
Start Time (ms)	0	0
Published Size (KB)	76,401	76,401
.NET Core SDK Version	3.1.404	3.1.404
First Request (ms)	76	78
Requests/sec	234,029	252,468
Requests	3,533,646	3,812,090
Mean latency (ms)	1.28	1.18
Max latency (ms)	61.30	47.33
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	736.49	794.52
Latency 50th (ms)	1.06	0.99
Latency 75th (ms)	1.17	1.09
Latency 90th (ms)	1.33	1.23
Latency 99th (ms)	10.08	9.10

inline the expiration check hot paths

load	before	after
CPU Usage (%)	21	21
Cores usage (%)	574	580
Working Set (MB)	48	48
Build Time (ms)	4,541	4,923
Start Time (ms)	0	0
Published Size (KB)	76,401	76,401
.NET Core SDK Version	3.1.404	3.1.404
First Request (ms)	78	79
Requests/sec	252,468	259,239
Requests	3,812,090	3,914,333
Mean latency (ms)	1.18	1.15
Max latency (ms)	47.33	48.63
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	794.52	815.83
Latency 50th (ms)	0.99	0.96
Latency 75th (ms)	1.09	1.07
Latency 90th (ms)	1.23	1.19
Latency 99th (ms)	9.10	9.03


          don't acces expensive CacheEntryHelper.Current if options can't be pr…

fe723e0

…opagated anyway, +20k RPS for TechEmpower benchmark!

adamsitnik added tenet-performance area-Extensions-Caching labels

adamsitnik added this to the 6.0.0 milestone

adamsitnik requested review from maryamariyan and eerhardt

November 27, 2020 16:31

ghost commented Nov 27, 2020

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

Issue Details

CacheEntryHelper.Current uses expensive AsyncLocal. Before accessing it, we can just check if the options can be propagated and don't use it at all if not.

+7.5% (+18k) RPS for TechEmpower benchmark!

load	before	after
CPU Usage (%)	19	21
Cores usage (%)	528	574
Working Set (MB)	48	48
Build Time (ms)	4,500	4,541
Start Time (ms)	0	0
Published Size (KB)	76,401	76,401
.NET Core SDK Version	3.1.404	3.1.404
First Request (ms)	76	78
Requests/sec	234,029	252,468
Requests	3,533,646	3,812,090
Mean latency (ms)	1.28	1.18
Max latency (ms)	61.30	47.33
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	736.49	794.52
Latency 50th (ms)	1.06	0.99
Latency 75th (ms)	1.17	1.09
Latency 90th (ms)	1.33	1.23
Latency 99th (ms)	10.08	9.10

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


          inline the hot paths, +7k RPS

066b296

adamsitnik changed the title ~~don't acces expensive CacheEntryHelper.Current if options can't be propagated anyway~~ More MemoryCache perf improvements

eerhardt reviewed

View reviewed changes

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

eerhardt reviewed

View reviewed changes

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

@@ @@ -220,7 +221,8 @@ public void Dispose() @@
                           }
                       }
-                      internal bool CheckExpired(DateTimeOffset now)
+                      [MethodImpl(MethodImplOptions.AggressiveInlining)]
+                      internal bool CheckExpired(in DateTimeOffset now)

Member

eerhardt Nov 27, 2020

How much does the ‘in’ help here?

Member Author

adamsitnik Nov 30, 2020

I just wanted to be 100% sure that this struct is not being copied on Linux.

Member

stephentoub Dec 1, 2020

I just wanted to be 100% sure that this struct is not being copied on Linux.

Did this one change make any measurable impact? We should not be sprinkling lots of ins everywhere unless they're actually meaningful.

eerhardt reviewed

View reviewed changes

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

-                          if (_slidingExpiration.HasValue
-                              && (now - LastAccessed) >= _slidingExpiration)
+                          return SlowPath(now);

Member

eerhardt Nov 27, 2020

Is the idea here that the above check can get inlined, and then the “SlowPath” code remains a normal method call? Is there a good guidance on when this pattern should be used?

Side nit - is “SlowPath” a good name for this method?

Member Author

adamsitnik Nov 30, 2020

Is the idea here that the above check can get inlined, and then the “SlowPath” code remains a normal method call?

Yes, exactly.

Is there a good guidance on when this pattern should be used?

I would say that it should be used when the condtion typically returns false and rarely executes the code inside it (like throwing exceptions) and is ofc on a hot path (small helper methods executed frequently)

adamsitnik added 2 commits

November 30, 2020 14:13


          prefer .HasValue over null check for nullables, use better names for …

8bbaad3

…private helper methods


          if possible, avoid expensive CacheEntryHelper.Current access when add…

1cb3d77

…ing cache entries

jnyrup reviewed

View reviewed changes

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

adamsitnik commented

View reviewed changes

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


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

8381ed6

…ry.cs

eerhardt approved these changes

View reviewed changes

Member

eerhardt left a comment

This LGTM. Nice work here @adamsitnik! Looks like a good perf win.

Tratcher approved these changes

View reviewed changes

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

@@ @@ -215,12 +216,17 @@ public void Dispose() @@
                               if (_valueHasBeenSet)
                               {
                                   _notifyCacheEntryCommit(this);
-                                  PropagateOptions(CacheEntryHelper.Current);
+                                  if (CanPropagateOptions())

Member

Tratcher Dec 1, 2020

Always leave a comment about why you're doing what appears to be a redundant check.

adamsitnik merged commit a2f81e7 into dotnet:master

adamsitnik deleted the anotherMemoryCachePerfImprovement branch

December 1, 2020 05:57

stephentoub reviewed

View reviewed changes

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

@@ @@ -235,27 +241,44 @@ internal void SetExpired(EvictionReason reason) @@
                           DetachTokens();
                       }
-                      private bool CheckForExpiredTime(DateTimeOffset now)
+                      private bool CheckForExpiredTime(in DateTimeOffset now)

Member

stephentoub Dec 1, 2020

Same question.

Tornhoof mentioned this pull request

AsyncLocal Memory Usage in MemoryCache #45436

Closed

adamsitnik mentioned this pull request

[Proposal] use Timer in MemoryCache #45842

Closed

ghost locked as resolved and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

jnyrup jnyrup left review comments

stephentoub stephentoub left review comments

Tratcher Tratcher approved these changes

eerhardt eerhardt approved these changes

maryamariyan Awaiting requested review from maryamariyan

Labels

area-Extensions-Caching tenet-performance