Simplify System.Threading.Volatile implementation

[jkotas]

Reduce amount of unsafe code and helper types

[Copilot]

Pull Request Overview

Simplifies the Volatile class by removing helper structs and Unsafe.As calls for most primitive types and replacing them with direct reads/writes paired with ReadBarrier/WriteBarrier calls.

• Convert Read/Write for Boolean, Byte, Int16, Int32, Int64, IntPtr, SByte, Single, UInt16, UInt32, UIntPtr, and generic T to barrier-based implementations
• Remove Volatile* helper structs
• Retain intrinsic-based implementation for double via underlying long overload

Comments suppressed due to low confidence (2)

src/libraries/System.Private.CoreLib/src/System/Threading/Volatile.cs:52

• [nitpick] The double Read method still relies on the intrinsic volatile pattern rather than the barrier-based approach used by other overloads. For consistency and clarity, consider refactoring it to a block body that does double value = location; ReadBarrier(); return value;.

        public static double Read(ref readonly double location)

src/libraries/System.Private.CoreLib/src/System/Threading/Volatile.cs:60

• [nitpick] The double Write overload remains an expression-bodied call to Write(ref Unsafe.As<double, long>(...)). To match the style of other Write methods, consider expanding it to a block, invoking WriteBarrier(), then assigning the bit‐converted long value directly.

        public static void Write(ref double location, double value) =>

[dotnet-policy-service]

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

[MichalPetryka]

This will probably only change Mono due to RyuJIT having intrinsics for those, would it make sense to make those must expand and have Mono handle it too instead?

[jkotas]

This will probably only change Mono due to RyuJIT having intrinsics for those, would it make sense to make those must expand and have Mono handle it too instead?

The must expand intrinsics have to be implemented in the coreclr interpreter as well now/soon (see #116769). I am not sure whether it would be an improvement. It is much easier to implement the non-intrinsic expansion in C#.

[NinoFloris]

How will these changes concretely affect code running on weakly ordered memory architectures? It seems this PR creates a behavioral difference between the intrinsics (which on arm64 use location based acquire/release instructions ldapr/stlr) while the barriers employed here would cause a full dmbish(ld) to be emitted.

Are we ok with over delivering on ordering semantics (which has significant performance impact) on runtimes that don't have intrinsic implementations?

[xtqqczze]

There are conflicts from 56b2a2a

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "78ef1e1ff2ddb5e1f923c8e3f81f9912b1cd59ac",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "cb84c8521ce6a4cbde63cb1a700fcb850be791ae",
  "last_reviewed_commit": "78ef1e1ff2ddb5e1f923c8e3f81f9912b1cd59ac",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "cb84c8521ce6a4cbde63cb1a700fcb850be791ae",
  "last_recorded_worker_run_id": "29690853212",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "c7c0f1d2e7d38bf493b27840b1fa9c91dc9d3a3b",
      "review_id": 4729976854
    },
    {
      "commit": "78ef1e1ff2ddb5e1f923c8e3f81f9912b1cd59ac",
      "review_id": 4730932579
    }
  ]
}

[github-actions]

Holistic Review

Motivation: System.Threading.Volatile previously implemented each Read/Write overload by reinterpreting the target ref as a private struct containing a volatile field (via Unsafe.As<T, VolatileXxx>), relying on a large family of helper structs and pervasive Unsafe casts. The goal here is to reduce the amount of unsafe code and eliminate those helper types.

Approach: Each overload is rewritten to an explicit, readable pattern: reads do a plain load followed by ReadBarrier() (acquire semantics), and writes do a WriteBarrier() followed by a plain store (release semantics). The VolatileBoolean/VolatileByte/.../VolatileObject helper structs and their Unsafe.As reinterpretations are removed. The 32-bit long/ulong paths retain their Interlocked implementations to preserve atomicity, and double/ulong continue to delegate to the long overload (comments clarify why). All methods remain [Intrinsic]/[NonVersionable], so RyuJIT still substitutes optimized codegen; the new C# fallback primarily affects Mono and the CoreCLR interpreter.

Summary: This is a low-risk, behavior-preserving refactor. The acquire (load-then-ReadBarrier) and release (WriteBarrier-then-store) orderings are correct and match the existing volatile semantics, and the atomicity-sensitive 32-bit 64-bit paths are unchanged. ReadBarrier/WriteBarrier are themselves intrinsics with self-referential fallbacks, consistent with the prior code. No correctness, performance, or convention issues were found. LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 57.8 AIC · ⌖ 10.2 AIC · ⊞ 10K

[tannergooding]

Ah, I thought the conflicts were resolved here. Its just a copilot reply and the notification took me to the wrong thing.

[Copilot]

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

[jeffhandley]

@tannergooding -- I need to defer to you on the right way to implement the feedback you had on this.

[jkotas]

That changes the behavior for non-intrinsic execution paths

This is similar to the @NinoFloris feedback above. It made me think twice about whether we want to take this change vs. rely on always-expand intrinsics instead. Doing the later will be a bit easier once Mono is gone.

[MichalPetryka]

rely on always-expand intrinsics instead. Doing the later will be a bit easier once Mono is gone.

I believe this will be the better option here, especially if interpreter starts using the JIT as frontend.

[github-actions]

Holistic Review

Motivation: System.Threading.Volatile previously implemented each Read/Write overload by reinterpreting the target ref as a private struct with a volatile field (via Unsafe.As<T, VolatileXxx>), relying on a large family of helper types and pervasive Unsafe casts. The PR removes those helpers and unsafe reinterpretations in favor of explicit, readable barrier-based implementations.

Approach: Each overload is rewritten so reads do a plain load followed by ReadBarrier() (acquire) and writes do WriteBarrier() followed by a plain store (release). The atomicity-sensitive 64-bit types (long/ulong) keep dedicated Interlocked fallbacks on 32-bit under #if TARGET_64BIT. The latest commit (78ef1e1, "Simplify ulong volatile access") stops routing ulong Read/Write through the long overload via Unsafe.As<ulong, long> and instead inlines the same TARGET_64BIT load/ReadBarrier and WriteBarrier/store fast path plus the 32-bit Interlocked.CompareExchange(...,0,0) / Interlocked.Exchange fallback directly on the ulong refs. All methods remain [Intrinsic]/[NonVersionable], so RyuJIT still substitutes optimized codegen and the C# body primarily serves Mono and the CoreCLR interpreter.

Summary: The incremental change is behavior-preserving. The inlined ulong implementation is now structurally identical to the long overload and drops one layer of Unsafe reinterpretation, which is a net simplification consistent with the rest of the file. Interlocked.CompareExchange(ref ulong,...) and Interlocked.Exchange(ref ulong,...) overloads exist, so the 32-bit path compiles and remains atomic; the 64-bit acquire/release orderings are correct. No correctness, performance, or convention issues were found in the delta. LGTM.

Assessment History

• review 4729976854 reviewed commit c7c0f1d with verdict LGTM. Current verdict is LGTM (unchanged). The new commit only replaces the ulong delegate-to-long implementation with an inlined copy of the same barrier/Interlocked pattern; motivation, approach, and risk are unchanged, so the assessment is unchanged.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 49.6 AIC · ⌖ 14.6 AIC · ⊞ 10K