Added support of atomic writes for custom value types

[sakno]

Added support of atomic writes for custom value types used as a generic argument of S.C.C.ConcurrentDictionary<TKey, TValue> class. See #65600 discussion.
/cc @danmoseley

[ghost]

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Added support of atomic writes for custom value types used as a generic argument of S.C.C.ConcurrentDictionary<TKey, TValue> class. See #65600 discussion.
/cc @danmoseley

Author:	sakno
Assignees:	-
Labels:	area-System.Collections, community-contribution
Milestone:	-

[omariom]

JITs, AOTs and interpreters don't have to implement copying structs (non-primitives) atomically.

[sakno]

@omariom , probably, I'm wrong but ECMA-335 says

A conforming CLI shall guarantee that read and write access to properly aligned memory
locations no larger than the native word size

Dissasembly of the code confirming my assumption (see linked discussion).

[danmoseley]

@EgorBo Is this safe in coreclr? @vargaz what about Mono?

[EgorBo]

I don't think it's safe, jit can enregister a struct (put its fields into registers) so then when it needs to save it to some memory location it does several movs, e.g. https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACXDKAVzA0YGUaBvGoyGMGjAJYA7LtjSNgAbhoBfGnSZlGAYX6Dh3ZourDGuocRQ8YGbgAoAlKcZ9jj47kYBeRhJgB3HvZOjNiejOSywKGkjEqGxirUSkA

[vargaz]

I don't think mono guarantees atomic moves for small structs either.

[sakno]

@EgorBo , regarding to ConcurrentDictionary case, placing the value to the node is similar to:
https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACXDKAVzA0YGUaBvGoyGMGjAJYA7LtjSNgAbhoBfGnSZlGAYX6Dh3ZourDGuocRQ8YGbgAp9AN2wAbNjACUpxn2OfjuRgC8jI4uMIbGKtRKQA=

which is atomic. Anyway, if it considered as risky then we need more clarification in ECMA standard.

[sakno]

A small recap of the issue from my side:

1. Moves of small value types (<= native int size) are atomic (at least on x64 and x86)
2. Initialization of small value type is not atomic (as mentioned by @EgorBo )
3. ECMA says that write access is atomic. Write access includes initialization and move. In reality, these two actions behave differently.

[EgorBo]

A small recap of the issue from my side:

1. Moves of small value types (<= native int size) are atomic (at least on x64 and x86)
2. Initialization of small value type is not atomic (as mentioned by @EgorBo )

It's not just initialization, e.g.: https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKBuIGYACXDKAVzA0YGUaBvGoyGMGjAJYA7LtjSNgAbhoBfGnSZlGAYX6Dh3ZuVm5Si6sMa6hkrtxgZuACgCUlxn2GvzAN2xRmjAF4DU3NvXzlA5gA6BU9hY0jcENChYgB2OWShFWolIA

at any point jit might decide to promote a structure and then have problems with merging the stores into a single 64 bit one.

[sakno]

For me, as for library writer, this statement in ECMA is completely unclear. That's why I decided to check my assumption in .NET codebase. Now I see the gap between what is stated in the standard and the actual behavior of the runtime. How I should treat this? Is .NET runtime not compliant? Is spec not accurate? Or my misunderstanding take the place?

From my point of view, this is a strange inconsistency because every small value type can be reinterpreted as primitive type which provides atomic write. As a result, we can have ugly workaround:

