Runtime: Add first class System.Threading.Lock type

[benaadams]

Locking on any class has overhead from the dual role of the syncblock as both lock field and hashcode et al. (e.g. https://github.com/dotnet/runtime/issues/34800)

Adding a first class lock type that didn't allow alternative uses and only acted as a lock would allow for a simpler and faster lock as well as be less ambiguous on type and purpose in source code.

namespace System.Threading
{
    public sealed class Lock
    {
        public void Enter();
        public void Exit();
        public bool TryEnter();
        public bool TryEnter(TimeSpan timeout);

        public override int GetHashCode() => throw new NotSupportedException();
    }
}

Change in usage

private object _lock = new object();

Becomes the clearer

private Lock _lock = new Lock();

[Joe4evr]

Should this type not be sealed?
Also, should there be some in-box analyzers shipped alongside? I can think of:

• Indicate this type's only purpose is to be used for locks
• Suggestion that places where a field is only used to be locked on, that field's type can be changed to this

[benaadams]

Should this type not be sealed?

Yep added, thanks

[stephentoub]

I'm not clear on the extent of this proposal.

You're proposing that Monitor would special-case this with its existing object-based methods? But only some of Monitor's methods?

[benaadams]

I'm not clear on the extent of this proposal.

I'm proposing a type that doesn't need to concern itself with the state of the syncblock so it will only be a thinlock and not worry about handling the flags BIT_SBLK_IS_HASH_OR_SYNCBLKINDEX, BIT_SBLK_IS_HASHCODE, BIT_SBLK_SPIN_LOCK etc

[Wraith2]

So no special treatment anywhere in the runtime it's just setup so that it'll never have a stored hashcode and the syncblock will only ever be used for locking? Though it'd be a pretty niche case where you have a lock on something that has been hashed i'd have thought.

[stephentoub]

I'm proposing a type that doesn't need to concern itself with the state of the syncblock so it will only be a thinlock and not worry about handling the flags

I understand. But how are you proposing interacting with it? Still via Monitor?

[benaadams]

Ah, get you point. Have added other methods.

Would still need to go via monitor currently to work with the lock statement?

[benaadams]

Though it'd be a pretty niche case where you have a lock on something that has been hashed i'd have thought.

Yes; but the handling for the niche case isn't without overhead. So the idea is to introduce a type that drops this overhead by being explicit in only being a lock.

[stephentoub]

Would still need to go via monitor currently to work with the lock statement?

Today, yes, though likely C# could be augmented to recognize the type in the future.

So the idea is to introduce a type that drops this overhead by being explicit in only being a lock.

A potentially much more impactful (and experimental) variation of this would be to actually only support locking on such a type and eliminate the memory overhead incurred for every other object in the system. Until code fully transitioned, locking on other objects would obviously be more expensive as the runtime would need to employ some kind of mapping to a Lock, e.g. https://github.com/dotnet/corert/blob/d0683071605aed956497506ed1aae4d366be0190/src/System.Private.CoreLib/src/System/Threading/Monitor.cs#L28

[GrabYourPitchforks]

/cc @davidwrighton as FYI since we were coincidentally talking about this just the other day.

[davidwrighton]

:) This brings back memories of the early days on the .NET Native project, when we actually implemented this type, and tied it into Monitor (so uses of this Lock type were really great, and other objects had a perf penalty due to the cost of checking for the Lock type. We eventually concluded penalizing all existing Monitor.Lock usage (even slightly) was a poor choice, and reverted back to supporting arbitrary object locking as the first class primitive. One idea I had at the time, which I never explored in detail was the interesting fact that in CoreCLR, there is an empty and unused pointer sized field in every System.Object instance. In theory, if optimizing Monitor performance really only needs a single non-gc tracked pointer field, and most locking occurs on instances of the exact System.Object type, we could use that instead. This would have the same drawbacks a an integrated Lock object for reducing the performance of locking on other object types, but we've been telling customers for years to lock on System.Object instances. Perhaps we could find out if that would be a worthwhile optimization.

If we want to have a lock object type not integrated into Monitor, then there is quite a bit of work required to make sure it integrates with the diagnostics infrastructure and the threading model correctly. (Monitor is an interruptible lock as it uses a COM interruptible wait so that STA frameworks such as WinForms and WPF work correctly. Any other lock that is intended for general use will also need to follow those strictures.)

[stephentoub]

in CoreCLR, there is an empty and unused pointer sized field in every System.Object instance

Why? What prevents us from just removing that?

[benaadams]

interesting fact that in CoreCLR, there is an empty and unused pointer sized field in every System.Object instance.

Is this due to the min size of 12 bytes?

https://github.com/dotnet/runtime/blob/c06810bf654e09f016fe34dd2b4ac31fb73370a1/src/coreclr/src/vm/object.h#L108-L111

This would have the same drawbacks a an integrated Lock object for reducing the performance of locking on other object types, but we've been telling customers for years to lock on System.Object instances.

What would be performance the drawback? Due to an extra _lock.GetType() == typeof(object) check to determine the code path?

[jkotas]

.NET Native project, when we actually implemented this type, and tied it into Monitor

This implementation is still there .

In CoreCLR, the lock can take two paths: The thinlock path that just flips the bit in the object header is used as long as there is no contention that requires waiting or other special situations. Once the thin lock hits the contention, the syncblock (a piece of unmanaged memory) is allocated for the object by the VM, and all lock operations go through the syncblock. Going through syncblock has extra overhead - there are several indirections required to get to syncblock.

In CoreRT, the thinlock path is same as in CoreCLR (the thin lock path was not there in the early .NET Native days). Once the lock hits contention, one of these Lock objects gets attached to the object via ConditionalWeakTable. The Lock object + ConditionalWeakTable is basically a replacement for syncblock as far as Monitor is concerned. I believe that the same strategy would work for CoreCLR too.

min size of 12/24 bytes

Here is where the min size comes into play: The current GC implementation needs to be able replace any chunk of free space with a special fake array to keep the heap walk-able. The object minimum size is a minimum size of this fake array. If we were to get rid of the free space on System.Object, we would need to introduce a special fake object that would be used for free space where the fake array does not fit, and all places in the GC that check for the free space would need to check for both these. Some of these checks are in hot loops that would take a perf hit.

The technically most challenging part for getting rid of object header is dealing with object hashcodes (ie RuntimeHelpers.GetHashCode) during GC object relocation. Many other components depend on these hashcodes, e.g. ConditionalWeakTable. Until we have satisfactory solution for that, the rest does not matter.