Improve/Fix ViewableProperty by Iliya-usov · Pull Request #591 · JetBrains/rd

Iliya-usov · 2026-02-16T21:13:43Z

Implement Memory.IsReadWriteAtomic
Make maybe return consistent value with hasValue
Fix tearing issues with myValue if T is long struct
Enforce memory ordering between myValue = value and myChange.Fire(value);

- Make maybe return consistent value with hasValue - Fix tearing issues with myValue if T is long struct - Enforce memory ordering between myValue = value and myChange.Fire(value);

rd-net/Lifetimes/Util/Memory.cs

## Memory.SizeOf<T>() Add a public unconstrained `SizeOf<T>()` method that returns the managed size of any type. Results are cached in a static generic class `SizeOfCache<T>`. Implementation per TFM: - net5+: delegates to `Unsafe.SizeOf<T>()` - net472: emits a `DynamicMethod` with the `sizeof` IL opcode - netstandard2.0: same DynamicMethod approach, enabled by adding a conditional `System.Reflection.Emit.Lightweight` package reference ## IsReadWriteAtomicCache<T> cleanup Replace the old approximate field-sum size calculation (which used `Marshal.SizeOf` for primitives and recursive field enumeration) with exact `SizeOf<T>()`. This fixes edge cases where padding/alignment caused the old heuristic to return incorrect sizes (e.g. `PaddedSequentialStruct` field sum = 8 but actual size = 12). The Pack check `(Pack != 0 && Pack != MaxAtomicSize)` is intentionally conservative: it rejects any non-default Pack that doesn't match the atomic word size, even if the Pack provides sufficient alignment (e.g. Pack=16). This avoids false positives for atomicity on architectures where misaligned access is non-atomic (ARM) or may cross cache lines (x86/x64). ## MaxAtomicSize simplification Use `IntPtr.Size` directly instead of checking `Is64BitOperatingSystem`, since 8-byte atomic reads are not safe in 32-bit processes even on a 64-bit OS. ## Tests (157 total) - Primitives, reference types, enums, pointer types - Struct layout edge cases: padding, field ordering, explicit layout with large offsets and overlapping fields (unions) - Pack variations: Pack=1/2/4/8/16 with various field combinations - StructLayout.Size attribute: forced sizes at boundary values - Generic structs: Wrapper<T>, Pair<T1,T2>, Triple<T1,T2,T3> with value types, reference types, nested generics, constrained generics - BCL generics: Nullable<T>, KeyValuePair<K,V>, ValueTuple - SizeOf<T>() for all of the above including reference types - Consistency checks between SizeOf and IsReadWriteAtomic - Endianness documentation: tests confirming atomicity is determined by size and alignment, not byte ordering Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR improves ViewableProperty<T> correctness under concurrency by adding a type-based atomicity check in Memory, using it to avoid torn reads for non-atomic value types, and strengthening memory ordering between value updates and change notifications.

Changes:

Add Memory.SizeOf<T>() and Memory.IsReadWriteAtomic<T>() (with caching) and wire required dependency for netstandard2.0.
Update ViewableProperty<T> to store value/hasValue separately, guard non-atomic reads with a lock, and add an explicit memory barrier before firing change notifications.
Add extensive unit tests covering SizeOf<T>() and IsReadWriteAtomic<T>() across many type/layout scenarios.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.

