Inconsistent MemoryCache stats #116193
Inconsistent MemoryCache stats #116193
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
How is the stats object now collected? |
|
@pedrobsaila I agree with @rhuijben that it's not clear what will happen. Perhaps some simpler approach to fix this will work... just two static longs and Interlocked.Add - since we never increment both at the same we should be good with this. We should be able to get rid of stats class completely and logic becomes much simpler. I don't think it will have much overhead as well |
|
In case Interlocked.Add is not suitable also consider not having Stats class at all, perhaps struct will be more suitable then you wouldn't have a reference you'd block |
|
IMO for the regular scenario this should be win for x64 - as far as I understand this should be close to a single instruction. For x32 - well they'll suffer a bit but less allocations and it's not super common scenario for perf anyway |
Interlocked.Add is a single instruction on x64. This single instruction can take thousands of cycles if multiple threads try to increment the same memory location in parallel. |
see the comment #108333 (comment) explaining the solution I implemented :
|
I'll try to implement it |
|
Perf test : Results :
|
|
These micro-benchmarks are insufficient to observe the impact on multithreaded workloads. Try to build multi-threaded version of |
|
Tried the following : [GlobalSetup(Targets = new[] { nameof(GetHitParallel) })]
public void SetupBasic()
{
_memCache = new MemoryCache(new MemoryCacheOptions() { TrackStatistics = true });
for (var i = 0; i < 1024; i++)
{
_memCache.Set(i, i.ToString());
}
}
[GlobalCleanup(Targets = new[] { nameof(GetHitParallel) })]
public void CleanupBasic() => _memCache.Dispose();
[Benchmark]
public void GetHitParallel()
{
Parallel.For(0, 100_000_000, new ParallelOptions { MaxDegreeOfParallelism = Environment.ProcessorCount }, i => _memCache.Get("256"));
}Got :
With some warnings :
|
|
@EgorBot -intel using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using Microsoft.Extensions.Caching.Memory;
public class MemoryCacheTest
{
MemoryCache _memCache;
[GlobalSetup(Targets = new[] { nameof(GetHitParallel) })]
public void SetupBasic()
{
_memCache = new MemoryCache(new MemoryCacheOptions() { TrackStatistics = true });
for (var i = 0; i < 1024; i++)
{
_memCache.Set(i.ToString(), new object());
}
}
[GlobalCleanup(Targets = new[] { nameof(GetHitParallel) })]
public void CleanupBasic() => _memCache.Dispose();
[Benchmark]
public async Task GetHitParallel()
{
await Task.WhenAll(
Enumerable
.Range(0, Environment.ProcessorCount)
.Select(_ =>
Task.Run(async delegate
{
for (int i = 0; i < 1000000; i++)
{
var x = _memCache.Get("256");
if (x == null) Environment.FailFast("Unexpected!");
}
})));
}
} |
|
I have fixed the microbenchmark to actually measure |
|
For my understanding, why using async for a CPU bound workload ? |
Copy&paste of the ArrayPool microbenchmark that I have linked to above. It is convenient to write it using async - it is certainly possible to write it without async. |
|
@krwq should I roll back the PR to the previous solution or do you have in mind a different implementation that is simpler/more performant ? |
|
Hello @pedrobsaila, I like the simplicity and readability you achieved using Interlocked.Increment. However, we need every bit of performance here, so the ThreadLocal implementation (although admittedly less elegant) seems like a better fit. What I found interesting is @noahfalk’s comment in the issue about using handlers to keep the numbers alive. I imagine the handler working something like this: internal sealed class StatsHandler
{
private readonly MemoryCache _cache;
private readonly int _index;
public Stats Value { get; }
public StatsHandler(MemoryCache memoryCache)
{
_cache = memoryCache;
Value = new Stats(this);
_index = memoryCache.AddToStats(Value);
}
~StatsHandler() => _cache.RemoveFromStats(Value, _index);
}Its role is simply to control the Stats instance - creating it, adding it to _allStats, providing access, and eventually removing it from there. The Stats is then just a simple class to hold the numbers: internal sealed class Stats
{
public long Hits;
public long Misses;
}The _allStats and _stats fields then have different types: private readonly ThreadLocal<StatsHandler>? _stats;
private readonly List<Stats>? _allStats;And as a little bonus, we don`t have to iterate the whole _allStats list now to remove one as we only add new instances to the end so the one to remove must be on the same or lower index it was added: private void RemoveFromStats(Stats current, int index)
{
lock (_allStats!)
{
_accumulatedHits += Volatile.Read(ref current.Hits);
_accumulatedMisses += Volatile.Read(ref current.Misses);
for (var i = index; i >= 0; i--)
{
if (ReferenceEquals(_allStats[i], current))
{
_allStats.RemoveAt(i);
break;
}
}
_allStats.TrimExcess();
}
}There will be a few places in the class that need to be updated to account for these changes, but in the end the performance profile should stay roughly the same, just with the bug fixed. This is just an idea, what do you think, does it make any sense? |
|
I see you actually had an initial implementation very close to the one I was thinking about, it seems plausible to me, @jkotas, I guess you reviewed the Interlocked.Increment implementation only, but how about keeping the ThreadLocal with handlers so we keep the stats alive between the thread is garbage collected and its finalizer executes? Is there a catch I don`t see? |
rosebyte
added
the
needs-author-action
|
#116193 (comment) suggested that the original implementation had a leak. I have not reviewed it in detail. I am sure you can make it work. We need to make sure that it does not leak (ie it does not keep the stats for dead threads around forever) and that it otherwise performs well. |
dotnet-policy-service
bot
removed
the
needs-author-action
|
Results of the program in Release configuration : using Microsoft.Extensions.Caching.Memory;
MemoryCache cache = new(new MemoryCacheOptions { TrackStatistics = true });
void RunThread()
{
Thread t = new(() =>
{
for (int j = 0; j < 10_000; j += 1)
{
_ = cache.Get("");
}
RunThread();
})
{
Name = "Cache Worker",
};
t.Start();
}
for (int i = 0; i < Environment.ProcessorCount - 1; i += 1)
{
RunThread();
}
Thread integrityThread = new(() =>
{
long lastValue = -1;
while (true)
{
long newValue = cache.GetCurrentStatistics()!.TotalMisses;
if (newValue < lastValue)
{
Console.WriteLine($"{DateTime.Now:HH:mm:ss.fff} ERROR: total misses decreased from {lastValue} to {newValue} (-{lastValue - newValue})");
}
lastValue = newValue;
}
})
{
Name = "Stats Integrity Checker"
};
integrityThread.Start();
integrityThread.Join();
Using perfview, we see that statsHandle object do get finalized with same frequency as Thread object : Finalized Object Counts for Process :
|
5 participants
Fixes #108333