ZiggyCreatures / FusionCache

FusionCache is an easy to use, fast and robust hybrid cache with advanced resiliency features.
MIT License
1.8k stars 94 forks source link

[FEATURE] AsyncKeyedLock #134

Open MarkCiliaVincenti opened 1 year ago

MarkCiliaVincenti commented 1 year ago

I am the author of the AsyncKeyedLock library and I believe that this library would benefit from depending on it. If you're interested, I can make a PR.

jodydonetti commented 1 year ago

Hi @MarkCiliaVincenti and thanks for your suggestion.

I took a quick look at your library and it's interesting, but FusionCache already has a similar mechanism in the form of various implementations of what I called a core "reactor".

The base abstraction is the IFusionCacheReactor interface which models a way to acquire/release a lock, with support for both sync and async programming models, timeouts, etc: you can see here I've been playing from the very beginning with different implementations, one of which (the "standard") uses a very similar technique as yours.

Currently I've yet to open up the whole reactor thing to the public: I mean, the source is of course already available (not all of them), but I'm not currently exposing the various impls as public because it's an area still under active research with a couple more experiments I'm doing on a separate local branch offline, not yet ready to share.

Can you point me to a potential advantage in using your lib?

Thanks!

jodydonetti commented 1 year ago

Closing for now since there has not been a response. Will reopen in case something new will came up.

MarkCiliaVincenti commented 1 year ago

Hi, I saw https://github.com/ZiggyCreatures/FusionCache/blob/main/src/ZiggyCreatures.FusionCache/Reactors/FusionCacheReactorUnboundedWithPool.cs now

The library is more or less the same techniques. I'm surprised though why you used striping for the pool and then still used a dictionary. Why not just use lock striping? https://github.com/MarkCiliaVincenti/AsyncKeyedLock/blob/master/AsyncKeyedLock/StripedAsyncKeyedLocker.cs

jodydonetti commented 1 year ago

Hi @MarkCiliaVincenti , the specific reactor you pointed at (unbounded with pool) is not using a lock striping approach: it is using a normal dictionary (not a concurrent one) with a semaphore-per-key (so 1:1, not striped).

The small lock pool you see is there just to access the semaphore dictionary when it's time to add a new one, instead of using a concurrent dictionary (and after an initial read phase, so the classic double-checked locking).

It is just one of the many experiments I've played with here and there, and not the real one currently used inside FusionCache.

MarkCiliaVincenti commented 10 months ago

@jodydonetti, AsyncKeyedLock has matured since we last discussed and you may be interested in adding a dependency to it. I can help with this if you're interested.

jodydonetti commented 9 months ago

Hi @MarkCiliaVincenti , I have some good news about this!

Since I'm about to get to the big v1.0 release, I'm also finally opening up the internals related to the locking part: because of this with the next release you'll be able to create your own implementation of the memory lock and provide that as a third-party package, just like an implementation of a backplane or something like that.

In this way users will be able to pick and choose whichever they prefer.

Makes sense? Hope you like this.

Will update soon with some details.

MarkCiliaVincenti commented 9 months ago

I'm not entirely sure what you mean Jody. Do you need some changes to the PR?

MarkCiliaVincenti commented 9 months ago

Ah I think I get it now. But that would mean needing to create my own package if I understand correctly, or will it be an official FusionCache package which you'd link to from a readme where you explain pros and cons?

jodydonetti commented 9 months ago

I'm not entirely sure what you mean Jody. Do you need some changes to the PR?

Wait, which PR? Is there one?

jodydonetti commented 9 months ago

Ah I think I get it now. But that would mean needing to create my own package if I understand correctly, or will it be an official FusionCache package which you'd link to from a readme where you explain pros and cons?

I thought about you creating your own package, just because I thought you would've preferred to do that to have control. Having said that, I can even create it myself like the serialization adapters I already created for the common serialization libs, no worries.

I'll update you soon then.

jodydonetti commented 9 months ago

Hi @MarkCiliaVincenti , I just release v0.25.0 🥳

It is now possible to create custom IFusionCacheMemoryLocker designs and implementations, like one with your own AsyncKeyedLock.

I quickly tried to create one and benchmarked it, with these results:

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 100 1 108.3 ms 0.43 ms 0.40 ms 108.8 ms 1.00 0.00 - - - 1773.63 KB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 1 108.2 ms 0.37 ms 0.34 ms 108.6 ms 1.00 0.01 - - - 1558.52 KB 0.88
FusionCache 100 10 100 10 108.7 ms 0.59 ms 0.55 ms 109.5 ms 1.00 0.00 400.0000 200.0000 - 6375.15 KB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 10 108.7 ms 0.43 ms 0.40 ms 109.1 ms 1.00 0.01 400.0000 200.0000 - 6165.36 KB 0.97
FusionCache 100 1000 100 1 231.7 ms 4.63 ms 7.87 ms 244.7 ms 1.00 0.00 10000.0000 9000.0000 1000.0000 112276.46 KB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 1 244.2 ms 4.84 ms 10.92 ms 261.5 ms 1.06 0.06 11000.0000 10000.0000 1000.0000 123452.45 KB 1.10
FusionCache 100 1000 100 10 507.8 ms 10.04 ms 25.38 ms 542.0 ms 1.00 0.00 34000.0000 23000.0000 6000.0000 393337.04 KB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 10 534.0 ms 14.71 ms 43.38 ms 592.5 ms 1.06 0.11 34000.0000 23000.0000 5000.0000 403604.58 KB 1.03

