Skip to content
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

Merged
merged 6 commits into from
Nov 18, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Internal;
Expand Down Expand Up @@ -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 />
Expand All @@ -287,7 +289,7 @@ public void Remove(object key)
entry.InvokeEvictionCallbacks();
}

StartScanForExpiredItems();
StartScanForExpiredItemsIfNeeded(_options.Clock.UtcNow);
}

private void RemoveEntry(CacheEntry entry)
Expand All @@ -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)]
Copy link
Member

@stephentoub stephentoub Nov 17, 2020

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)?

Copy link
Member Author

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:

obraz

now StartScanForExpiredItemsIfNeeded does get inlined an ScheduleTask does not:

obraz

obraz

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

Copy link
Member

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.

Copy link
Member Author

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?

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))
Expand Down Expand Up @@ -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));
}
}
}