-
Notifications
You must be signed in to change notification settings - Fork 5.4k
few minor MemoryCache perf improvements #44797
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
Changes from all commits
6fa2ccf
9de3a9e
bcc8584
5d395aa
e03d0a2
4eb26ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| using System.Collections.Concurrent; | ||
| using System.Collections.Generic; | ||
| using System.Linq; | ||
| using System.Runtime.CompilerServices; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.Extensions.Internal; | ||
|
|
@@ -230,44 +231,45 @@ private void SetEntry(CacheEntry entry) | |
| } | ||
| } | ||
|
|
||
| StartScanForExpiredItems(utcNow); | ||
| StartScanForExpiredItemsIfNeeded(utcNow); | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool TryGetValue(object key, out object result) | ||
| { | ||
| ValidateCacheKey(key); | ||
|
|
||
| CheckDisposed(); | ||
|
|
||
| result = null; | ||
| DateTimeOffset utcNow = _options.Clock.UtcNow; | ||
| bool found = false; | ||
|
|
||
| if (_entries.TryGetValue(key, out CacheEntry entry)) | ||
| { | ||
| // Check if expired due to expiration tokens, timers, etc. and if so, remove it. | ||
| // Allow a stale Replaced value to be returned due to concurrent calls to SetExpired during SetEntry. | ||
| if (entry.CheckExpired(utcNow) && entry.EvictionReason != EvictionReason.Replaced) | ||
| if (!entry.CheckExpired(utcNow) || entry.EvictionReason == EvictionReason.Replaced) | ||
| { | ||
| // TODO: For efficiency queue this up for batch removal | ||
| RemoveEntry(entry); | ||
| } | ||
| else | ||
| { | ||
| found = true; | ||
| entry.LastAccessed = utcNow; | ||
| result = entry.Value; | ||
|
|
||
| // When this entry is retrieved in the scope of creating another entry, | ||
| // that entry needs a copy of these expiration tokens. | ||
| entry.PropagateOptions(CacheEntryHelper.Current); | ||
|
|
||
| StartScanForExpiredItemsIfNeeded(utcNow); | ||
|
|
||
| return true; | ||
| } | ||
| else | ||
| { | ||
| // TODO: For efficiency queue this up for batch removal | ||
| RemoveEntry(entry); | ||
| } | ||
| } | ||
|
|
||
| StartScanForExpiredItems(utcNow); | ||
| StartScanForExpiredItemsIfNeeded(utcNow); | ||
|
|
||
| return found; | ||
| result = null; | ||
| return false; | ||
adamsitnik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
|
|
@@ -287,7 +289,7 @@ public void Remove(object key) | |
| entry.InvokeEvictionCallbacks(); | ||
| } | ||
|
|
||
| StartScanForExpiredItems(); | ||
| StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); | ||
| } | ||
|
|
||
| private void RemoveEntry(CacheEntry entry) | ||
|
|
@@ -306,26 +308,30 @@ private void EntryExpired(CacheEntry entry) | |
| { | ||
| // TODO: For efficiency consider processing these expirations in batches. | ||
| RemoveEntry(entry); | ||
| StartScanForExpiredItems(); | ||
| StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow); | ||
| } | ||
|
|
||
| // 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)] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. before my changes, the now I was simply not sure if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Ok, but the same argument goes for whether ScheduleTask may get inlined automatically even though you don't want it to.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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? |
||
| private void StartScanForExpiredItemsIfNeeded(DateTimeOffset utcNow) | ||
| { | ||
| // Since fetching time is expensive, minimize it in the hot paths | ||
| DateTimeOffset now = utcNow ?? _options.Clock.UtcNow; | ||
| if (_options.ExpirationScanFrequency < now - _lastExpirationScan) | ||
| if (_options.ExpirationScanFrequency < utcNow - _lastExpirationScan) | ||
| { | ||
| _lastExpirationScan = now; | ||
| ScheduleTask(utcNow); | ||
| } | ||
|
|
||
| void ScheduleTask(DateTimeOffset utcNow) | ||
| { | ||
| _lastExpirationScan = utcNow; | ||
| Task.Factory.StartNew(state => ScanForExpiredItems((MemoryCache)state), this, | ||
| CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default); | ||
| } | ||
| } | ||
|
|
||
| private static void ScanForExpiredItems(MemoryCache cache) | ||
| { | ||
| DateTimeOffset now = cache._options.Clock.UtcNow; | ||
| DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock.UtcNow; | ||
adamsitnik marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| foreach (CacheEntry entry in cache._entries.Values) | ||
| { | ||
| if (entry.CheckExpired(now)) | ||
|
|
@@ -500,16 +506,20 @@ private void CheckDisposed() | |
| { | ||
| if (_disposed) | ||
| { | ||
| throw new ObjectDisposedException(typeof(MemoryCache).FullName); | ||
| Throw(); | ||
| } | ||
|
|
||
| static void Throw() => throw new ObjectDisposedException(typeof(MemoryCache).FullName); | ||
| } | ||
|
|
||
| private static void ValidateCacheKey(object key) | ||
| { | ||
| if (key == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(key)); | ||
| Throw(); | ||
| } | ||
|
|
||
| static void Throw() => throw new ArgumentNullException(nameof(key)); | ||
| } | ||
| } | ||
| } | ||



Uh oh!
There was an error while loading. Please reload this page.