static void AtomicWrite<T>(T input, ref T location)
	{
	  if (!typeof(T).IsValueType)
	  {
		  location = input; // reference type
	  }
	  else if (Unsafe.SizeOf<T>() > IntPtr.Size)
	  {
		  throw new InvalidOperationException("Cannot write atomically");
	  }
	  else if (RuntimeHelpers.IsReferenceOrContainsReferences<T>())
	  {
		  location = input; // wrapped reference type
	  }
	  else
	  {
		  switch (Unsafe.SizeOf<T>())
		  {
			  case sizeof(byte):
				  Unsafe.As<T, byte>(ref location) = Unsafe.As<T, byte>(ref input);
				  break;
			  case sizeof(short):
				  Unsafe.As<T, short>(ref location) = Unsafe.As<T, short>(ref input);
				  break;
			  case sizeof(int):
				  Unsafe.As<T, int>(ref location) = Unsafe.As<T, int>(ref input);
				  break;
			  case sizeof(long) when IntPtr.Size == sizeof(long):
				  Unsafe.As<T, long>(ref location) = Unsafe.As<T, long>(ref input);
				  break;
			  default:
				  throw new InvalidOperationException("Cannot write atomically");
		  }
	  }
	}

According to the current implementation, this method will work for small value type correctly. Why?

[GSPP]

There's also:

A conforming CLI shall guarantee that read and write access to properly aligned memory locations no larger
than the native word size (the size of type native int) is atomic (see §12.6.2) when all the write accesses to a
location are the same size

So if user code outside of CD performs a byte-sized access, anywhere in the program, this is unsafe according to the standard. A method boundary would not make a difference. That access could even happen on another thread, or once at program initialization. A single access is able to decrease the guarantees.

I can understand @sakno's remarks with respect to inconsistency. This is something to be taken into consideration when working on:

• Document the memory model guaranteed by dotnet/runtime runtimes #63474

[sakno]

@GSPP , if so, then we cannot treat primitive types as tearing-free data type as well because I can reinterpret them as bytes:

int field;
Unsafe.Add(ref As<int, byte>(ref field), 1) = 10;

[sakno]

