search

dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.82k stars 773 forks source link

Investigate: Special case Lock type in the lock function #17287

Open vzarytovskii opened 3 weeks ago

vzarytovskii commented 3 weeks ago

As pointed out in https://github.com/dotnet/runtime/pull/103085#issuecomment-2150981294, lock shouldn't use new Lock type in the Monitor, as it might misbehave.

We can't use fslib special syntax for it to have separate implementation, since the type is not in the netstandard.

Two other ways for having its implementation separated for Lock and other types are

  1. Make it a compiler intrinsic, which is probably a "no" for us.
  2. Check the type in runtime, which might impact performance, since we'll have to use reflection for it, as well as might be a breaking change for some of the customers.
T-Gro commented 3 weeks ago

What about a separate library targeting net9 (or "netcurrent" in general), which would be overwriting the lock function and could be statically optimized for Lock type.

vzarytovskii commented 3 weeks ago

What about a separate library targeting net9 (or "netcurrent" in general), which would be overwriting the lock function and could be statically optimized for Lock type.

It can be done, sure (not by us currently though, we don't want additional nuget to support at this moment), the issue with it though is that it will either be only supporting Lock type and not any other or will have to do srtp magic.

I will experiment with it with my fsharp.core drop-in replacement.

T-Gro commented 3 weeks ago

I don't think we can do a combination of statically optimized for "something" and being SRTP at the same time. (or the compiler would have to be taught to do it).

Since the biggest motivation is to keep using lock call inside codebases, and make it do "the right thing", adding separate differently-called functions will not do the trick here.

abonie commented 2 weeks ago

Can't do much about this at the moment. Anyone moving to System.Threading.Lock now should write their own function to work with it.