File	Description
rd-net/Test.Lifetimes/Utils/MemoryTest.cs	New test suite for `Memory.SizeOf<T>()` and `Memory.IsReadWriteAtomic<T>()` across primitives, structs, layout/pack edge cases
rd-net/Lifetimes/Util/Memory.cs	Adds `SizeOf<T>()` and `IsReadWriteAtomic<T>()` implementations with caching and runtime-specific sizing strategy
rd-net/Lifetimes/Lifetimes.csproj	Adds `System.Reflection.Emit.Lightweight` for `netstandard2.0` to support `DynamicMethod` sizing path
rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs	Uses atomicity check + locking to avoid torn reads and adds ordering barrier between state update and `Fire()`

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+      Assert.IsTrue(Memory.IsReadWriteAtomic<long>());
+    }
+
+    [Test]
+    public void IsReadWriteAtomic_ULong_ReturnsTrue()
+    {
+      Assert.IsTrue(Memory.IsReadWriteAtomic<ulong>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+      Assert.IsTrue(Memory.IsReadWriteAtomic<long>());
+    }
+
+    [Test]
+    public void IsReadWriteAtomic_ULong_ReturnsTrue()
+    {
+      Assert.IsTrue(Memory.IsReadWriteAtomic<ulong>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+    [Test]
+    public void IsReadWriteAtomic_Double_ReturnsTrue()
+    {
+      Assert.IsTrue(Memory.IsReadWriteAtomic<double>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+      // DateTime is 8 bytes (single ulong internally)
+      Assert.IsTrue(Memory.IsReadWriteAtomic<DateTime>());
+    }
+
+    [Test]
+    public void IsReadWriteAtomic_TimeSpan_ReturnsTrue()
+    {
+      // TimeSpan is 8 bytes (single long internally)
+      Assert.IsTrue(Memory.IsReadWriteAtomic<TimeSpan>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+      // OptimalSequentialStruct: int + short + byte + 1pad = 8 bytes → atomic on 64-bit
+      // PaddedSequentialStruct:  short + 2pad + int + byte + 3pad = 12 bytes → non-atomic
+      Assert.IsTrue(Memory.IsReadWriteAtomic<OptimalSequentialStruct>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+      // Size=8 <= MaxAtomicSize on 64-bit, so this is atomic.
+      Assert.IsTrue(Memory.IsReadWriteAtomic<Pack16IntShort>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+
+    // Auto layout, mixed types: int(4) + byte(1) + long(8) = 13 bytes of fields
+    // Non-atomic regardless of reordering (field sum alone > 8)
+    [StructLayout(LayoutKind.Sequential)]


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+    public void IsReadWriteAtomic_TimeSpan_ReturnsTrue()
+    {
+      // TimeSpan is 8 bytes (single long internally)
+      Assert.IsTrue(Memory.IsReadWriteAtomic<TimeSpan>());


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+      // int + byte, worst-case padded to 8 → always atomic
+      Assert.IsTrue(Memory.IsReadWriteAtomic<AutoLayoutMixedSmall>());


Replace unconditional Assert.IsTrue for 8-byte types with Assert.AreEqual(MaxAtomicSize >= 8, ...) so tests are correct when IntPtr.Size is 4. Also fix SizeOf_DateTimeOffset which expects 16 bytes but gets 12 on 32-bit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR improves ViewableProperty<T> correctness under concurrency by avoiding torn reads for non-atomic value types and enforcing ordering between storing the new value and firing change notifications. It also adds Memory.SizeOf<T>() and Memory.IsReadWriteAtomic<T>() utilities plus extensive tests covering many layout/size edge cases.

Changes:

Added Memory.SizeOf<T>() and Memory.IsReadWriteAtomic<T>() with caching and a Reflection.Emit fallback.
Updated ViewableProperty<T> to store myValue/myHasValue separately, use atomicity detection to avoid tearing, and add an explicit memory barrier before firing change events.
Added a large new NUnit test suite validating SizeOf<T>() and IsReadWriteAtomic<T>() across many types and layouts.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
rd-net/Test.Lifetimes/Utils/MemoryTest.cs	Adds comprehensive tests for `Memory.SizeOf<T>()` and `Memory.IsReadWriteAtomic<T>()`.
rd-net/Lifetimes/Util/Memory.cs	Implements `SizeOf<T>()` and `IsReadWriteAtomic<T>()` with per-type caching and NET5+/fallback paths.
rd-net/Lifetimes/Lifetimes.csproj	Adds `System.Reflection.Emit.Lightweight` for `netstandard2.0` to support the fallback `DynamicMethod` path.
rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs	Uses atomicity detection to avoid torn reads, makes `Maybe` consistent with `HasValue`, and enforces ordering before firing change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

rd-net/Lifetimes/Util/Memory.cs

rd-net/Lifetimes/Collections/Viewable/ViewableProperty.cs

        {
          if (Maybe.HasValue && EqualityComparer<T>.Default.Equals(Maybe.Value, value)) return;
-          Maybe = new Maybe<T>(value);
+          myValue = value;


rd-net/Test.Lifetimes/Utils/MemoryTest.cs

+
+    // Auto layout, mixed types: int(4) + byte(1) + long(8) = 13 bytes of fields
+    // Non-atomic regardless of reordering (field sum alone > 8)
+    [StructLayout(LayoutKind.Sequential)]


Iliya-usov added 2 commits February 16, 2026 22:12

Implement Memory.IsReadWriteAtomic

f6687f9

Improve ViewableProperty

1a4733d

- Make maybe return consistent value with hasValue - Fix tearing issues with myValue if T is long struct - Enforce memory ordering between myValue = value and myChange.Fire(value);

Iliya-usov requested a review from ForNeVeR February 16, 2026 21:13

ForNeVeR requested changes Mar 9, 2026

View reviewed changes

rd-net/Lifetimes/Util/Memory.cs Outdated Show resolved Hide resolved

rd-net/Lifetimes/Util/Memory.cs Outdated Show resolved Hide resolved

rd-net/Lifetimes/Util/Memory.cs Show resolved Hide resolved

ForNeVeR assigned ForNeVeR and Iliya-usov Mar 9, 2026

Iliya-usov requested review from ForNeVeR and Copilot March 16, 2026 20:19

Copilot started reviewing on behalf of Iliya-usov March 16, 2026 20:20 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

Iliya-usov requested a review from Copilot March 16, 2026 20:50

Copilot started reviewing on behalf of Iliya-usov March 16, 2026 20:51 View session

Copilot AI reviewed Mar 16, 2026

View reviewed changes

ForNeVeR approved these changes Mar 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve/Fix ViewableProperty#591

Improve/Fix ViewableProperty#591
Iliya-usov wants to merge 4 commits intomasterfrom
usov/viewable-property

Iliya-usov commented Feb 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		// Size=8 <= MaxAtomicSize on 64-bit, so this is atomic.
		Assert.IsTrue(Memory.IsReadWriteAtomic<Pack16IntShort>());

		// int + byte, worst-case padded to 8 → always atomic
		Assert.IsTrue(Memory.IsReadWriteAtomic<AutoLayoutMixedSmall>());

Conversation

Iliya-usov commented Feb 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants