[FEATURE] AsyncKeyedLock #134
Comments
|
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 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 Can you point me to a potential advantage in using your lib? Thanks! |
|
Closing for now since there has not been a response. |
|
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 |
|
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. |
|
@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. |
|
Hi @MarkCiliaVincenti , I have some good news about this! Since I'm about to get to the big 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. |
|
I'm not entirely sure what you mean Jody. Do you need some changes to the PR? |
|
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? |
Wait, which PR? Is there one? |
I thought about you creating your own package, just because I thought you would've preferred to do that to have control. I'll update you soon then. |
|
Hi @MarkCiliaVincenti , I just release v0.25.0 🥳 It is now possible to create custom I quickly tried to create one and benchmarked it, with these results:
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! |
|
Can you please show me the code you used for benchmarking? Did you enable pooling? |
|
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. |
|
Tried a basic experiment (no logging so far and pool size is 20, probably not ideal)
|
|
With a pool size of 100 instead
|
|
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
|
|
Updated https://github.com/MarkCiliaVincenti/FusionCache/tree/AsyncKeyedLocker New benchmarks:
|
|
Thanks @MarkCiliaVincenti , I'll take a look for sure and will let you know! |
|
Results from a more powerful PC (I usually like to use my older laptop for benchmarks):
|
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! |
|
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<string>();
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
<#134 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AF7U7YEHAVAVLSSVXQH3TY3YSCLP3AVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGQ3TGMJXGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
|
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<string>();
>
> 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
> <#134 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AF7U7YEHAVAVLSSVXQH3TY3YSCLP3AVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGQ3TGMJXGY>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
|
Also didn't realise the string is key so maybe you can get better
performance using AsyncKeyedLocker<string> 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<string>();
>>
>> 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
>> <#134 (comment)>,
>> or unsubscribe
>> <https://github.com/notifications/unsubscribe-auth/AF7U7YEHAVAVLSSVXQH3TY3YSCLP3AVCNFSM6AAAAAAWYV67O2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMRWGQ3TGMJXGY>
>> .
>> You are receiving this because you were mentioned.Message ID:
>> ***@***.***>
>>
>
|
Yep, in my tests I've in fact used |
|
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? |
You mean 1000 accessors and 100 key count? Sure thing, I'll run it now and will report back with the results.
Yes, that's the idea I meant here:
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? |
|
Sounds good! |
|
Here are the results:
Thougts? |
|
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) |
|
Try setting PoolSize and PoolInitialFill to Environment.ProcessorCount * 2 on startup. |
You previously said that:
Therefore I did not set any special values.
I initialize FusionCache, including the memory locker, which in turn includes the AsyncKeyedLocker instance, before the benchmark code, so it's not included. |
|
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 |
Usually I benchmark with the default settings, otherwise it would feel like cheating (imho, it's a personal thing).
I'm doing this right now, results asap. |
|
Here are the results:
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! |
|
Hi @MarkCiliaVincenti , while doing some final benchmarking yesterday I stumbled upon a couple of runs where 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. |
|
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. |
|
Hi @MarkCiliaVincenti , so here's the output of the benchmark where it got stuck: This is memory locker implementation: This is the benchmark code: Let me know if it happens on your machine too, and if you can spot the problem. Thanks! |
|
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. |
Of course, whenever you can Mark 🙂
Will try again tomorrow and update you. Thanks. |
|
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. can be simplified to this: and: to: |
|
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: 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. |
|
Some other notes:
|
|
I also understand that you're probably used with receiving either an object or a null, and probably deciding whether to run the code or not based on checking if the return was null. But you can't have it that way. That's going to break the pooling of my library and others (if they have pooling). So you'll need to do some refactoring. Usually the way you'd use AsyncKeyedLock is to call LockAsync and you'd get the AsyncKeyedLockTimeoutReleaser instance back, which has the boolean EnteredSemaphore in it, and you check against that. |
|
Hi @MarkCiliaVincenti , yesterday I went to sleep, quite a week to recover from.
If you mean the return value for the For example the default implementation uses a And yes, I know that
I didn't know that about AsyncKeyedLock, thanks, I'm getting to know it better.
It depends on the user's usage, but in general I would not expect to frequently have a non-infinite timeout.
One thing I'd like to understand though is: what is the use-case for returning a special Even because, and this is my other question, what can I do with a The only think I can think of is a rationale like "I want to use the For example this works fine: using (var foo = (IDisposable?)null) {
Console.WriteLine($"foo is null: {foo is null}");
}I'm positive I'm missing something here, maybe some scenarios or design consideration: can you help me understand? And to be clear: I'm not suggesting to switch to a different design, change your library, or else I'm trying to understand better. Btw I'm updating the code per your suggestions, will update with the results. Thanks! |
|
The reason is simple actually. There's always a reference counter. It needs to be decremented by 1. If it's zero it needs to be removed from the dictionary. Furthermore, the object needs to always be returned to the pool. If I just returned null I can't do anything about this and the dictionary is not cleaned and the pool will not get replenished. |
Sorry I'm not sure I understand, or maybe I expressed myself badly. If the only thing I can do with a I think this summarizes the main point I'd like to understand:
Thanks! |
|
That would introduce a race condition. Imagine thread A is locked on key ABC Furthermore if B puts the object in the pool before it's finished processing there's a chance that it gets picked up by another request whilst B is still processing. A big recipe for disaster. These operations can't be done preemptively. |
|
Getting back to this:
Based on your next notes, I also need to handle infinite timeout and lock not getting acquired (because timeout) + immediate release (if I got that part right), so it should be changed to this: public async ValueTask<object?> AcquireLockAsync(string cacheName, string cacheInstanceId, string key, string operationId, TimeSpan timeout, ILogger? logger, CancellationToken token)
{
IDisposable? releaser;
if (timeout == Timeout.InfiniteTimeSpan)
{
releaser = await _locker.LockAsync(key, token).ConfigureAwait(false);
}
else
{
var tmp = await _locker.LockAsync(key, timeout, token).ConfigureAwait(false);
if (tmp.EnteredSemaphore)
{
releaser = tmp;
}
else
{
tmp.Dispose();
releaser = null;
}
}
return releaser;
}Is this correct?
Since I have to call 2 different overloads based on the timeout (infinite or not) I would then need to check for both return types, correct? |
I have to think about this race condition a bit, will upadte later. |
|
As to your penultimate comment, no, you cannot dispose just because you didn't enter the semaphoreslim. This will 100% create race conditions and bugs. You want correctness. |
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.
The text was updated successfully, but these errors were encountered: