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

Reduce CacheEntry size #45962

Merged
merged 19 commits into from
Jan 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
227c96d
reduce the size of CacheEntry instance by 2xsizeof(int)
adamsitnik Dec 10, 2020
5c8b862
remove _lock field and use "this" for locking instead
adamsitnik Dec 10, 2020
4b529fb
move _expirationTokenRegistrations, _postEvictionCallbacks and _expir…
adamsitnik Dec 10, 2020
c04da17
move private types into separate files
adamsitnik Dec 10, 2020
217ab43
rename the types + add comments
adamsitnik Dec 10, 2020
c6597db
inline private properties that are used just twice in total
adamsitnik Dec 11, 2020
c519666
force inline some methods based on profiling data
adamsitnik Dec 11, 2020
4a5715a
rename files
adamsitnik Dec 11, 2020
f170e8d
apply code review suggestion
adamsitnik Dec 11, 2020
01a2686
apply code review suggestion: don't use Enum.HasFlag
adamsitnik Dec 11, 2020
0c07731
apply code review suggestion: don't use Enum.HasFlag
adamsitnik Dec 11, 2020
46ae2ea
apply code review suggestion: remove redundant Interlocked.CompareExc…
adamsitnik Dec 11, 2020
c3e75ea
address code review feedback: encapsulate the fields of CacheEntryTokens
adamsitnik Dec 14, 2020
f582eb6
Apply suggestions from code review
adamsitnik Jan 5, 2021
6a39239
there is no need for explicit memory layout anymore
adamsitnik Jan 5, 2021
58d6a0e
there is no need for using IList abstraction (the field was changed f…
adamsitnik Jan 5, 2021
a1fcbd1
restore call to parameterless ctor to ensure that all struct fields a…
adamsitnik Jan 5, 2021
356e02f
CopyTokens => PropagateTokens for naming consistency with PropagateOp…
adamsitnik Jan 5, 2021
9b189eb
Apply suggestions from code review
adamsitnik Jan 7, 2021
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
@@ -0,0 +1,61 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;

namespace Microsoft.Extensions.Caching.Memory
{
internal partial class CacheEntry
{
// this type exists just to reduce CacheEntry size by replacing many enum & boolean fields with one of a size of Int32
private struct CacheEntryState
{
private byte _flags;
private byte _evictionReason;
private byte _priority;

internal CacheEntryState(CacheItemPriority priority) : this() => _priority = (byte)priority;
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

internal bool IsDisposed
{
get => ((Flags)_flags & Flags.IsDisposed) != 0;
set => SetFlag(Flags.IsDisposed, value);
}

internal bool IsExpired
{
get => ((Flags)_flags & Flags.IsExpired) != 0;
set => SetFlag(Flags.IsExpired, value);
}

internal bool IsValueSet
{
get => ((Flags)_flags & Flags.IsValueSet) != 0;
set => SetFlag(Flags.IsValueSet, value);
}

internal EvictionReason EvictionReason
{
get => (EvictionReason)_evictionReason;
set => _evictionReason = (byte)value;
}

internal CacheItemPriority Priority
{
get => (CacheItemPriority)_priority;
set => _priority = (byte)value;
}

private void SetFlag(Flags option, bool value) => _flags = (byte)(value ? (_flags | (byte)option) : (_flags & ~(byte)option));
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved

[Flags]
private enum Flags : byte
{
Default = 0,
IsValueSet = 1 << 0,
IsExpired = 1 << 1,
IsDisposed = 1 << 2,
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Primitives;

namespace Microsoft.Extensions.Caching.Memory
{
internal partial class CacheEntry
{
// this type exists just to reduce average CacheEntry size
// which typically is not using expiration tokens or callbacks
private sealed class CacheEntryTokens
{
private List<IChangeToken> _expirationTokens;
private List<IDisposable> _expirationTokenRegistrations;
private List<PostEvictionCallbackRegistration> _postEvictionCallbacks; // this is not really related to tokens, but was moved here to shrink typical CacheEntry size

internal List<IChangeToken> ExpirationTokens => _expirationTokens ??= new List<IChangeToken>();
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
internal List<PostEvictionCallbackRegistration> PostEvictionCallbacks => _postEvictionCallbacks ??= new List<PostEvictionCallbackRegistration>();

internal void AttachTokens()
{
if (_expirationTokens != null)
{
lock (this)
{
for (int i = 0; i < _expirationTokens.Count; i++)
{
IChangeToken expirationToken = _expirationTokens[i];
if (expirationToken.ActiveChangeCallbacks)
{
_expirationTokenRegistrations ??= new List<IDisposable>(1);
IDisposable registration = expirationToken.RegisterChangeCallback(ExpirationCallback, this);
_expirationTokenRegistrations.Add(registration);
}
}
}
}
}

internal bool CheckForExpiredTokens(CacheEntry cacheEntry)
{
if (_expirationTokens != null)
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this need a lock like the other two methods?

Copy link
Member Author

Choose a reason for hiding this comment

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

{
for (int i = 0; i < _expirationTokens.Count; i++)
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
{
IChangeToken expiredToken = _expirationTokens[i];
if (expiredToken.HasChanged)
{
cacheEntry.SetExpired(EvictionReason.TokenExpired);
return true;
}
}
}
return false;
}

internal bool CanPropagateTokens() => _expirationTokens != null;

internal void PropagateTokens(CacheEntry parentEntry)
{
if (_expirationTokens != null)
{
lock (this)
{
lock (parentEntry.GetOrCreateTokens())
Copy link
Member

Choose a reason for hiding this comment

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

Where else is this locked on?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how that answers my question. My primary concern would be if it'd ever be possible for something to take the second lock and then try to take the first, creating a priority inversion and deadlock potential. I'm simply wondering if that's possible. Does the child/parent nature of this guarantee locks are always acquired in the same order?

Copy link
Member Author

Choose a reason for hiding this comment

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

Does the child/parent nature of this guarantee locks are always acquired in the same order?

I think it does in the case when the cache item is being added to the cache (by calling the Dispose method):

if (_previous != null && CanPropagateOptions())
{
PropagateOptions(_previous);

but I am not 100% sure if it would not be possible to create a code that does this and abuses the second possible call to this method from the Cache.TryGetValue method (which is very unintuitive for me):

// 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);

FWIW I have not changed the locking order, it's the same as it used to be so far:

lock (_lock)
{
lock (parent._lock)
{
foreach (IChangeToken expirationToken in _expirationTokens)
{
parent.AddExpirationToken(expirationToken);
}
}
}

{
foreach (IChangeToken expirationToken in _expirationTokens)
Copy link
Member

Choose a reason for hiding this comment

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

Why in some cases are we using a for loop with direct indexing to enumerate the contents and in others we're using foreach?

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 don't know as I am not the author of the original code. To minimize the diff and make the review easier I've decided to change as few things as possible

{
parentEntry.AddExpirationToken(expirationToken);
}
}
}
}
}

internal void DetachTokens()
{
// _expirationTokenRegistrations is not checked for null, because AttachTokens might initialize it under lock
// instead we are checking for _expirationTokens, because if they are not null, then _expirationTokenRegistrations might also be not null
if (_expirationTokens != null)
{
lock (this)
{
List<IDisposable> registrations = _expirationTokenRegistrations;
if (registrations != null)
{
_expirationTokenRegistrations = null;
for (int i = 0; i < registrations.Count; i++)
{
IDisposable registration = registrations[i];
registration.Dispose();
}
}
}
}
}

internal void InvokeEvictionCallbacks(CacheEntry cacheEntry)
{
if (_postEvictionCallbacks != null)
{
Task.Factory.StartNew(state => InvokeCallbacks((CacheEntry)state), cacheEntry,
CancellationToken.None, TaskCreationOptions.DenyChildAttach, TaskScheduler.Default);
}
}

private static void InvokeCallbacks(CacheEntry entry)
{
List<PostEvictionCallbackRegistration> callbackRegistrations = Interlocked.Exchange(ref entry._tokens._postEvictionCallbacks, null);

if (callbackRegistrations == null)
{
return;
}

for (int i = 0; i < callbackRegistrations.Count; i++)
{
PostEvictionCallbackRegistration registration = callbackRegistrations[i];

try
{
registration.EvictionCallback?.Invoke(entry.Key, entry.Value, entry.EvictionReason, registration.State);
}
catch (Exception e)
{
// This will be invoked on a background thread, don't let it throw.
entry._cache._logger.LogError(e, "EvictionCallback invoked failed");
}
}
}
}
}
}
Loading