I found a source of this inconsistency. It is ECMA-334 (C# language specification), 10.6:

Reads and writes of the following data types shall be atomic: bool, char, byte, sbyte, short, ushort,
uint, int, float, and reference types.

ECMA-335 doesn't have this restriction.

[krwq]

It might be worth to keep it as a field rather than property to not risk any perf impact cc: @stephentoub

[krwq]

@EgorBo @vargaz so if I understand above statements we currently do not guarantee atomic writes for small structs (<= sizeof(IntPtr)). If that's the case we should close the PR and open the issue to consider fixing this or ECMA-335 spec

[stephentoub]

Thanks for the discussion. As was pointed out, this isn't valid: there's no guarantee that types with a size <= IntPtr.Size are written atomically, e.g. SharpLab. It's why ConcurrentDictionary is written the way it is.

[sakno]

@stephentoub , but this behavior is inconsistent with ECMA standard. Otherwise, we just leaving this issue without any solution.

[stephentoub]

Otherwise, we just leaving this issue without any solution.

This was a PR making an invalid change to ConcurrentDictionary. We don't need to keep it open to discuss ECMA spec language that won't impact the end state of the PR. Feel free to open an issue for discussion if you believe it's warranted. Thanks.

[jkotas]

Open #70384 with the ECMA-335 clarification

[sakno]

@stephentoub , @jkotas I would like to continue work on this PR and provide atomic read/write for sizeof(TValue) <= IntPtr.Size data types even with the current restrictions. I'll convert it to Draft for further reviews and discussions. Can you please reopen it?

[stephentoub]

provide atomic read/write for sizeof(TValue) <= IntPtr.Size data types even with the current restrictions

Can you elaborate?

[sakno]

Sure, here is the prototype of WriteValueAtomically method that is called in case of sizeof(TValue) <= IntPtr.Size:

internal void WriteValueAtomically(TValue value)
            {
                Debug.Assert(IsValueWriteAtomic);

                // object reference can be updated atomically
                // as well as primitive data types.
                // If underlying data type is not primitive we need to reinterpret
                // it accordingly to satisfy constraints described in Section 12.6.6 of ECMA-335
                if (RuntimeHelpers.IsReferenceOrContainsReferences<TValue>())
                {
                    _value = value;
                }
                else
                {
                    // JIT or AOT is able to eliminate redundant branches because 
                    // sizeof(TValue) is a constant
                    switch (Unsafe.SizeOf<TValue>())
                    {
                        case sizeof(ulong):
                            Unsafe.As<TValue, ulong>(ref _value) = Unsafe.As<TValue, ulong>(ref value);
                            break;
                        case sizeof(uint):
                            Unsafe.As<TValue, uint>(ref _value) = Unsafe.As<TValue, uint>(ref value);
                            break;
                        case sizeof(ushort):
                            Unsafe.As<TValue, ushort>(ref _value) = Unsafe.As<TValue, ushort>(ref value);
                            break;
                        case sizeof(byte):
                            Unsafe.As<TValue, byte>(ref _value) = Unsafe.As<TValue, byte>(ref value);
                            break;
                        default:
                            Debug.Fail("Unexpected size of TValue", $"Expected 1, 2, 4, or 8 bytes but actual is {Unsafe.SizeOf<TValue>()}");
                            break;
                    }
                }
            }

[stephentoub]

RuntimeHelpers.IsReferenceOrContainsReferences()

This isn't valid. The "ContainsReferences" part means this could be an arbitrarily large struct that has at least one reference field.

Unsafe.As<TValue, uint>(ref _value) = Unsafe.As<TValue, uint>(ref value);

The reading/writing of these as the primitive types is only guaranteed to be atomic if the field is appropriately aligned.

[sakno]

@stephentoub , all these issues are taken into account:

1. WriteValueAtomically method call is protected by IsValueWriteAtomic property. It means that this method is used only when sizeof(TValue) <= IntPtr.Size. Otherwise, concurrent dictionary uses fallback scenario and creates a new Node just like as the current version.
2. IsReferenceOrContainsReferences() is safe here. We know that sizeof(TValue) <= IntPtr.Size, therefore TValue is a reference type or value type that wraps a single field of reference type.
3. Reinterpreting memory location is safe here, because input value argument is aligned by CLR as well as _value field is aligned properly (because it is declared in the class with automatic memory layout).

[stephentoub]

because input value argument is aligned by CLR as well as _value field is aligned properly (because it is declared in the class with automatic memory layout).

If I have a struct:

struct S
{
    public ushort Value1, Value2;
}

when contained in something else that needn't be 4-byte aligned. What forces it to be?

[sakno]

Node class has 4-bytes or 8-bytes alignment because it has a field of reference type.

[stephentoub]

Node class has 4-bytes or 8-bytes alignment because it has a field of reference type.

That doesn't mean every one of its fields is aligned as such. I copied out Node from CD and tweaked it for a repro...

new Node<byte, S>().PrintValueAlignment();

struct S
{
    public ushort Value1, Value2;
}

sealed class Node<TKey, TValue> where TValue : unmanaged
{
    internal readonly TKey _key;
    internal TValue _value;
    internal volatile Node<TKey, TValue>? _next;
    internal readonly int _hashcode;

    internal unsafe void PrintValueAlignment()
    {
        fixed (TValue* ptr = &_value)
        {
            Console.WriteLine($"Value address: {(nint)ptr}");
            Console.WriteLine($"4-byte aligned: {Math.DivRem((long)ptr, 4).Remainder == 0}");
            Console.WriteLine($"8-byte aligned: {Math.DivRem((long)ptr, 8).Remainder == 0}");
        }
    }
}

When I run that, I get results like this:

Value address: 2359333810534
4-byte aligned: False
8-byte aligned: False

[sakno]

I tried the same on my machine but the address is always 4-bytes aligned. Anyway, we can assume that S has 2-bytes alignment. In that case, ReadUnaligned/WriteUnaligned can help with that.

[jkotas]

ReadUnaligned/WriteUnaligned is not atomic.

[sakno]

I would like to put the reference to #2167 proposal. In future, we can check alignment of the type. If it is equal to 4 or 8 then we can reinterpret memory location correctly and then write value atomically. Some of the types such as TimeSpan or DateTime are not considered as atomic data types.