diff --git a/rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs b/rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs index 2c0ec3457..5f2b724f4 100644 --- a/rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs +++ b/rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs @@ -1,8 +1,10 @@ using System; using System.Collections.Generic; +using System.Threading; using JetBrains.Core; using JetBrains.Diagnostics; using JetBrains.Lifetimes; +using JetBrains.Util.Internal; namespace JetBrains.Collections.Viewable { @@ -12,11 +14,36 @@ namespace JetBrains.Collections.Viewable /// public class ViewableProperty : IViewableProperty { + private static readonly bool ourIsReadWriteAtomic = Memory.IsReadWriteAtomic(); + private readonly Signal myChange = new Signal(); + private T myValue = default!; + private volatile bool myHasValue; + public ISource Change => myChange; - public Maybe Maybe { get; private set; } + public Maybe Maybe + { + get + { + if (myHasValue) + { + if (ourIsReadWriteAtomic) + { + return new Maybe(myValue); + } + + lock (myChange) + { + return new Maybe(myValue); + } + } + + return Maybe.None; + } + + } public ViewableProperty() {} @@ -35,8 +62,13 @@ public virtual T Value { lock (myChange) { - if (Maybe.HasValue && EqualityComparer.Default.Equals(Maybe.Value, value)) return; - Maybe = new Maybe(value); + if (myHasValue && EqualityComparer.Default.Equals(myValue, value)) return; + myValue = value; + myHasValue = true; + // After optimizing signal, `Fire` no longer provides a full memory fence (triggered by `Interlocked.CompareExchange`). + // This caused our tests to become flaky because we rely on `Fire(value)` being observed strictly after `myValue = value`; + // To enforce this ordering, we explicitly add a memory barrier here. + Interlocked.MemoryBarrier(); myChange.Fire(value); } } diff --git a/rd-net/Lifetimes/Util/Memory.cs b/rd-net/Lifetimes/Util/Memory.cs index 59d272f42..2bfaa6222 100644 --- a/rd-net/Lifetimes/Util/Memory.cs +++ b/rd-net/Lifetimes/Util/Memory.cs @@ -1,3 +1,4 @@ +using System; using System.Runtime.CompilerServices; using System.Threading; @@ -94,5 +95,53 @@ public static void VolatileWrite(ref bool location, bool value) { Volatile.Write(ref location, value); } + + public static bool IsReadWriteAtomic() + { + return IsReadWriteAtomicCache.IsReadWriteAtomic; + } + + private static class IsReadWriteAtomicCache + { + // ReSharper disable once StaticMemberInGenericType + public static readonly bool IsReadWriteAtomic = Compute(); + + private static bool Compute() + { + //According to runtime specification + //https://github.com/dotnet/runtime/blob/main/docs/design/specs/Memory-model.md#atomic-memory-accesses + //Managed references accesses are atomic + if (!typeof(T).IsValueType) + return true; + //Otherwise, only primitive and Enum types with size up to the platform pointer size accesses are atomic + if (!typeof(T).IsPrimitive && !typeof(T).IsEnum) + return false; + + //Native integer primitive types + if (typeof(T) == typeof(IntPtr) || typeof(T) == typeof(UIntPtr)) + return true; + + //Other primitive types and Enum types (via underlying primitive type) + switch (Type.GetTypeCode(typeof(T))) + { + case TypeCode.Boolean: + case TypeCode.SByte: + case TypeCode.Byte: + case TypeCode.Char: + case TypeCode.Int16: + case TypeCode.UInt16: + case TypeCode.Int32: + case TypeCode.UInt32: + case TypeCode.Single: + return true; + case TypeCode.Int64: + case TypeCode.UInt64: + case TypeCode.Double: + return IntPtr.Size == 8; + default: + return false; + } + } + } } } \ No newline at end of file diff --git a/rd-net/Test.Lifetimes/Utils/MemoryTest.cs b/rd-net/Test.Lifetimes/Utils/MemoryTest.cs new file mode 100644 index 000000000..bdef928ad --- /dev/null +++ b/rd-net/Test.Lifetimes/Utils/MemoryTest.cs @@ -0,0 +1,83 @@ +using System; +using JetBrains.Util.Internal; +using NUnit.Framework; + +namespace Test.Lifetimes.Utils +{ + public class MemoryTest + { + #region TestTypes + private enum ByteEnum : byte { Value = 1 } + private enum IntEnum : int { Value = 1 } + private enum UIntEnum : int { Value = 1 } + private enum LongEnum : long { Value = 1 } + private enum ULongEnum : long { Value = 1 } + + private struct UserDefinedStruct + { + public int Value; + } + #endregion + + [TestCase(typeof(string))] + [TestCase(typeof(object))] + public void IsReadWriteAtomic_ReferenceTypes_ReturnsTrue(Type type) + { + Assert.IsTrue(IsReadWriteAtomic(type)); + } + + [TestCase(typeof(bool))] + [TestCase(typeof(byte))] + [TestCase(typeof(sbyte))] + [TestCase(typeof(char))] + [TestCase(typeof(short))] + [TestCase(typeof(ushort))] + [TestCase(typeof(int))] + [TestCase(typeof(uint))] + [TestCase(typeof(float))] + [TestCase(typeof(IntPtr))] + [TestCase(typeof(UIntPtr))] + public void IsReadWriteAtomic_PrimitiveTypes_ReturnsTrue(Type type) + { + Assert.IsTrue(IsReadWriteAtomic(type)); + } + + [TestCase(typeof(long))] + [TestCase(typeof(ulong))] + [TestCase(typeof(double))] + public void IsReadWriteAtomic_LargePrimitiveTypes_DependsOnArchitecture(Type type) + { + Assert.AreEqual(IsReadWriteAtomic(type), IntPtr.Size == 8); + } + + [TestCase(typeof(ByteEnum))] + [TestCase(typeof(IntEnum))] + [TestCase(typeof(UIntEnum))] + public void IsReadWriteAtomic_Enums_ReturnsTrue(Type type) + { + Assert.IsTrue(IsReadWriteAtomic(type)); + } + + [TestCase(typeof(LongEnum))] + [TestCase(typeof(ULongEnum))] + public void IsReadWriteAtomic_LargeEnums_DependsOnArchitecture(Type type) + { + Assert.AreEqual(IsReadWriteAtomic(type), IntPtr.Size == 8); + } + + [TestCase(typeof(DateTime))] + [TestCase(typeof(decimal))] + [TestCase(typeof(UserDefinedStruct))] + public void IsReadWriteAtomic_UserDefinedStructs_ReturnsFalse(Type type) + { + Assert.IsFalse(IsReadWriteAtomic(type)); + } + + private static bool IsReadWriteAtomic(Type type) + { + var canon = typeof(Memory).GetMethod(nameof(Memory.IsReadWriteAtomic)); + var generic = canon!.MakeGenericMethod(type); + return (bool)generic.Invoke(null,null); + } + } +}