peachpiecompiler / peachpie

PeachPie - the PHP compiler and runtime for .NET and .NET Core
https://www.peachpie.io
Apache License 2.0
2.31k stars 201 forks source link

Feature/kendallb/mysql wrapping performance #1030

Closed kendallb closed 2 years ago

kendallb commented 2 years ago

Improve runtime performance of unwrapping MySQL connections, readers and commands.

I am not sure why you did it this way with a ConcurrentDictionary?

var method = lazyCache.GetOrAdd( @object.GetType(), type => type.GetMethod(getterMethodName, BindingFlags.Instance | BindingFlags.Public)) ?? throw new InvalidOperationException($"'{getterMethodName}' method could not be resolved for {@object.GetType().Name}.");

As far as I can tell from reading the code there are three separater concurrent dictionaries, one for each of the types we are resolving. While it works, I do question if we want that much overhead at runtime every time we need to cast a value that has been wrapped? While the code will early out nicely when the type is not wrapped, if it is wrapped it is always going to hit the concurrent dictionary. That is going to be much slower than my original code for two reasons a) the concurrent dictionary is going to use locking to ensure exclusive access (expensive) and b) its going to be using hashing to look up the value. The dictionary is only going to have one value in it per type (as there are three dictionaries; it is not shared).

My original code simply uses a static global variable for the getting method, which is very fast to use. The only real overhead is the invoking of the function to get the mapped value, but that should be way less than a concurrent dictionary? And it also avoids any locking. Sure, you can potentially get a race condition when multiple threads need to map a connection for the first time, but that's OK because I always write my code to assume a last in wins. So if 10 threads all hit the code at the same time, they will all perform the same reflection work to get the appropriate method, do the cast and then update the global variable. It does not really matter which one happens to get in last because the value will always get updated correctly in the end. I am pretty positive that for references, .net guarantees atomic updates to those values, so you don't need to worry about one thread happening to read another threads half baked value.

Finally the dictionary is not initialized (I assume to avoid the overhead of the storage at runtime). If you use a static global variable, you won't need to bother with that since you are just managing the reference?

Update to support the coming change for wrapped commands in MiniProfiler:

https://github.com/MiniProfiler/dotnet/issues/599

jakubmisek commented 2 years ago

Because @object.GetType() may differ. If it would be meant only as a workaround for the MiniProfiler, a single variable would be enough tho.