Skip to content

Add metrics for caching #66479

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

Merged
merged 30 commits into from
Apr 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
68478c3
Adds API to retrieve caching metrics
maryamariyan Mar 11, 2022
1c4d8b5
Make CurrentSize and CurrentEntryCount return coherent values
maryamariyan Mar 15, 2022
2a4743a
Add test
maryamariyan Mar 15, 2022
92c187b
Apply API review feedback - TODO: DIM support
maryamariyan Mar 15, 2022
f18b4cf
Remove MatchingRefApiCompatBaseline, leftover from init on struct
maryamariyan Mar 15, 2022
8f3c13c
Undo CoherentState._cacheSize changes and add DIM
maryamariyan Mar 16, 2022
d36af3f
Add DIM test and fix up csproj condition
maryamariyan Mar 16, 2022
d013412
CommonPath -> CoreLibSharedDir
maryamariyan Mar 16, 2022
da10ccd
ref fix up
maryamariyan Mar 16, 2022
86db1b3
Applies PR feedback
maryamariyan Mar 16, 2022
e7be224
Update src/libraries/Microsoft.Extensions.Caching.Abstractions/src/IM…
maryamariyan Mar 16, 2022
fede414
Rename to "*net7.0.cs" from "*net7.cs"
maryamariyan Mar 18, 2022
77e390d
using thread local to get per memory cache stats and better perf
maryamariyan Mar 18, 2022
4aefd1b
Applies PR feedback - with micro perf improvement
maryamariyan Mar 19, 2022
bdd6fb7
nit fix/readability improvement
maryamariyan Mar 19, 2022
c900483
apply nit feedback
maryamariyan Mar 21, 2022
2eef441
supporting long hit/miss
maryamariyan Mar 21, 2022
e7b8d8f
Adds MemoryCacheOptions.TrackStatistics options flag
maryamariyan Mar 21, 2022
e149d8f
Update src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCa…
maryamariyan Mar 22, 2022
f446293
update on lock for 32bit machine
maryamariyan Mar 22, 2022
1a5c7f4
undo unnecessary Interlock
maryamariyan Mar 22, 2022
3e2ce4f
Add TrimExcess
maryamariyan Mar 23, 2022
fc10cc0
test cleanup
maryamariyan Mar 23, 2022
0cc5d7f
change flag condition to _allStats not null
maryamariyan Mar 23, 2022
20cc800
Applies latest PR feedback
maryamariyan Mar 23, 2022
b3c5653
Second round feedback
maryamariyan Mar 24, 2022
663b66f
support DIM on net6 as well
maryamariyan Mar 24, 2022
2a5b0c6
Update src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCa…
maryamariyan Mar 24, 2022
cc641ed
Caching inlining
maryamariyan Mar 24, 2022
9d2c26a
Removing method and writing code inline
maryamariyan Mar 25, 2022
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,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// The compiler emits a reference to the internal copy of this type in our non-NETCoreApp assembly
// so we must include a forward to be compatible with libraries compiled against non-NETCoreApp Microsoft.Extensions.Caching.Abstractions
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Runtime.CompilerServices.IsExternalInit))]
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,14 @@ public MemoryCacheEntryOptions() { }
public long? Size { get { throw null; } set { } }
public System.TimeSpan? SlidingExpiration { get { throw null; } set { } }
}
public partial class MemoryCacheStatistics
{
public MemoryCacheStatistics() { }
public long CurrentEntryCount { get { throw null; } init { } }
public long? CurrentEstimatedSize { get { throw null; } init { } }
public long TotalHits { get { throw null; } init { } }
public long TotalMisses { get { throw null; } init { } }
}
public partial class PostEvictionCallbackRegistration
{
public PostEvictionCallbackRegistration() { }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@

<ItemGroup>
<Compile Include="Microsoft.Extensions.Caching.Abstractions.cs" />
<Compile Include="Microsoft.Extensions.Caching.Abstractions.net6.0.cs" Condition="$([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'net6.0'))" />
<Compile Include="Microsoft.Extensions.Caching.Abstractions.Typeforwards.netcoreapp.cs" Condition="'$(TargetFrameworkIdentifier)' == '.NETCoreApp'"/>
<Compile Include="$(CoreLibSharedDir)System\Runtime\CompilerServices\IsExternalInit.cs" Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'"
Link="Common\System\Runtime\CompilerServices\IsExternalInit.cs" />
<ProjectReference Include="..\..\Microsoft.Extensions.Primitives\ref\Microsoft.Extensions.Primitives.csproj" />
</ItemGroup>

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// ------------------------------------------------------------------------------
// Changes to this file must follow the https://aka.ms/api-review process.
// ------------------------------------------------------------------------------

namespace Microsoft.Extensions.Caching.Memory
{
public partial interface IMemoryCache : System.IDisposable
{
MemoryCacheStatistics? GetCurrentStatistics() => null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,13 @@ public interface IMemoryCache : IDisposable
/// </summary>
/// <param name="key">An object identifying the entry.</param>
void Remove(object key);

#if NET6_0_OR_GREATER
/// <summary>
/// Gets a snapshot of the cache statistics if available.
/// </summary>
/// <returns>An instance of <see cref="MemoryCacheStatistics"/> containing a snapshot of the cache statistics.</returns>
MemoryCacheStatistics? GetCurrentStatistics() => null;
#endif
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
// 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
{
/// <summary>
/// Holds a snapshot of statistics for a memory cache.
/// </summary>
public class MemoryCacheStatistics
{
/// <summary>
/// Initializes an instance of MemoryCacheStatistics.
/// </summary>
public MemoryCacheStatistics() { }

/// <summary>
/// Gets the number of <see cref="ICacheEntry" /> instances currently in the memory cache.
/// </summary>
public long CurrentEntryCount { get; init; }

/// <summary>
/// Gets an estimated sum of all the <see cref="ICacheEntry.Size" /> values currently in the memory cache.
/// </summary>
/// <returns>Returns <see langword="null"/> if size isn't being tracked. The common MemoryCache implementation tracks size whenever a SizeLimit is set on the cache.</returns>
public long? CurrentEstimatedSize { get; init; }

/// <summary>
/// Gets the total number of cache misses.
/// </summary>
public long TotalMisses { get; init; }

/// <summary>
/// Gets the total number of cache hits.
/// </summary>
public long TotalHits { get; init; }
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// The compiler emits a reference to the internal copy of this type in our non-NETCoreApp assembly
// so we must include a forward to be compatible with libraries compiled against non-NETCoreApp Microsoft.Extensions.Caching.Abstractions
[assembly: System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Runtime.CompilerServices.IsExternalInit))]
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,9 @@ Microsoft.Extensions.Caching.Memory.IMemoryCache</PackageDescription>
<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.Primitives\src\Microsoft.Extensions.Primitives.csproj" />
</ItemGroup>

<ItemGroup Condition="'$(TargetFrameworkIdentifier)' != '.NETCoreApp'">
<Compile Remove="Microsoft.Extensions.Caching.Abstractions.Typeforwards.netcoreapp.cs" />
<Compile Include="$(CoreLibSharedDir)System\Runtime\CompilerServices\IsExternalInit.cs" Link="Common\System\Runtime\CompilerServices\IsExternalInit.cs" />
</ItemGroup>

</Project>
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public void Compact(double percentage) { }
public void Dispose() { }
protected virtual void Dispose(bool disposing) { }
~MemoryCache() { }
public Microsoft.Extensions.Caching.Memory.MemoryCacheStatistics? GetCurrentStatistics() { throw null; }
public void Remove(object key) { }
public bool TryGetValue(object key, out object? result) { throw null; }
}
Expand All @@ -45,6 +46,7 @@ public MemoryCacheOptions() { }
Microsoft.Extensions.Caching.Memory.MemoryCacheOptions Microsoft.Extensions.Options.IOptions<Microsoft.Extensions.Caching.Memory.MemoryCacheOptions>.Value { get { throw null; } }
public long? SizeLimit { get { throw null; } set { } }
public bool TrackLinkedCacheEntries { get { throw null; } set { } }
public bool TrackStatistics { get { throw null; } set { } }
Copy link
Member

Choose a reason for hiding this comment

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

what do people think about making the default value of this property true? I suspect most developers, even those writing high performance services, will still probably prefer being able to monitor their cache statistics rather than squeezing out a tiny bit more speed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

during the API review meeting today the ask was to disable it by default to not hurt the perf for those not tracking the stats

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just tested it again now, with TrackStatistics enabled.

As summarized earlier, the new approach would drop the TE RPS by 5%:

image

Copy link
Member

Choose a reason for hiding this comment

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

IMO true would be the better default but I'm not going to die on that hill : )

Copy link
Member

Choose a reason for hiding this comment

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

We can disable stats in the benchmark though, this is similar to what we do with logging. That should apply here as well right?

Copy link
Member

Choose a reason for hiding this comment

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

I think the difference is that far more people want logging by default than will want MemoryCache.GetCurrentStatistics() by default. On the other hand as @eerhardt said in API review, this benchmark is pretty much the worst-case overhead.

I think defaulting to not tracking makes sense, but I'm not super opinionated.

Copy link
Contributor Author

@maryamariyan maryamariyan Mar 23, 2022

Choose a reason for hiding this comment

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

I'd be fine either way, though this would need another API review churn. Would personally prefer to have this PR dealt with as-is and deal with this discussion on a separate issue.

cc @sebastienros @adamsitnik do you have preference here of what default to choose?

Copy link
Member

Choose a reason for hiding this comment

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

This would be my speculation how projects break down:

  1. Projects that deploy on x86 and run within one order of magnitude of the load created by the TE benchmark: < 0.1% of apps
  2. Projects where the developer wants to directly invoke GetCurrentStatistics(): <1% of apps
  3. Projects where the developer will appreciate using a monitoring solution (AppInsights, OpenTelemetry, etc) or ad-hoc tools (dotnet-counters) to make the stats available for them in the future: 10-20% apps
  4. Projects where neither the stats nor the perf matters: 50+% of apps

I'm worried about the experience of people in group (3). I want them to be able to turn on telemetry very easily when they want it. Anything where they have to follow extra instructions or change more lines of code creates friction in that process. If that means a tiny set of projects in group (1) have to have some friction to disable stats they don't care about I think it is the right tradeoff given my guess at the relative sizes of those groups. Certainly if others think my guesses about the group sizes are wrong they might come to a different conclusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd assume for case 3, when using dotnet-counters there would still be need for code change for setting up counters with the GetCurrentStatistics API and in which case also setting the flag should be not too much churn.

But thinking perhaps with OT or AppInsights there is no need for code change and therefore, as you said once those solutions are in for cache metrics retrieval via OT, then it could be that extra instruction for them to also set TrackStatistics to true.

}
public partial class MemoryDistributedCacheOptions : Microsoft.Extensions.Caching.Memory.MemoryCacheOptions
{
Expand Down
115 changes: 115 additions & 0 deletions src/libraries/Microsoft.Extensions.Caching.Memory/src/MemoryCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ public class MemoryCache : IMemoryCache

private readonly MemoryCacheOptions _options;

private readonly List<WeakReference<Stats>>? _allStats;
private readonly Stats? _accumulatedStats;
private readonly ThreadLocal<Stats>? _stats;
private CoherentState _coherentState;
private bool _disposed;
private DateTimeOffset _lastExpirationScan;
Expand Down Expand Up @@ -53,6 +56,13 @@ public MemoryCache(IOptions<MemoryCacheOptions> optionsAccessor!!, ILoggerFactor
_options.Clock = new SystemClock();
}

if (_options.TrackStatistics)
{
_allStats = new List<WeakReference<Stats>>();
_accumulatedStats = new Stats();
_stats = new ThreadLocal<Stats>(() => new Stats(this));
}

_lastExpirationScan = _options.Clock.UtcNow;
TrackLinkedCacheEntries = _options.TrackLinkedCacheEntries; // we store the setting now so it's consistent for entire MemoryCache lifetime
}
Expand Down Expand Up @@ -219,6 +229,14 @@ public bool TryGetValue(object key!!, out object? result)
}