As you can see the numbers are roughly the same (I've used the standard one, not the striped), and with higher numbers of keys, accessors and so on it's using a little more resources so I don't feel like taking the extra burden of creating and maintaining a separated package myself.

Having said that you are now free to play with it if interested, and maybe come up with an even better one.

Thanks!

MarkCiliaVincenti commented 9 months ago

Can you please show me the code you used for benchmarking? Did you enable pooling?

MarkCiliaVincenti commented 9 months ago

Also one thing I'd change from your public benchmarks is to not include the cache creation parts within the benchmark itself but in setup methods so that you measure the operation parts.

MarkCiliaVincenti commented 9 months ago

Tried a basic experiment (no logging so far and pool size is 20, probably not ideal)

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 100 1 121.7 ms 1.96 ms 1.83 ms 124.3 ms 1.00 0.00 600.0000 200.0000 - 2.6 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 1 119.2 ms 2.27 ms 2.43 ms 122.8 ms 0.98 0.03 400.0000 200.0000 - 2.22 MB 0.86
FusionCache 100 10 100 10 132.1 ms 2.64 ms 3.43 ms 137.7 ms 1.00 0.00 1500.0000 750.0000 - 7.28 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 10 130.6 ms 2.59 ms 3.72 ms 136.2 ms 0.99 0.04 1750.0000 500.0000 - 6.9 MB 0.95
FusionCache 100 1000 100 1 1,332.3 ms 26.31 ms 53.15 ms 1,406.6 ms 1.00 0.00 34000.0000 13000.0000 2000.0000 181.32 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 1 1,397.1 ms 27.60 ms 69.74 ms 1,515.0 ms 1.06 0.08 34000.0000 13000.0000 3000.0000 178.79 MB 0.99
FusionCache 100 1000 100 10 2,210.3 ms 40.58 ms 37.96 ms 2,275.1 ms 1.00 0.00 111000.0000 30000.0000 2000.0000 578.72 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 10 2,153.9 ms 40.60 ms 67.84 ms 2,258.9 ms 0.98 0.04 78000.0000 26000.0000 5000.0000 459.4 MB 0.79
MarkCiliaVincenti commented 9 months ago

With a pool size of 100 instead

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 100 1 121.3 ms 1.93 ms 1.80 ms 123.3 ms 1.00 0.00 600.0000 200.0000 - 2.6 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 1 119.7 ms 2.27 ms 2.23 ms 122.8 ms 0.99 0.03 600.0000 200.0000 - 2.21 MB 0.85
FusionCache 100 10 100 10 131.8 ms 2.60 ms 3.89 ms 137.5 ms 1.00 0.00 1250.0000 500.0000 - 7.25 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 10 129.4 ms 2.14 ms 2.78 ms 133.6 ms 0.99 0.04 1250.0000 500.0000 - 6.89 MB 0.95
FusionCache 100 1000 100 1 1,403.6 ms 26.98 ms 67.19 ms 1,525.5 ms 1.00 0.00 34000.0000 13000.0000 2000.0000 187.32 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 1 1,369.7 ms 27.15 ms 52.95 ms 1,440.5 ms 0.97 0.07 33000.0000 13000.0000 2000.0000 178.27 MB 0.95
FusionCache 100 1000 100 10 2,148.5 ms 42.16 ms 60.47 ms 2,228.1 ms 1.00 0.00 80000.0000 25000.0000 6000.0000 456.96 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 10 2,096.3 ms 40.93 ms 66.10 ms 2,193.8 ms 0.98 0.04 76000.0000 23000.0000 3000.0000 463.16 MB 1.01
MarkCiliaVincenti commented 9 months ago

And after adding more overloaded methods and adding logging like the other lockers.

You can check for yourself at https://github.com/MarkCiliaVincenti/FusionCache/tree/AsyncKeyedLocker

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 100 1 121.1 ms 1.07 ms 1.00 ms 122.2 ms 1.00 0.00 600.0000 200.0000 - 2.58 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 1 119.4 ms 2.30 ms 2.25 ms 122.7 ms 0.99 0.02 600.0000 200.0000 - 2.24 MB 0.87
FusionCache 100 10 100 10 132.7 ms 2.60 ms 2.78 ms 137.4 ms 1.00 0.00 1500.0000 750.0000 - 7.29 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 10 128.9 ms 2.46 ms 2.74 ms 133.1 ms 0.97 0.03 1000.0000 500.0000 - 6.89 MB 0.95
FusionCache 100 1000 100 1 1,383.6 ms 27.58 ms 72.65 ms 1,484.1 ms 1.00 0.00 33000.0000 12000.0000 2000.0000 185.41 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 1 1,386.0 ms 27.52 ms 60.97 ms 1,502.4 ms 1.01 0.08 35000.0000 14000.0000 3000.0000 178.14 MB 0.96
FusionCache 100 1000 100 10 2,178.4 ms 39.43 ms 45.40 ms 2,230.2 ms 1.00 0.00 79000.0000 27000.0000 5000.0000 470.72 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 10 2,163.7 ms 43.24 ms 86.35 ms 2,304.8 ms 1.01 0.04 77000.0000 24000.0000 3000.0000 464.65 MB 0.99
MarkCiliaVincenti commented 9 months ago

Updated https://github.com/MarkCiliaVincenti/FusionCache/tree/AsyncKeyedLocker

New benchmarks: Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 100 10 132.9 ms 2.38 ms 2.74 ms 137.0 ms 1.00 0.00 1500.0000 750.0000 - 7.25 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 10 129.7 ms 2.48 ms 2.76 ms 134.0 ms 0.98 0.03 1250.0000 500.0000 - 6.91 MB 0.95
FusionCache 100 10 1000 10 342.7 ms 6.48 ms 6.06 ms 351.6 ms 1.00 0.00 16000.0000 7000.0000 3000.0000 70.75 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 1000 10 336.2 ms 6.71 ms 14.73 ms 359.4 ms 0.98 0.04 13000.0000 5000.0000 2000.0000 69.49 MB 0.98
FusionCache 100 1000 100 10 2,179.5 ms 43.46 ms 78.38 ms 2,306.0 ms 1.00 0.00 78000.0000 23000.0000 3000.0000 467.44 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 10 2,153.1 ms 42.33 ms 80.54 ms 2,268.5 ms 0.99 0.05 76000.0000 23000.0000 3000.0000 465.63 MB 1.00
FusionCache 100 1000 1000 10 23,424.7 ms 462.26 ms 601.07 ms 24,193.4 ms 1.00 0.00 757000.0000 253000.0000 4000.0000 4532.05 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 1000 10 23,199.7 ms 463.65 ms 664.95 ms 24,350.6 ms 0.99 0.03 745000.0000 263000.0000 9000.0000 4501.53 MB 0.99
jodydonetti commented 9 months ago

Thanks @MarkCiliaVincenti , I'll take a look for sure and will let you know!

MarkCiliaVincenti commented 9 months ago

Results from a more powerful PC (I usually like to use my older laptop for benchmarks):

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 100 10 125.6 ms 2.48 ms 3.04 ms 130.5 ms 1.00 0.00 400.0000 200.0000 - 7.36 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 100 10 122.4 ms 1.59 ms 1.33 ms 123.6 ms 0.97 0.03 250.0000 - - 6.98 MB 0.95
FusionCache 100 10 1000 10 244.5 ms 4.40 ms 4.12 ms 249.6 ms 1.00 0.00 3500.0000 1500.0000 500.0000 71.05 MB 1.00
FusionCacheAsyncKeyedLocker 100 10 1000 10 239.4 ms 3.75 ms 3.32 ms 243.2 ms 0.98 0.02 4000.0000 1666.6667 666.6667 70.07 MB 0.99
FusionCache 100 1000 100 10 1,155.7 ms 23.05 ms 46.03 ms 1,246.0 ms 1.00 0.00 55000.0000 19000.0000 3000.0000 946.66 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 100 10 1,166.2 ms 23.32 ms 28.64 ms 1,200.1 ms 1.00 0.06 37000.0000 13000.0000 4000.0000 639.07 MB 0.68
FusionCache 100 1000 1000 10 9,461.7 ms 208.04 ms 606.85 ms 10,653.9 ms 1.00 0.00 306000.0000 115000.0000 11000.0000 5549.55 MB 1.00
FusionCacheAsyncKeyedLocker 100 1000 1000 10 9,634.1 ms 190.99 ms 522.82 ms 10,610.1 ms 1.01 0.06 281000.0000 120000.0000 10000.0000 5165.03 MB 0.93
jodydonetti commented 9 months ago

Did you enable pooling?

I used the defaults with just:

var asyncKeyedLocker = new AsyncKeyedLocker<string>();

I've read the wiki about pooling, but I'm not 100% sure about the reasoning behind enabling pooling or not, and in that case how to pick the various options (size, fill).

Can you elaborate a little bit more about these details? Thanks!

MarkCiliaVincenti commented 9 months ago

Think of initial fill as the minimum number of semaphoreslim objects created and pool size as the maximum number of semaphoreslim objects that will remain permanently in memory.

If the pool is empty, an object is created (as if there was no pooling). Whenever an object is going to be discarded it tries to see if there's empty space in the pool and if so adds it there so that it can be reused.

On Mon, 5 Feb 2024, 09:42 Jody Donetti, @.***> wrote:

Did you enable pooling?

I used the defaults with just:

var asyncKeyedLocker = new AsyncKeyedLocker();

I've read the wiki https://github.com/MarkCiliaVincenti/AsyncKeyedLock/wiki/How-to-use-AsyncKeyedLocker#pooling about pooling, but I'm not 100% sure about the reasoning behind enabling pooling or not, and in that case how to pick the various options (size, fill).

Can you elaborate a little bit more about these details? Thanks!

— Reply to this email directly, view it on GitHub https://github.com/ZiggyCreatures/FusionCache/issues/134#issuecomment-1926473176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7U7YEHAVAVLSSVXQH3TY3YSCLP3AVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGQ3TGMJXGY . You are receiving this because you were mentioned.Message ID: @.***>

MarkCiliaVincenti commented 9 months ago

So initial fill of 1 and pool size of 20 for example means that it creates 1 at startup which remains in memory. If you never have concurrent keys being used, then it will be enough and never create additional semaphores.

On Mon, 5 Feb 2024, 13:41 Mark Cilia Vincenti, @.***> wrote:

Think of initial fill as the minimum number of semaphoreslim objects created and pool size as the maximum number of semaphoreslim objects that will remain permanently in memory.

If the pool is empty, an object is created (as if there was no pooling). Whenever an object is going to be discarded it tries to see if there's empty space in the pool and if so adds it there so that it can be reused.

On Mon, 5 Feb 2024, 09:42 Jody Donetti, @.***> wrote:

Did you enable pooling?

I used the defaults with just:

var asyncKeyedLocker = new AsyncKeyedLocker();

I've read the wiki https://github.com/MarkCiliaVincenti/AsyncKeyedLock/wiki/How-to-use-AsyncKeyedLocker#pooling about pooling, but I'm not 100% sure about the reasoning behind enabling pooling or not, and in that case how to pick the various options (size, fill).

Can you elaborate a little bit more about these details? Thanks!

— Reply to this email directly, view it on GitHub https://github.com/ZiggyCreatures/FusionCache/issues/134#issuecomment-1926473176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7U7YEHAVAVLSSVXQH3TY3YSCLP3AVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGQ3TGMJXGY . You are receiving this because you were mentioned.Message ID: @.***>

MarkCiliaVincenti commented 9 months ago

Also didn't realise the string is key so maybe you can get better performance using AsyncKeyedLocker instead of object

On Mon, 5 Feb 2024, 13:43 Mark Cilia Vincenti, @.***> wrote:

So initial fill of 1 and pool size of 20 for example means that it creates 1 at startup which remains in memory. If you never have concurrent keys being used, then it will be enough and never create additional semaphores.

On Mon, 5 Feb 2024, 13:41 Mark Cilia Vincenti, < @.***> wrote:

Think of initial fill as the minimum number of semaphoreslim objects created and pool size as the maximum number of semaphoreslim objects that will remain permanently in memory.

If the pool is empty, an object is created (as if there was no pooling). Whenever an object is going to be discarded it tries to see if there's empty space in the pool and if so adds it there so that it can be reused.

On Mon, 5 Feb 2024, 09:42 Jody Donetti, @.***> wrote:

Did you enable pooling?

I used the defaults with just:

var asyncKeyedLocker = new AsyncKeyedLocker();

I've read the wiki https://github.com/MarkCiliaVincenti/AsyncKeyedLock/wiki/How-to-use-AsyncKeyedLocker#pooling about pooling, but I'm not 100% sure about the reasoning behind enabling pooling or not, and in that case how to pick the various options (size, fill).

Can you elaborate a little bit more about these details? Thanks!

— Reply to this email directly, view it on GitHub https://github.com/ZiggyCreatures/FusionCache/issues/134#issuecomment-1926473176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7U7YEHAVAVLSSVXQH3TY3YSCLP3AVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGQ3TGMJXGY . You are receiving this because you were mentioned.Message ID: @.***>

jodydonetti commented 9 months ago

Also didn't realise the string is key so maybe you can get better performance using AsyncKeyedLocker instead of object

Yep, in my tests I've in fact used AsyncKeyedLocker<string>.

jodydonetti commented 9 months ago

and pool size as the maximum number of semaphoreslim objects that will remain permanently in memory.

Nitpicking here, but I haven't finished looking at the code in detail yet so I'll ask just to be sure: what would happen if I set a pool size of, say, 10 and at a certain moment there are concurrent requests for 11 different cache keys? Would it still work or would it block/fail somehow?

UPDATE: ok, by re-reading the wiki I think I got it now. Specifically this part:

Whenever a new key is needed, it is taken from the pool (rather than created from scratch). If the pool is empty, a new object is created. This means that the pool will not throttle or limit the number of keys being concurrently processed. Once a key is no longer in use, the AsyncKeyedLockReleaser object is returned back to the pool, unless the pool is already full up.

So basically it can have as many semaphores as needed (so no limits on concurrent requests for different cache leys), it's just that it will not create them from scratch for every new key encountered, but would pick them from the pool of already used ones to avoid GC and stuff.

If this is correct than it's interesting, and somewhat similar to one of the other 3 or 4 designs I played with some time ago but never finished.

Can you confirm to me the explanation is correct? Thanks!

MarkCiliaVincenti commented 9 months ago

It will just get created as if you had no pooling. It will not restrict.

On Mon, 5 Feb 2024, 14:08 Jody Donetti, @.***> wrote:

and pool size as the maximum number of semaphoreslim objects that will remain permanently in memory.

Nitpicking here, but I haven't finished looking at the code in detail yet so I'll ask just to be sure: what would happen if I set a pool size of, say, 10 and at a certain moment there are concurrent requests for 11 different cache keys? Would it still work or would it block/fail somehow?

— Reply to this email directly, view it on GitHub https://github.com/ZiggyCreatures/FusionCache/issues/134#issuecomment-1926963293, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7U7YF3ARM3AIYGCIMDLUDYSDKVXAVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWHE3DGMRZGM . You are receiving this because you were mentioned.Message ID: @.***>

MarkCiliaVincenti commented 9 months ago

So let's say you have a pool size of 20 and you have a peak where there's 30 concurrent keys, that's no problem but when it all quiets down only 20 will remain in memory in the pool.

On Mon, 5 Feb 2024, 14:23 Mark Cilia Vincenti, @.***> wrote:

It will just get created as if you had no pooling. It will not restrict.

On Mon, 5 Feb 2024, 14:08 Jody Donetti, @.***> wrote:

and pool size as the maximum number of semaphoreslim objects that will remain permanently in memory.

Nitpicking here, but I haven't finished looking at the code in detail yet so I'll ask just to be sure: what would happen if I set a pool size of, say, 10 and at a certain moment there are concurrent requests for 11 different cache keys? Would it still work or would it block/fail somehow?

— Reply to this email directly, view it on GitHub https://github.com/ZiggyCreatures/FusionCache/issues/134#issuecomment-1926963293, or unsubscribe https://github.com/notifications/unsubscribe-auth/AF7U7YF3ARM3AIYGCIMDLUDYSDKVXAVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWHE3DGMRZGM . You are receiving this because you were mentioned.Message ID: @.***>

jodydonetti commented 9 months ago

Got it, thanks 👍

jodydonetti commented 2 months ago

Hi @MarkCiliaVincenti , getting back to this.

Some time has passed, and I'd like to give this another shot with a potential extra implementation of IFusionCacheMemoryLocker. Would you be so kind to suggest me a good set of default options to use as a FusionCache memory locker?

I'd like to try again to use your AsyncKeyedLock library as an alternative implementation, but before publishing it officially I'd like to see with my own eyes that it does in fact provide a potential advantage over the default implementation. Should I stick to this comment and this implementation or is there something new to know?

Thanks!

MarkCiliaVincenti commented 2 months ago

What you need to know is that v7.0.0 is out which is slightly more performant and which now doesn't require you to set pooling options since they are set by default to what is tried-and-tested. You can tweak them for your own purpose or you could leave them unset so you can rely on the default settings in case I manage to find some more optimal settings.

jodydonetti commented 2 months ago

What you need to know is that v7.0.0 is out which is slightly more performant and which now doesn't require you to set pooling options since they are set by default to what is tried-and-tested. You can tweak them for your own purpose or you could leave them unset so you can rely on the default settings in case I manage to find some more optimal settings.

Ok, good to know! So I'll take your implementation from here, update the refs and test the results.

Will update later, thanks!

jodydonetti commented 2 months ago

Hi @MarkCiliaVincenti , reporting back.

I created a new project/package, took the old version of the code, updated it to align with FusionCache post v1 and the latest version of the StandardMemoryLocker internals (eg: logging). I also updated the reference to the AsyncKeyedLock package to the latest version (7.0.1).

With this, I proceeded with testing and benchmarking.

Tests

I added that memory locker implementation to the tests, in particular to the Cache Stampede ones (CacheStampedeTests.cs file) and checked that everything was ok, and it was: all tests passed.

Benchmark

Finally I added a benchmark to specifically target this version, and these are the results:

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev Median P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 10 1000 1 2.862 ms 0.0572 ms 0.0908 ms 2.857 ms 2.988 ms 1.00 0.04 468.7500 175.7813 62.5000 5.08 MB 1.00
FusionCache_AsyncKeyed 100 10 1000 1 1.616 ms 0.1859 ms 0.5480 ms 1.306 ms 2.327 ms 0.57 0.19 - - - 5.15 MB 1.01
FusionCache 100 100 1000 1 29.248 ms 0.5697 ms 0.8170 ms 29.146 ms 30.471 ms 1.00 0.04 2906.2500 1625.0000 375.0000 32.81 MB 1.00
FusionCache_AsyncKeyed 100 100 1000 1 15.431 ms 0.3828 ms 1.0607 ms 15.318 ms 17.108 ms 0.53 0.04 2000.0000 1000.0000 - 32.59 MB 0.99
FusionCache 100 1000 1000 1 222.850 ms 4.4383 ms 8.0032 ms 223.083 ms 236.247 ms 1.00 0.05 22666.6667 11666.6667 666.6667 300.99 MB 1.00
FusionCache_AsyncKeyed 100 1000 1000 1 223.029 ms 4.7064 ms 13.8768 ms 223.961 ms 243.264 ms 1.00 0.07 23000.0000 12000.0000 1000.0000 300.42 MB 1.00

Regarding memory usage: with 10, 100 and 1000 accessors the memory is basically the same, sometimes a little less, sometimes a little more. In a way I'm a little surprised, I would've expected less memory to be used.

Regarding cpu time: with 10 and 100 accessors the cpu time is a little over 50% which is definitely nice, while with 1000 accessors it's the same.

Conclusions

I'd like to release the new package, which will act as a bridge to your AsyncKeyedLock package, to give users the choice to pick the one they prefer.

I'll also update the official docs to mention it and put the package reference and everything else, like with the serialization packages (see here and here).

Then of course it will be mentioned in the v1.4.0 release notes, public posts, etc.

Please let me know what do you think.

Thanks!

MarkCiliaVincenti commented 2 months ago

Very interesting, I would like it if you could retest 1000/1000.

I'm still not sure what you will do though, will you create a new package that adds AsyncKeyedLock as a dependency?

jodydonetti commented 2 months ago

Very interesting, I would like it if you could retest 1000/1000.

You mean 1000 accessors and 100 key count? Sure thing, I'll run it now and will report back with the results.

I'm still not sure what you will do though, will you create a new package that adds AsyncKeyedLock as a dependency?

Yes, that's the idea I meant here:

I'd like to release the new package, which will act as a bridge to your AsyncKeyedLock package, to give users the choice to pick the one they prefer.

I'll also update the official docs to mention it and put the package reference and everything else, like with the serialization packages (see here and here).

Then of course it will be mentioned in the v1.4.0 release notes, public posts, etc.

Basically I want to give users the option to use it, but don't force the extra dependency on everyone.

Is it ok for you?

MarkCiliaVincenti commented 2 months ago

Sounds good!

jodydonetti commented 2 months ago

Here are the results:

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 1000 1000 1 230.2 ms 4.52 ms 9.14 ms 247.1 ms 1.00 0.05 22666.6667 11666.6667 666.6667 301.31 MB 1.00
FusionCache_AsyncKeyed 100 1000 1000 1 224.3 ms 4.55 ms 13.34 ms 246.0 ms 0.98 0.07 23000.0000 12000.0000 1000.0000 301.58 MB 1.00
FusionCache 100 1000 1000 10 2,242.5 ms 44.11 ms 52.52 ms 2,308.8 ms 1.00 0.03 224000.0000 114000.0000 4000.0000 3004.4 MB 1.00
FusionCache_AsyncKeyed 100 1000 1000 10 2,214.0 ms 34.80 ms 30.85 ms 2,253.4 ms 0.99 0.03 224000.0000 114000.0000 4000.0000 3004.09 MB 1.00

Thougts?

MarkCiliaVincenti commented 2 months ago

Just curious what pooling settings you're using and whether or not you're including setup as part of the actual benchmarks. Ideally to be fair, any setup should be excluded from the benchmarks. Creating the dictionary, creating the semaphores for the pool, etc. Those will be created at startup (or when there's load if PoolInitialFill is lower than PoolSize)

MarkCiliaVincenti commented 2 months ago

Try setting PoolSize and PoolInitialFill to Environment.ProcessorCount * 2 on startup.

jodydonetti commented 2 months ago

Just curious what pooling settings you're using

You previously said that:

What you need to know is that v7.0.0 is out which is slightly more performant and which now doesn't require you to set pooling options since they are set by default to what is tried-and-tested.

Therefore I did not set any special values.

and whether or not you're including setup as part of the actual benchmarks.

I initialize FusionCache, including the memory locker, which in turn includes the AsyncKeyedLocker instance, before the benchmark code, so it's not included.

MarkCiliaVincenti commented 2 months ago

Yes I did but for benchmarking it's a bit different isn't it? For benchmarking purposes you should always set the same value for PoolInitialFill as PoolSize

jodydonetti commented 2 months ago

Yes I did but for benchmarking it's a bit different isn't it?

Usually I benchmark with the default settings, otherwise it would feel like cheating (imho, it's a personal thing). But I can benchmark with some sort of "optimal settings" to see what we can get.

Try setting PoolSize and PoolInitialFill to Environment.ProcessorCount * 2 on startup.

I'm doing this right now, results asap.

jodydonetti commented 2 months ago

Here are the results:

Method FactoryDurationMs Accessors KeysCount Rounds Mean Error StdDev P95 Ratio RatioSD Gen0 Gen1 Gen2 Allocated Alloc Ratio
FusionCache 100 1000 1000 1 242.4 ms 4.80 ms 10.24 ms 262.2 ms 1.00 0.06 22666.6667 11666.6667 666.6667 300.85 MB 1.00
FusionCache_AsyncKeyed 100 1000 1000 1 221.1 ms 4.68 ms 13.72 ms 244.2 ms 0.91 0.07 22000.0000 11000.0000 1000.0000 299.71 MB 1.00
FusionCache_AsyncKeyedOptimized 100 1000 1000 1 242.0 ms 6.55 ms 19.31 ms 268.9 ms 1.00 0.09 23000.0000 12000.0000 1000.0000 300.54 MB 1.00
FusionCache 100 1000 1000 10 2,305.3 ms 45.25 ms 50.30 ms 2,388.3 ms 1.00 0.03 224000.0000 114000.0000 4000.0000 3005.55 MB 1.00
FusionCache_AsyncKeyed 100 1000 1000 10 2,314.8 ms 44.44 ms 56.20 ms 2,390.9 ms 1.00 0.03 225000.0000 114000.0000 4000.0000 3009.13 MB 1.00
FusionCache_AsyncKeyedOptimized 100 1000 1000 10 2,277.4 ms 43.70 ms 48.57 ms 2,359.6 ms 0.99 0.03 224000.0000 114000.0000 4000.0000 3007.41 MB 1.00

Let me know if you nedd more, but for today I'm good, I need some time off 😅

Will release probably in the next few days.

Thanks!

jodydonetti commented 1 month ago

Hi @MarkCiliaVincenti , while doing some final benchmarking yesterday I stumbled upon a couple of runs where AsyncKeyedLock seems to get stuck with what looked like a deadlock (the BenchmarkDotNet run simply stopped forever).

Did this ever happened to you? Maybe it's a race condition or something.

I'll post here the benchmark code I've used as soon as I can (daily job right now) so you can take a look at that.

MarkCiliaVincenti commented 1 month ago

I haven't had any reports about deadlocks, Jody. During development I sometimes had some issues whilst testing, and every time it turned out to be an issue with the test code rather than the library. Waiting for your code.

jodydonetti commented 1 month ago

Hi @MarkCiliaVincenti , so here's the output of the benchmark where it got stuck:

// ** Remained 1 (50.0%) benchmark(s) to run. Estimated finish 2024-09-13 22:23 (0h 0m from now) **
Setup power plan (GUID: 8c5e7fda-e8bf-4a96-9a85-a6e23a8c635c FriendlyName: High performance)
// **************************
// Benchmark: ParallelComparisonBenchmark.FusionCache_AsyncKeyed: DefaultJob [FactoryDurationMs=100, Accessors=1000, KeysCount=1000, Rounds=1]
// *** Execute ***
// Launch: 1 / 1
// Execute: dotnet 90a949a1-f557-4f8c-a66c-b5bb0df737f1.dll --anonymousPipes 2564 2364 --benchmarkName "ZiggyCreatures.Caching.Fusion.Benchmarks.ParallelComparisonBenchmark.FusionCache_AsyncKeyed(FactoryDurationMs: 100, Accessors: 1000, KeysCount: 1000, Rounds: 1)" --job Default --benchmarkId 1 in C:\Users\indas\source\repos\ZiggyCreatures\FusionCache\benchmarks\ZiggyCreatures.FusionCache.Benchmarks\bin\Release\net8.0\90a949a1-f557-4f8c-a66c-b5bb0df737f1\bin\Release\net8.0
// BeforeAnythingElse

// Benchmark Process Environment Information:
// BenchmarkDotNet v0.14.0
// Runtime=.NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX2
// GC=Concurrent Workstation
// HardwareIntrinsics=AVX2,AES,BMI1,BMI2,FMA,LZCNT,PCLMUL,POPCNT,AvxVnni,SERIALIZE VectorSize=256
// Job: DefaultJob

OverheadJitting  1: 1 op, 125800.00 ns, 125.8000 us/op

This is memory locker implementation:

using System;
using System.Threading;
using System.Threading.Tasks;
using AsyncKeyedLock;
using Microsoft.Extensions.Logging;

namespace ZiggyCreatures.Caching.Fusion.Locking.AsyncKeyedLocking;

/// <summary>
/// An implementation of <see cref="IFusionCacheMemoryLocker"/> based on AsyncKeyedLock.
/// </summary>
public sealed class AsyncKeyedMemoryLocker
    : IFusionCacheMemoryLocker
{
    private readonly AsyncKeyedLocker<string> _locker;

    /// <summary>
    /// Initializes a new instance of the <see cref="AsyncKeyedLocker"/> class.
    /// </summary>
    public AsyncKeyedMemoryLocker(AsyncKeyedLockOptions? options = null)
    {
        options ??= new AsyncKeyedLockOptions();
        _locker = new AsyncKeyedLocker<string>(options);
    }

    /// <inheritdoc/>
    public async ValueTask<object?> AcquireLockAsync(string cacheName, string cacheInstanceId, string key, string operationId, TimeSpan timeout, ILogger? logger, CancellationToken token)
    {
        var releaser = _locker.GetOrAdd(key);
        var acquired = await releaser.SemaphoreSlim.WaitAsync(timeout, token).ConfigureAwait(false);
        return acquired ? releaser : null;
    }

    /// <inheritdoc/>
    public object? AcquireLock(string cacheName, string cacheInstanceId, string key, string operationId, TimeSpan timeout, ILogger? logger, CancellationToken token)
    {
        var releaser = _locker.GetOrAdd(key);
        var acquired = releaser.SemaphoreSlim.Wait(timeout, token);
        return acquired ? releaser : null;
    }

    /// <inheritdoc/>
    public void ReleaseLock(string cacheName, string cacheInstanceId, string key, string operationId, object? lockObj, ILogger? logger)
    {
        if (lockObj is null)
            return;

        try
        {
            ((AsyncKeyedLockReleaser<string>)lockObj).Dispose();
        }
        catch (Exception exc)
        {
            if (logger?.IsEnabled(LogLevel.Warning) ?? false)
                logger.Log(LogLevel.Warning, exc, "FUSION [N={CacheName} I={CacheInstanceId}] (O={CacheOperationId} K={CacheKey}): an error occurred while trying to release a SemaphoreSlim in the memory locker", cacheName, cacheInstanceId, operationId, key);
        }
    }

    // IDISPOSABLE
    private bool disposedValue;
    private void Dispose(bool disposing)
    {
        if (!disposedValue)
        {
            if (disposing)
            {
                if (_locker is not null)
                {
                    _locker.Dispose();
                }
            }

            disposedValue = true;
        }
    }

    /// <inheritdoc/>
    public void Dispose()
    {
        Dispose(disposing: true);
        GC.SuppressFinalize(this);
    }
}

This is the benchmark code:

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Threading.Tasks;
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Columns;
using BenchmarkDotNet.Configs;
using CacheTower;
using CacheTower.Extensions;
using CacheTower.Providers.Memory;
using EasyCaching.Core;
using LazyCache;
using LazyCache.Providers;
using Microsoft.Extensions.Caching.Memory;
using Microsoft.Extensions.DependencyInjection;
using ZiggyCreatures.Caching.Fusion.Locking;
using ZiggyCreatures.Caching.Fusion.Locking.AsyncKeyedLocking;

namespace ZiggyCreatures.Caching.Fusion.Benchmarks;

[MemoryDiagnoser]
[Config(typeof(Config))]
public class ParallelComparisonBenchmark
{
    private class Config : ManualConfig
    {
        public Config()
        {
            AddColumn(
                StatisticColumn.P95
            );
        }
    }

    [Params(100)]
    public int FactoryDurationMs;

    [Params(1_000)]
    public int Accessors;

    [Params(1_000)]
    public int KeysCount;

    [Params(1)]
    public int Rounds;

    private List<string> Keys = null!;
    private TimeSpan CacheDuration = TimeSpan.FromDays(10);
    private IServiceProvider ServiceProvider = null!;

    private FusionCache _FusionCache = null!;
    private FusionCache _FusionCache_AsyncKeyed = null!;
    private FusionCache _FusionCache_AsyncKeyedOptimized = null!;
    private FusionCache _FusionCache_Probabilistic = null!;
    private CacheStack _CacheTower = null!;
    private IEasyCachingProvider _EasyCaching = null!;
    private CachingService _LazyCache = null!;

    [GlobalSetup]
    public void Setup()
    {
        // SETUP KEYS
        Keys = [];
        for (int i = 0; i < KeysCount; i++)
        {
            var key = Guid.NewGuid().ToString("N") + "-" + i.ToString();
            Keys.Add(key);
        }

        // SETUP DI
        var services = new ServiceCollection();
        services.AddEasyCaching(options => { options.UseInMemory("default"); });
        ServiceProvider = services.BuildServiceProvider();

        // SETUP CACHES
        _FusionCache = new FusionCache(new FusionCacheOptions { DefaultEntryOptions = new FusionCacheEntryOptions(CacheDuration) });
        _FusionCache_AsyncKeyed = new FusionCache(new FusionCacheOptions { DefaultEntryOptions = new FusionCacheEntryOptions(CacheDuration) }, memoryLocker: new AsyncKeyedMemoryLocker());
        _FusionCache_AsyncKeyedOptimized = new FusionCache(new FusionCacheOptions { DefaultEntryOptions = new FusionCacheEntryOptions(CacheDuration) }, memoryLocker: new AsyncKeyedMemoryLocker(new AsyncKeyedLock.AsyncKeyedLockOptions()
        {
            PoolSize = Environment.ProcessorCount * 2,
            PoolInitialFill = Environment.ProcessorCount * 2
        }));
        _FusionCache_Probabilistic = new FusionCache(new FusionCacheOptions { DefaultEntryOptions = new FusionCacheEntryOptions(CacheDuration) }, memoryLocker: new ProbabilisticMemoryLocker());
        _CacheTower = new CacheStack(null, new CacheStackOptions([new MemoryCacheLayer()]) { Extensions = [new AutoCleanupExtension(TimeSpan.FromMinutes(5))] });
        _EasyCaching = ServiceProvider.GetRequiredService<IEasyCachingProviderFactory>().GetCachingProvider("default");
        _LazyCache = new CachingService(new MemoryCacheProvider(new MemoryCache(new MemoryCacheOptions())));
        _LazyCache.DefaultCachePolicy = new CacheDefaults { DefaultCacheDurationSeconds = (int)(CacheDuration.TotalSeconds) };
    }

    [GlobalCleanup]
    public void Cleanup()
    {
        _FusionCache.Dispose();
        _FusionCache_AsyncKeyed.Dispose();
        _FusionCache_AsyncKeyedOptimized.Dispose();
        _FusionCache_Probabilistic.Dispose();
        _CacheTower.DisposeAsync().AsTask().Wait();
    }

    [Benchmark(Baseline = true)]
    public async Task FusionCache()
    {
        for (int i = 0; i < Rounds; i++)
        {
            var tasks = new ConcurrentBag<Task>();

            Parallel.ForEach(Keys, key =>
            {
                Parallel.For(0, Accessors, _ =>
                {
                    var t = _FusionCache.GetOrSetAsync<SamplePayload>(
                        key,
                        async ct =>
                        {
                            await Task.Delay(FactoryDurationMs).ConfigureAwait(false);
                            return new SamplePayload();
                        }
                    );
                    tasks.Add(t.AsTask());
                });
            });

            await Task.WhenAll(tasks).ConfigureAwait(false);
        }
    }

    [Benchmark]
    public async Task FusionCache_AsyncKeyed()
    {
        for (int i = 0; i < Rounds; i++)
        {
            var tasks = new ConcurrentBag<Task>();

            Parallel.ForEach(Keys, key =>
            {
                Parallel.For(0, Accessors, _ =>
                {
                    var t = _FusionCache_AsyncKeyed.GetOrSetAsync<SamplePayload>(
                        key,
                        async ct =>
                        {
                            await Task.Delay(FactoryDurationMs).ConfigureAwait(false);
                            return new SamplePayload();
                        }
                    );
                    tasks.Add(t.AsTask());
                });
            });

            await Task.WhenAll(tasks).ConfigureAwait(false);
        }
    }
}

Let me know if it happens on your machine too, and if you can spot the problem.

Thanks!

MarkCiliaVincenti commented 1 month ago

It's late here so I will take a look tomorrow but it would be nice if you could try to reduce as much clutter as possible so we could pinpoint the issue.

jodydonetti commented 1 month ago

It's late here so I will take a look tomorrow

Of course, whenever you can Mark 🙂

it would be nice if you could try to reduce as much clutter as possible so we could pinpoint the issue Eh, I tried to create a MRE, but failed in that, meaning that I think that the problem may be in the combo betweeen FusionCache and AsyncKeyedLock.

Will try again tomorrow and update you.

Thanks.

MarkCiliaVincenti commented 1 month ago

Can you try something real quick? Not in front of a PC so I can't test exactly

First of all see if you can use generics instead of returning object as the releaser.

Secondly since you're passing on a timeout, you shouldn't be doing the if statement checks yourself.

/// <inheritdoc/>
    public async ValueTask<object?> AcquireLockAsync(string cacheName, string cacheInstanceId, string key, string operationId, TimeSpan timeout, ILogger? logger, CancellationToken token)
    {
        var releaser = _locker.GetOrAdd(key);
        var acquired = await releaser.SemaphoreSlim.WaitAsync(timeout, token).ConfigureAwait(false);
        return acquired ? releaser : null;
    }

can be simplified to this:

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
    public async ValueTask<object?> AcquireLockAsync(string cacheName, string cacheInstanceId, string key, string operationId, TimeSpan timeout, ILogger? logger, CancellationToken token)
    {
                return _locker.LockAsync(timeout, token).ConfigureAwait(false);
    }

and:

/// <inheritdoc/>
    public void ReleaseLock(string cacheName, string cacheInstanceId, string key, string operationId, object? lockObj, ILogger? logger)
    {
        if (lockObj is null)
            return;

        try
        {
            ((AsyncKeyedLockReleaser<string>)lockObj).Dispose();
        }
        catch (Exception exc)
        {
            if (logger?.IsEnabled(LogLevel.Warning) ?? false)
                logger.Log(LogLevel.Warning, exc, "FUSION [N={CacheName} I={CacheInstanceId}] (O={CacheOperationId} K={CacheKey}): an error occurred while trying to release a SemaphoreSlim in the memory locker", cacheName, cacheInstanceId, operationId, key);
        }
    }

to:

/// <inheritdoc/>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
    public void ReleaseLock(string cacheName, string cacheInstanceId, string key, string operationId, object? lockObj, ILogger? logger)
    {
        try
        {
            ((AsyncKeyedLockTimeoutReleaser<string>)lockObj).Dispose();
        }
        catch (Exception exc)
        {
            if (logger?.IsEnabled(LogLevel.Warning) ?? false)
                logger.Log(LogLevel.Warning, exc, "FUSION [N={CacheName} I={CacheInstanceId}] (O={CacheOperationId} K={CacheKey}): an error occurred while trying to release a SemaphoreSlim in the memory locker", cacheName, cacheInstanceId, operationId, key);
        }
    }
MarkCiliaVincenti commented 1 month ago

I'm looking at the code I sent you yesterday.

When it failed to enter a semaphore you weren't disposing anything, meaning you skip all this code: https://github.com/MarkCiliaVincenti/AsyncKeyedLock/blob/master/AsyncKeyedLock/AsyncKeyedLockDictionary.cs#L168

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public void ReleaseWithoutSemaphoreRelease(AsyncKeyedLockReleaser<TKey> releaser)
        {
            if (PoolingEnabled)
            {
#if NET9_0_OR_GREATER
                releaser.Lock.Enter();
#else
                Monitor.Enter(releaser);
#endif

                if (releaser.ReferenceCount == 1)
                {
                    TryRemove(releaser.Key, out _);
                    releaser.IsNotInUse = true;
#if NET9_0_OR_GREATER
                    releaser.Lock.Exit();
#else
                    Monitor.Exit(releaser);
#endif
                    _pool.PutObject(releaser);
                    return;
                }
                --releaser.ReferenceCount;
#if NET9_0_OR_GREATER
                releaser.Lock.Exit();
#else
                Monitor.Exit(releaser);
#endif
            }
            else
            {
                Monitor.Enter(releaser);

                if (releaser.ReferenceCount == 1)
                {
                    TryRemove(releaser.Key, out _);
                    releaser.IsNotInUse = true;
                    Monitor.Exit(releaser);
                    return;
                }
                --releaser.ReferenceCount;
                Monitor.Exit(releaser);
            }
        }

the reference count wasn't getting decremented, meaning the item remains in the dictionary and the reference count is out of sync. Furthermore the releaser class (which includes the instance of the semaphore) doesn't get put back into the pool for reuse.

I don't think it should deadlock though, but let's start with this first.

MarkCiliaVincenti commented 1 month ago

Some other notes:

1) try to avoid object in the interface. At least use ValueTask<IDisposable> 2) if the timeout is not being used, don't pass it on. It's extra allocations. A class of AsyncKeyedLockTimeoutReleaser<TKey> needs to be created every time you lock. It's not a heavy one. It only has a boolean (enteredsemaphore) and the instance of AsyncKeyedLockReleaser<TKey> which came from the pool, but it's an allocation anyway. So if you're passing on an infinite timeout for example, it would be better if you call an alternate method for it that doesn't accept timeouts. It would be more performant. Do you always have actual timeouts?