Possible lock(System.Threading.Lock) perf improvement

[peto268]

Reading https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-9/#threading i'm wondering if instead of replacing lock(_lock) with _lock.EnterScope() ... Dispose the compiler shouldn't actually use something like:

_lock.Enter();
try
{
    ...
}
finally
{
    _lock.Exit();
}

This prevents the allocation of the scope struct and in my benchmarks:

[Benchmark]
public void WithLock()
{
    lock (_lock)
    {
        _value++;
    }
}

[Benchmark]
public void WithLock2()
{
    _lock2.Enter();
    try
    {
        _value++;
    }
    finally
    {
        _lock2.Exit();
    }
}

this performs ~5% better.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[jjonescz]

The speclet of the feature addresses this:

This prevents the allocation of the scope struct and in my benchmarks:

Structs are not allocated, so I don't understand this.

this performs ~5% better.

cc @kouvel

[peto268]

From ref structure types (C# reference):

Instances of a ref struct type are allocated on the stack and can't escape to the managed heap.

Thnx for the speclet link, i didn't know about the extra thread lookup in Exit. This makes it even weirder. The perf improvement i observe is ~the same as switching from lock(object) to lock(Lock). Have you tried benchmarking it yourself?

[kouvel]

This makes it even weirder. The perf improvement i observe is ~the same as switching from lock(object) to lock(Lock). Have you tried benchmarking it yourself?

Some processors may be able to prefetch through the thread-local lookup more easily, so the cost of the extra lookup may not show up in some cases. Depending on what is done inside and outside the lock, the perf may vary and can be slightly better with Lock when the overhead of the thread-local lookups show up.

[jilles-sg]

This prevents the allocation of the scope struct and in my benchmarks:

Structs are not allocated, so I don't understand this.

What is meant with "allocation" here is extra unnecessary memory reads and writes and missed optimizations that can result from using structs (for example, foreach on spans has a special expansion rather than using the Span<T>.Enumerator struct). However, Lock.Scope appears to be carefully crafted such that the JIT turns the struct into its two fields (verified using godbolt.org).

Even then, keeping the thread ID between enter and exit is not entirely free and if thread-local storage is very fast on the system it may not be a win.

The Benchmark2 code assumes that _lock2 does not change. The JIT normally does not assume that readonly class instance fields do not change, but perhaps Roslyn can do this. If not, the benchmark is not entirely realistic.

[peto268]

Is the thread check necessary in this case though? Is there a scenario when a thread can change inside a lock block like this?

[jjonescz]

Is the thread check necessary in this case though? Is there a scenario when a thread can change inside a lock block like this?

Lock.Exit can be used in other cases than lock block though, so the check is necessary there. The runtime could expose another API which doesn't check the thread, e.g., ExitUnsafe or something like that, then compiler could use that API:

_lock.Enter();
try
{
    _value++;
}
finally
{
    _lock.ExitUnsafe();
}

[peto268]

Yes, that is what i meant, the compiler can use some kind of unsafe exit.

[jaredpar]

Moving to runtime. This is about the performance of the overall locking system. It's hard for compiler to make a judgement on whether this is or is not a good path forward. Runtime can make that call and if they believe it's worth the change we can sync up on compiler implementation.

[dotnet-policy-service]

Tagging subscribers to this area: @mangod9
See info in area-owners.md if you want to be subscribed.