roslyn
roslyn copied to clipboard
Compiler unnecessarily guards readonly object when locking.
Version Used: Latest
Steps to Reproduce:
- Declare a readonly object field
_locker
and instantiate it withnew();
private readonly object _locker = new();
- Create a method which locks on the
_locker
variable.public void A() { lock(_locker) { } }
- Observe the lowered code, you will see that in the method
A()
, the compiler declares a new variablelocker
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 beingreadonly
), the constructor is synchronous and so even ifA()
was called from the constructor, it is impossible for_locker
to be reassigned whilstA()
is executing.
Note: As a fellow programmer has pointed out, this optimization cannot be applied to lock
's directly inside a constructor method body.
Expected Lowering Behavior:
Actual Lowering Behavior:
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.
This can be optimized in methods, but not in ctors
I'm genuinely curious, why could this assignment not also be elided in constructors?
Because readonly fields can be modified in the ctor any number of times.
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 🙂
Could _locker
be re-assigned with reflection?
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 :)
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
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.
Why is this optimization worth it? Are there any measurements showing this is a problem?
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.
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.
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.
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.