Compiler unnecessarily guards readonly object when locking.

[Zintom]

Version Used: Latest

Steps to Reproduce:

1. Declare a readonly object field _locker and instantiate it with new(); private readonly object _locker = new();
2. Create a method which locks on the _locker variable. public void A() { lock(_locker) { } }
3. Observe the lowered code, you will see that in the method A(), the compiler declares a new variable locker and assigns _locker to it, this is unnecessary as _locker could only ever be re-assigned within the constructor of the class (due to it being readonly), the constructor is synchronous and so even if A() was called from the constructor, it is impossible for _locker to be reassigned whilst A() is executing.

Note: As a fellow programmer has pointed out, this optimization cannot be applied to lock's directly inside a constructor method body.

Sharplab Example

Expected Lowering Behavior:

Actual Lowering Behavior:

[TahirAhmadov]

This can be optimized in methods, but not in ctors, and not in init methods (if and when we get those). PS. To be clear, I think it should be optimized in the OP example.

[Zintom]

This can be optimized in methods, but not in ctors

I'm genuinely curious, why could this assignment not also be elided in constructors?

[TahirAhmadov]

Because readonly fields can be modified in the ctor any number of times.

[Zintom]

Because readonly fields can be modified in the ctor any number of times.

Ahh yes I see now, inside a constructor _locker could be reassigned after the Monitor.Enter and before Monitor.Exit. However like you say, in normal methods my proposal can be applied 🙂

[Youssef1313]

Could _locker be re-assigned with reflection?

[TahirAhmadov]

Could _locker be re-assigned with reflection?

I'm not 100% sure, if I recall correctly, FieldInfo.SetValue prohibits it, and ILEmit used to prohibit it in .NET 4.8, but no longer does in .NET Core "line". Still, even if it's possible to do so, it's not a good reason to avoid optimizing code - once you hack into reassigning readonly fields, you're on your own :)

[Youssef1313]

Anyway, I guess the compiler is behaving per specification:

https://github.com/dotnet/csharpstandard/blob/draft-v7/standard/statements.md#1213-the-lock-statement

Note the following from spec:

except that x is only evaluated once

[TahirAhmadov]

Well it's kind of up to interpretation. In a method which cannot modify readonly fields, evaluating a readonly field is guaranteed to produce the same value, and can be viewed as the result of that one-time evaluation. Still, I think it's worth it to modify the spec, if needed, to introduce this optimization.

[CyrusNajmabadi]

Why is this optimization worth it? Are there any measurements showing this is a problem?

[Zintom]

Why is this optimization worth it? Are there any measurements showing this is a problem?

It's not exactly a high priority, but is an oversight; in my example the guarding of the object reference is functionally unnecessary.

BenchmarkDotNet results after 1 million iterations:

There is a tiny performance benefit as there is one less value being loaded onto the stack.

I imagine this wouldn't be too hard to fix by someone who knows how the compiler works; at my very high level understanding, when the compiler encounters a lock statement it should check whether it is in a constructor, if not, it can elide the guarding.

[CyrusNajmabadi]

in my example the guarding of the object reference is functionally unnecessary.

I'm not sure that's true. I don't know if there is any actual guarantee that these values cannot change. It may be an optimization that breaks something somewhere.

There is a tiny performance benefit as there is one less value being loaded onto the stack.

This looks to be virtually meaningless. Absent a real scenario affected by this, it feels far too risky to me. The current code is entirely correct and has been what we've done for over 20 years. Changing that now without very strong cause feels unnecessary.

[CyrusNajmabadi]

Note:. I would be fine with a jit/runtime optimization here. They are the ones that can be certain they know if a reference will or won't change, and can decide if it's more optimal to read that versus a local.

[jaredpar]

Could _locker be re-assigned with reflection?

Yes it can be.

Still, even if it's possible to do so, it's not a good reason to avoid optimizing code - once you hack into reassigning readonly fields, you're on your own :)

Sorry but disagree here. Reflection, even private reflection, is a 100% supported .NET scenario. It is a fact of the ecosystem that we live in. It cannot be casually ignored like that.

This looks to be virtually meaningless. Absent a real scenario affected by this, it feels far too risky to me. The current code is entirely correct and has been what we've done for over 20 years. Changing that now without very strong cause feels unnecessary.

100% agree. This is the way I think about it as well.