fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Investigate: Special case Lock type in the lock function

Open vzarytovskii opened this issue 1 year ago • 6 comments

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.

vzarytovskii avatar Jun 06 '24 19:06 vzarytovskii

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.

T-Gro avatar Jun 07 '24 08:06 T-Gro

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.

vzarytovskii avatar Jun 07 '24 18:06 vzarytovskii

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.

T-Gro avatar Jun 10 '24 07:06 T-Gro

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.

abonie avatar Jun 17 '24 17:06 abonie

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.

I guess that would probably look something like this?

let inline lock (lockObject : System.Threading.Lock) ([<InlineIfLambda>] action) =
    let mutable scope = lockObject.EnterScope ()
    try action ()
    finally scope.Dispose ()

And if somebody wanted to add support in their own codebase incrementally, I guess they could do something like the following, which uses a few tricks to be source-compatible with both old and new:

// ----------------------------------------------------------------
// Fake types for demo purposes, but with the real shapes.
module System =
    module Threading =
        module Lock =
            [<Struct; System.Runtime.CompilerServices.IsByRefLike>]
            type Scope =
                member _.Dispose () = ()

        [<Sealed>]
        type Lock () =
            member _.EnterScope () = Lock.Scope ()
// End fake types.
// ----------------------------------------------------------------

open System.Threading

[<Sealed; AbstractClass>]
type LockImpl = class end

[<AutoOpen>]
module LockPriority2 =
    type LockImpl with
        static member inline lock (lockObject : Lock) = fun action ->
            let mutable scope = lockObject.EnterScope ()
            try action ()
            finally scope.Dispose ()

[<AutoOpen>]
module LockPriority1 =
    type LockImpl with
        static member inline lock lockObject = fun action ->
            FSharp.Core.Operators.lock lockObject action

open type LockImpl

// Calls the overload equivalent to the existing function.
let f x = lock (obj ()) (fun () -> x)

// Calls the System.Threading.Lock-specific overload.
let g x = lock (Lock ()) (fun () -> x)

// lockObj still resolves to 'a when 'a : not struct by default.
let h lockObj x = lock lockObj (fun () -> x)

brianrourkeboll avatar Jul 24 '24 22:07 brianrourkeboll

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.

I guess that would probably look something like this?


let inline lock (lockObject : System.Threading.Lock) ([<InlineIfLambda>] action) =

    let mutable scope = lockObject.EnterScope ()

    try action ()

    finally scope.Dispose ()

And if somebody wanted to add support in their own codebase incrementally, I guess they could do something like the following, which uses a few tricks to be source-compatible with both old and new:


// ----------------------------------------------------------------

// Fake types for demo purposes, but with the real shapes.

module System =

    module Threading =

        module Lock =

            [<Struct; System.Runtime.CompilerServices.IsByRefLike>]

            type Scope =

                member _.Dispose () = ()



        [<Sealed>]

        type Lock () =

            member _.EnterScope () = Lock.Scope ()

// End fake types.

// ----------------------------------------------------------------



open System.Threading



[<Sealed; AbstractClass>]

type LockImpl = class end



[<AutoOpen>]

module LockPriority2 =

    type LockImpl with

        static member inline lock (lockObject : Lock) = fun action ->

            let mutable scope = lockObject.EnterScope ()

            try action ()

            finally scope.Dispose ()



[<AutoOpen>]

module LockPriority1 =

    type LockImpl with

        static member inline lock lockObject = fun action ->

            FSharp.Core.Operators.lock lockObject action



open type LockImpl



// Calls the overload equivalent to the existing function.

let f x = lock (obj ()) (fun () -> x)



// Calls the System.Threading.Lock-specific overload.

let g x = lock (Lock ()) (fun () -> x)



// lockObj still resolves to 'a when 'a : null by default.

let h lockObj x = lock lockObj (fun () -> x)

I have implemented it in a similar (ish) way in my experimental fslib

vzarytovskii avatar Jul 25 '24 08:07 vzarytovskii