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

More MemoryCache perf improvements #45280

Merged
Merged
Show file tree
Hide file tree
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
55 changes: 40 additions & 15 deletions src/libraries/Microsoft.Extensions.Caching.Memory/src/CacheEntry.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -215,12 +216,17 @@ public void Dispose()
if (_valueHasBeenSet)
{
_notifyCacheEntryCommit(this);
PropagateOptions(CacheEntryHelper.Current);

if (CanPropagateOptions())
Copy link
Member

Choose a reason for hiding this comment

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

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

{
PropagateOptions(CacheEntryHelper.Current);
}
}
}
}

internal bool CheckExpired(DateTimeOffset now)
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal bool CheckExpired(in DateTimeOffset now)
Copy link
Member

Choose a reason for hiding this comment

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

How much does the ‘in’ help 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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

{
return _isExpired || CheckForExpiredTime(now) || CheckForExpiredTokens();
}
Expand All @@ -235,27 +241,44 @@ internal void SetExpired(EvictionReason reason)
DetachTokens();
}

private bool CheckForExpiredTime(DateTimeOffset now)
private bool CheckForExpiredTime(in DateTimeOffset now)
Copy link
Member

Choose a reason for hiding this comment

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

Same question.

{
if (_absoluteExpiration.HasValue && _absoluteExpiration.Value <= now)
if (!_absoluteExpiration.HasValue && !_slidingExpiration.HasValue)
{
SetExpired(EvictionReason.Expired);
return true;
return false;
}

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

bool FullCheck(in DateTimeOffset offset)
{
SetExpired(EvictionReason.Expired);
return true;
}
if (_absoluteExpiration.HasValue && _absoluteExpiration.Value <= offset)
{
SetExpired(EvictionReason.Expired);
return true;
}

return false;
if (_slidingExpiration.HasValue
&& (offset - LastAccessed) >= _slidingExpiration)
{
SetExpired(EvictionReason.Expired);
return true;
}

return false;
}
}

internal bool CheckForExpiredTokens()
private bool CheckForExpiredTokens()
{
if (_expirationTokens != null)
if (_expirationTokens == null)
{
return false;
}

return CheckTokens();

bool CheckTokens()
{
for (int i = 0; i < _expirationTokens.Count; i++)
{
Expand All @@ -266,8 +289,8 @@ internal bool CheckForExpiredTokens()
return true;
}
}
return false;
}
return false;
}

internal void AttachTokens()
Expand Down Expand Up @@ -355,6 +378,8 @@ private static void InvokeCallbacks(CacheEntry entry)
}
}

internal bool CanPropagateOptions() => _expirationTokens != null || _absoluteExpiration.HasValue;

internal void PropagateOptions(CacheEntry parent)
{
if (parent == null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,9 +250,12 @@ public bool TryGetValue(object key, out object result)
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);
if (entry.CanPropagateOptions())
{
// 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);

Expand Down