Implement minimal implementation of HybridCache by mgravell · Pull Request #55147 · dotnet/aspnetcore

mgravell · 2024-04-15T15:57:49Z

Implement minimal implementation of `HybridCache`

This is the first "make something that works" iteration of HybridCache, that builds on the abstract API proposed in #55084 (epic: #53255 also: #54647); it will be re-targeted at main as soon as #55084 is merged

Broad design overview in src/Caching/Hybrid/src/Internal/readme.md

Implemented this wave:

stampede protection (concurrent request combining)
serialization (including configuration)
underlying service invocation
L2 (distributed cache) DI, read, and write-back
L1 (memory cache) DI, read, and write-back
immutable vs mutable type support (defensive copies for mutable)
support for legacy (byte[]) and "buffer" (ReadOnlySequence<byte>, IBufferWriter<byte>) L2 backends
flags detection to disable L1, L2, underlying
direct set-value with L1/L2 write
Redis and SQL Server support for "buffer" L2 backend
supports net9.0; ns2.0; ns2.1; netfx
benchmarks for IDistributedCache vs IBufferDistributedCache
extensive tests for Redis and SQL Server implementations of IBufferDistributedCache (and we found a SqlClient glitch: Inconsistent handling of empty BLOB slices as parameters SqlClient#2465)

Explicitly not implemented yet:

tagging
L2-assisted cache invalidation
additional storage metadata
metrics
logging
compression
support for L3+

Here's the basic headline numbers comparing HybridCache to a class IDistributedCache "get, test, deserialize | fetch+serialize+set" loop using a local Redis backend and a simple POCO type (the fetch is artificial, but the test is fundamentally a 100% hit scenario):

BenchmarkDotNet=v0.13.0, OS=Windows 10.0.26100
AMD Ryzen 9 7900X, 1 CPU, 24 logical and 12 physical cores
.NET SDK=9.0.100-preview.4.24215.2
  [Host]     : .NET 9.0.0 (9.0.24.20603), X64 RyuJIT
  DefaultJob : .NET 9.0.0 (9.0.24.21104), X64 RyuJIT


|                  Method |          Mean |        Error |       StdDev | Ratio |  Gen 0 | Gen 1 | Gen 2 | Allocated |
|------------------------ |--------------:|-------------:|-------------:|------:|-------:|------:|------:|----------:|
|     HitDistributedCache | 553,492.01 ns | 5,439.496 ns | 4,821.972 ns | 1.000 |      - |     - |     - |   1,464 B |
|          HitHybridCache |     608.02 ns |    12.098 ns |    29.218 ns | 0.001 | 0.0105 |     - |     - |     176 B |
| HitHybridCacheImmutable |      33.02 ns |     0.664 ns |     1.145 ns | 0.000 |      - |     - |     - |         - |

(yes, callers could add L1 support, but many don't because it is awkward; stampede support is very tricky to get right, so is almost never implemented; the point of HybridCache is to make it easy to get all the right behaviors)

Co-authored-by: Andrew Casey <amcasey@users.noreply.github.com>

Co-authored-by: Brennan <brecon@microsoft.com>

…into marc/hybrid-api

…ent states

…teT.cs Co-authored-by: Safia Abdalla <safia@microsoft.com>

…netcore into marc/hybrid-api-basic

amcasey

This is where I ran out of time for today. Mostly simple questions.

amcasey

Responses to responses.

amcasey · 2024-04-19T20:07:42Z

+            }
+            return value;
+
+            static void Throw() => throw new ObjectDisposedException("The cache item has been recycled before the value was obtained");


The reason the JIT doesn't inline methods with throws is that it wants to keep them on the stack in the event of an exception?

amcasey · 2024-04-19T20:08:35Z

+
+partial class DefaultHybridCache
+{
+    internal readonly struct StampedeKey : IEquatable<StampedeKey>


ToString seems adequate to me.

BrennanConroy · 2024-04-19T19:18:20Z

+
+            // otherwise, either nothing existed - or the thing that already exists can't be joined;
+            // in that case, go ahead and use the state that we invented a moment ago (outside of the lock)
+            _currentOperations[stampedeKey] = stampedeState;


I think there is technically still a race here. Up to you to decide if it's worth fixing though.

The only competition scenario involves the remove loop, but that would mean we also couldn't join with the thing being removed (or we would have exited 5 lines above), which means we're still doing the right thing by running our own, even if we end up being the only caller. I concede there may be some extreme edge case where we end up unnecessarily having a worker, but: our aim here is to reduce that to almost zero; at that point, we're good, IMO

BrennanConroy · 2024-04-19T19:21:14Z

+        {
+            _key = key;
+            _flags = flags;
+            _hashCode = key.GetHashCode() ^ (int)flags;


I pointed to the apisof page because it tells you what frameworks the API exists on and what package you need to use on what framework.

TLDR; there is a package with the API for netfx.

BrennanConroy · 2024-04-19T19:54:06Z

        {
            var reader = new Utf8JsonReader(source);
+#pragma warning disable IDE0079 // unnecessary suppression: TFM-dependent
 #pragma warning disable IL2026, IL3050 // AOT bits


Is this something that will be fixed in future previews?

I believe so, yes

BrennanConroy · 2024-04-19T20:42:56Z

+        // give it a moment for the eviction callback to kick in
+        for (var i = 0; i < 10 && cacheItem.NeedsEvictionCallback; i++)
+        {
+            await Task.Delay(250);


I strongly dislike test code like this (even though I occasionally do it too), any way to make it deterministic instead?

We're at the mercy of MemoryCache's timing on calling the eviction callback, which seems to be non-deterministic - I'm guessing they'r using QUWI. We could make it deterministic by adding an event or something that lets us know when that happens, but that is bending the design a bit far for testing. We could perhaps add a DEBUG-only event that we hook in DEBUG?

amcasey

I don't understand all of it, but I also don't see anything that makes me think we shouldn't merge it.

amcasey · 2024-04-19T21:29:24Z

Ah, I missed that this was auto-merge. Well, I stand by my claim that it was mergeable, but I think a follow-up changing addressing the remaining feedback is in order.

mgravell · 2024-04-20T10:00:08Z

amcasey 13 hours ago
Sorry, I'm going to push since this seems important and I'm still not following it. Are you saying that the built-in byte[].Length will return a negative value if it came from the pool? Did we forcibly set the sign bit somewhere? Or maybe there's some reason this isn't the built-in byte[].Length?

adding comment

mgravell · 2024-04-20T10:02:56Z

The reason the JIT doesn't inline methods with throws is that it wants to keep them on the stack in the event of an exception?

I can't speak for the "why", but: it does reduce the inlineable method size to the point where the main thing (the non-exceptional case) can often be inlined; I believe modern JIT can also see methods that always / only throw, and never try inlining, which again: can leave the non-exceptional cases more inlineable; we don't really care about the stack frames - it is more about body size, IIRC

mgravell · 2024-04-20T10:08:24Z

I pointed to the apisof page because it tells you what frameworks the API exists on and what package you need to use on what framework.

TLDR; there is a package with the API for netfx.

@BrennanConroy this really doesn't warrant an extra package; heck, we could probably remove the flags part of the hash and just use the key's hashcode, and we'd be absolutely fine

mgravell · 2024-04-22T08:46:21Z

@BrennanConroy race condition concerns addressed here: eeaf857

mgravell and others added 29 commits April 10, 2024 19:21

initial API cut (post review)

35e9cea

basic API test

12b3a0e

prove that the API can be configured

c5d28d4

demonstrate serializer/factory configuration working

20e1cb2

move to NuGet only to make the build happier

4379647

defer on trimming

d382a7a

PR review comments

e8bc9a9

tyop

7edddb1

Update src/Caching/Hybrid/src/Runtime/IsExternalInit.cs

dc622a8

Co-authored-by: Andrew Casey <amcasey@users.noreply.github.com>

return a leased array on netfx

4920182

prefer ForEach to Walk

8841e97

Update src/Caching/Hybrid/src/Internal/InbuiltTypeSerializer.cs

6cbe02e

Co-authored-by: Brennan <brecon@microsoft.com>

Update src/Caching/Hybrid/src/Internal/DefaultJsonSerializerFactory.cs

0c0d589

Co-authored-by: Brennan <brecon@microsoft.com>

Update src/Caching/Hybrid/src/Internal/InbuiltTypeSerializer.cs

ef0ad56

Co-authored-by: Brennan <brecon@microsoft.com>

comment nits

e28f316

Merge branch 'marc/hybrid-api' of https://github.com/dotnet/aspnetcore …

fbedef4

…into marc/hybrid-api

use TimeProvider throughout

b49151e

more nits

c2326fd

regen projects list

29dcc2e

TryAdd

4ddba7f

clarify intent of null on Remove{Tags|Keys}Async

427601c

basic stampede infrastructure

cef964f

streamline the non-canceled scenario

6f572f3

L2

a41175c

L1

82a34e3

L2 tests

e3a9173

mutable/immutable tests

373aaa8

build warnings

8c1cc9e

implement SetValueAsync

40e2c47

ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Apr 15, 2024

mgravell and others added 11 commits April 18, 2024 12:41

use standard Try* pattern

6a25601

redundant ?.

17383c4

TCS: async completions

14bb5fc

use double-checked lock to avoid race conditions creating two concurr…

e6b2cca

…ent states

move state above methods; use NullLogger

96d75fa

Update src/Caching/Hybrid/src/Internal/DefaultHybridCache.StampedeSta…

366bd82

…teT.cs Co-authored-by: Safia Abdalla <safia@microsoft.com>

add proper buffer recycling of buffers inside cache items

256581b

Merge branch 'marc/hybrid-api-basic' of https://github.com/dotnet/asp…

45d31b3

…netcore into marc/hybrid-api-basic

prefer HashCode when possible

3de225e

Merge branch 'main' into marc/hybrid-api-basic

c796f52

fix bad merge (need longer name for disambiguation)

ce7ec84

amcasey reviewed Apr 18, 2024

View reviewed changes

mgravell added 4 commits April 19, 2024 10:07

look into CI failure; timing

0489dce

nits

e5a080f

disposable support, and fix IDE analyzer nits

de2a4b0

remove IOptions<T> for now

d4133d1

mgravell enabled auto-merge (squash) April 19, 2024 16:16

amcasey reviewed Apr 19, 2024

View reviewed changes

BrennanConroy reviewed Apr 19, 2024

View reviewed changes

amcasey approved these changes Apr 19, 2024

View reviewed changes

mgravell merged commit 8660e3a into main Apr 19, 2024

mgravell deleted the marc/hybrid-api-basic branch April 19, 2024 21:24

dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 19, 2024

mgravell mentioned this pull request Apr 20, 2024

HybridCache - minor post-PR fixes #55251

Merged

mgravell mentioned this pull request Apr 22, 2024

Epic: IDistributedCache updates in .NET 9 #53255

Closed

Conversation

mgravell commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Implement minimal implementation of HybridCache

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgravell Apr 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amcasey commented Apr 19, 2024

Uh oh!

mgravell commented Apr 20, 2024

Uh oh!

mgravell commented Apr 20, 2024

Uh oh!

mgravell commented Apr 20, 2024

Uh oh!

mgravell commented Apr 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mgravell commented Apr 15, 2024 •

edited

Loading

Implement minimal implementation of `HybridCache`

mgravell Apr 20, 2024 •

edited

Loading