StartScanForExpiredItemsIfNeeded(utcNow);
// Hit
if (_allStats is not null)
{
if (IntPtr.Size == 4)
Interlocked.Increment(ref GetStats().Hits);
else
GetStats().Hits++;
}

return true;
}
Expand All @@ -232,6 +250,15 @@ public bool TryGetValue(object key!!, out object? result)
StartScanForExpiredItemsIfNeeded(utcNow);

result = null;
// Miss
if (_allStats is not null)
{
if (IntPtr.Size == 4)
Interlocked.Increment(ref GetStats().Misses);
else
GetStats().Misses++;
}

return false;
}

Expand Down Expand Up @@ -270,6 +297,27 @@ public void Clear()
}
}

/// <summary>
/// Gets a snapshot of the current statistics for the memory cache.
/// </summary>
/// <returns>Returns <see langword="null"/> if statistics are not being tracked because <see cref="MemoryCacheOptions.TrackStatistics" /> is <see langword="false"/>.</returns>
public MemoryCacheStatistics? GetCurrentStatistics()
{
if (_allStats is not null)
{
(long hit, long miss) sumTotal = Sum();
return new MemoryCacheStatistics()
{
TotalMisses = sumTotal.miss,
TotalHits = sumTotal.hit,
CurrentEntryCount = Count,
CurrentEstimatedSize = _options.SizeLimit.HasValue ? Size : null
};
}

return null;
}

internal void EntryExpired(CacheEntry entry)
{
// TODO: For efficiency consider processing these expirations in batches.
Expand All @@ -295,6 +343,72 @@ void ScheduleTask(DateTimeOffset utcNow)
}
}

private (long, long) Sum()
{
lock (_allStats!)
{
long hits = _accumulatedStats!.Hits;
long misses = _accumulatedStats.Misses;

foreach (WeakReference<Stats> wr in _allStats)
{
if (wr.TryGetTarget(out Stats? stats))
{
hits += Interlocked.Read(ref stats.Hits);
misses += Interlocked.Read(ref stats.Misses);
}
}

return (hits, misses);
}
}

private Stats GetStats() => _stats!.Value!;

internal sealed class Stats
{
private readonly MemoryCache? _memoryCache;
public long Hits;
public long Misses;

public Stats() { }

public Stats(MemoryCache memoryCache)
{
_memoryCache = memoryCache;
_memoryCache.AddToStats(this);
}

~Stats() => _memoryCache?.RemoveFromStats(this);
}

private void RemoveFromStats(Stats current)
{
lock (_allStats!)
{
for (int i = 0; i < _allStats.Count; i++)
{
if (_allStats[i].TryGetTarget(out Stats? stats) && stats == current)
{
_allStats.RemoveAt(i);
break;
}
}

_accumulatedStats!.Hits += Interlocked.Read(ref current.Hits);
_accumulatedStats.Misses += Interlocked.Read(ref current.Misses);
_allStats.TrimExcess();
}
}

private void AddToStats(Stats current)
{
lock (_allStats!)
{
_allStats.Add(new WeakReference<Stats>(current));
}
}

private static void ScanForExpiredItems(MemoryCache cache)
{
DateTimeOffset now = cache._lastExpirationScan = cache._options.Clock!.UtcNow;
Expand Down Expand Up @@ -469,6 +583,7 @@ protected virtual void Dispose(bool disposing)
{
if (disposing)
{
_stats?.Dispose();
GC.SuppressFinalize(this);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ public double CompactionPercentage
/// <remarks>Prior to .NET 7 this feature was always enabled.</remarks>
public bool TrackLinkedCacheEntries { get; set; }

/// <summary>
/// Gets or sets whether to track memory cache statistics. Disabled by default.
/// </summary>
public bool TrackStatistics { get; set; }

MemoryCacheOptions IOptions<MemoryCacheOptions>.Value
{
get { return this; }
Expand Down
Loading