From f7af7e870132a879c7ad3134dfe27d7743149a51 Mon Sep 17 00:00:00 2001 From: GGrain Development Team Date: Wed, 22 Apr 2026 09:47:12 +0200 Subject: [PATCH] refactor: replace FieldInfo reflection with UnsafeAccessor MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Swap three same-assembly FieldInfo.GetValue/SetValue sites for [UnsafeAccessor]-based extern shims. Each shim is resolved by the JIT at source-gen time and inlined into a direct field load/store — same hot-path cost as a normal field access, with no trim/AOT warnings and no reflection stack frame. Adoption sites (4 call sites, 3 distinct accessors): - CpuMemoryBufferView._parent (Backends.CPU/CpuMemoryManager.cs): CpuMemoryBufferTypedWrapper previously reflected into the view's internal parent reference on every GetParentBuffer() call. Now uses a strongly-typed CpuMemoryBufferViewAccessor. - UnifiedBuffer.Length / SizeInBytes backing fields (Core/DotCompute.Memory/UnifiedBufferMemory.cs): Resize() previously used two FieldInfo.SetValue calls against the compiler-generated "k__BackingField" and "k__BackingField" to mutate the otherwise-readonly auto-properties. Now uses UnifiedBufferBackingFields.LengthRef/SizeInBytesRef, which leverage UnsafeAccessor's .NET 9+ generic-parameter support. - MetalCompiledKernel._pipelineState (Backends.Metal): Two call sites (MetalExecutionEngine.GetPipelineStateFromKernel and MetalCommandExecutor.SetComputePipelineState) previously reflected to extract the native MTLComputePipelineState handle on every kernel dispatch. Now share a single MetalCompiledKernelAccessor shim. The command executor also pattern-matches on MetalCompiledKernel directly instead of walking the runtime type, so mocks in tests still no-op cleanly. Rejected candidates (kept as-is): - Core.Memory/TypedMemoryBufferWrapper._underlyingBuffer (accessed by Metal ExtractMetalBuffer and UnwrapBuffer): concrete T not known at the call site, so UnsafeAccessor's generic-parameter resolution cannot be applied. Would require a non-generic marker interface or InternalsVisibleTo + a new public accessor — out of scope here. - Runtime/GeneratedKernelDiscoveryService: reflects across KernelAttribute instances from arbitrary consumer assemblies. Attribute types are not referenced at compile time. - Runtime/AotPluginRegistry / Plugins/PluginSystem: reflect on runtime-loaded plugin Types. Not statically resolvable. - CudaKernelLauncher / MetalKernelParameterBinder / CPU/Metal/CUDA RingKernelRuntime: reflect on runtime-generated or runtime-supplied generic types whose element type is only known at dispatch time. - IAcceleratorExtensions / ICompiledKernelExtensions: reflect on arbitrary IAccelerator / ICompiledKernel implementations. Targets are interface methods, not private members. Build: dotnet build DotCompute.sln -c Release → 0 errors, 0 warnings. Tests: Backends.CPU.Tests 140/140 pass; Core.Tests 1692/1700 pass (same 6 flakes as main baseline, which shows 9 failures); UnifiedBuffer Resize tests 13/13 pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../CpuMemoryManager.cs | 25 +++++++----- .../Execution/MetalCommandExecutor.cs | 28 +++++++------- .../Execution/MetalExecutionEngine.cs | 33 +++------------- .../Kernels/MetalCompiledKernelAccessor.cs | 34 +++++++++++++++++ .../DotCompute.Memory/UnifiedBufferMemory.cs | 38 +++++++++++++------ 5 files changed, 95 insertions(+), 63 deletions(-) create mode 100644 src/Backends/DotCompute.Backends.Metal/Kernels/MetalCompiledKernelAccessor.cs diff --git a/src/Backends/DotCompute.Backends.CPU/CpuMemoryManager.cs b/src/Backends/DotCompute.Backends.CPU/CpuMemoryManager.cs index 9a8775c38..44482a65f 100644 --- a/src/Backends/DotCompute.Backends.CPU/CpuMemoryManager.cs +++ b/src/Backends/DotCompute.Backends.CPU/CpuMemoryManager.cs @@ -1236,13 +1236,20 @@ public ValueTask CopyToHostAsync(Memory destination, public ValueTask DisposeAsync() => _view.DisposeAsync(); // Helper methods - private CpuMemoryBuffer GetParentBuffer() - { - // Access the parent buffer from the view - // This uses reflection to access the private field, which is not ideal - // but necessary for the current implementation - var field = typeof(CpuMemoryBufferView).GetField("_parent", - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); - return (CpuMemoryBuffer)(field?.GetValue(_view) ?? throw new InvalidOperationException("Could not access parent buffer")); - } + private CpuMemoryBuffer GetParentBuffer() => CpuMemoryBufferViewAccessor.GetParent(_view); +} + +/// +/// AOT-friendly accessor for the private _parent field on +/// . Replaces runtime FieldInfo.GetValue +/// reflection with a source-gen-resolved extern shim that the JIT inlines into a +/// direct field load — same speed as a normal field access, no trim/AOT warnings. +/// +internal static class CpuMemoryBufferViewAccessor +{ + [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_parent")] + private static extern ref CpuMemoryBuffer ParentRef(CpuMemoryBufferView view); + + public static CpuMemoryBuffer GetParent(CpuMemoryBufferView view) + => ParentRef(view) ?? throw new InvalidOperationException("Could not access parent buffer"); } diff --git a/src/Backends/DotCompute.Backends.Metal/Execution/MetalCommandExecutor.cs b/src/Backends/DotCompute.Backends.Metal/Execution/MetalCommandExecutor.cs index 0eaf197f4..2d460bb3a 100644 --- a/src/Backends/DotCompute.Backends.Metal/Execution/MetalCommandExecutor.cs +++ b/src/Backends/DotCompute.Backends.Metal/Execution/MetalCommandExecutor.cs @@ -90,23 +90,21 @@ public void SetComputePipelineState(IntPtr encoder, object kernel) ArgumentNullException.ThrowIfNull(kernel); - // Extract pipeline state from kernel using reflection (MetalCompiledKernel has private _pipelineState field) - var kernelType = kernel.GetType(); -#pragma warning disable IL2075 // Reflection on kernel type is safe - Metal backend controls kernel types - var pipelineStateField = kernelType.GetField("_pipelineState", - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); -#pragma warning restore IL2075 - - if (pipelineStateField != null && pipelineStateField.GetValue(kernel) is IntPtr pipelineState && pipelineState != IntPtr.Zero) + // Production path: statically-typed access via UnsafeAccessor (AOT-safe, no reflection). + // Test path: anything that isn't a MetalCompiledKernel (e.g. mock) silently no-ops, matching + // the previous reflection-based behavior. + if (kernel is MetalCompiledKernel compiledKernel) { - MetalNative.SetComputePipelineState(encoder, pipelineState); - _logger.LogTrace("Set pipeline state {Pipeline} on encoder {Encoder}", pipelineState, encoder); - } - else - { - // This might be a test scenario with a mock kernel - _logger.LogTrace("Skipping pipeline state for kernel type: {KernelType}", kernelType.Name); + var pipelineState = MetalCompiledKernelAccessor.PipelineState(compiledKernel); + if (pipelineState != IntPtr.Zero) + { + MetalNative.SetComputePipelineState(encoder, pipelineState); + _logger.LogTrace("Set pipeline state {Pipeline} on encoder {Encoder}", pipelineState, encoder); + return; + } } + + _logger.LogTrace("Skipping pipeline state for kernel type: {KernelType}", kernel.GetType().Name); } /// diff --git a/src/Backends/DotCompute.Backends.Metal/Execution/MetalExecutionEngine.cs b/src/Backends/DotCompute.Backends.Metal/Execution/MetalExecutionEngine.cs index c45997453..b5a58f2eb 100644 --- a/src/Backends/DotCompute.Backends.Metal/Execution/MetalExecutionEngine.cs +++ b/src/Backends/DotCompute.Backends.Metal/Execution/MetalExecutionEngine.cs @@ -257,39 +257,18 @@ private static void ValidateGridDimensions(GridDimensions dimensions, string par } /// - /// Extracts the pipeline state handle from a compiled Metal kernel using reflection. + /// Extracts the pipeline state handle from a compiled Metal kernel. /// /// The compiled kernel. /// The pipeline state handle. - /// Thrown when pipeline state cannot be extracted. /// - /// This method uses reflection to access the private _pipelineState field. - /// This is necessary because MetalCompiledKernel doesn't expose the pipeline state publicly. + /// Delegates to , which uses + /// UnsafeAccessor to access the private _pipelineState field on + /// . The accessor is resolved at source-gen time + /// (AOT-safe, no trim warnings) and inlined into a direct field load by the JIT. /// private static IntPtr GetPipelineStateFromKernel(MetalCompiledKernel kernel) - { - var kernelType = typeof(MetalCompiledKernel); - -#pragma warning disable IL2075 // Reflection on kernel type is safe - Metal backend controls kernel types - var pipelineStateField = kernelType.GetField("_pipelineState", - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); -#pragma warning restore IL2075 - - if (pipelineStateField == null) - { - throw new InvalidOperationException( - "Unable to access pipeline state from MetalCompiledKernel. Internal structure may have changed."); - } - - var pipelineStateValue = pipelineStateField.GetValue(kernel); - if (pipelineStateValue is IntPtr pipelineState) - { - return pipelineState; - } - - throw new InvalidOperationException( - "Pipeline state field exists but has unexpected type or null value."); - } + => MetalCompiledKernelAccessor.PipelineState(kernel); /// /// Disposes the execution engine and releases native resources. diff --git a/src/Backends/DotCompute.Backends.Metal/Kernels/MetalCompiledKernelAccessor.cs b/src/Backends/DotCompute.Backends.Metal/Kernels/MetalCompiledKernelAccessor.cs new file mode 100644 index 000000000..29f2e8bbf --- /dev/null +++ b/src/Backends/DotCompute.Backends.Metal/Kernels/MetalCompiledKernelAccessor.cs @@ -0,0 +1,34 @@ +// Copyright (c) 2025 Michael Ivertowski +// Licensed under the MIT License. See LICENSE file in the project root for license information. + +using System.Runtime.CompilerServices; + +namespace DotCompute.Backends.Metal.Kernels; + +/// +/// AOT-friendly accessor for the private _pipelineState field on +/// . +/// +/// +/// The execution layer needs the raw MTLComputePipelineState handle to bind +/// to a command encoder, but deliberately keeps the +/// native handle private. Using instead of +/// FieldInfo.GetValue resolves the field at source-gen time, removes the +/// runtime reflection call (and its associated trim/AOT warnings), and lets the JIT +/// inline the access into a single field load. +/// +/// +internal static class MetalCompiledKernelAccessor +{ + [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_pipelineState")] + private static extern ref IntPtr PipelineStateRef(MetalCompiledKernel kernel); + + /// + /// Returns the native MTLComputePipelineState handle for the given compiled kernel. + /// + public static IntPtr PipelineState(MetalCompiledKernel kernel) + { + ArgumentNullException.ThrowIfNull(kernel); + return PipelineStateRef(kernel); + } +} diff --git a/src/Core/DotCompute.Memory/UnifiedBufferMemory.cs b/src/Core/DotCompute.Memory/UnifiedBufferMemory.cs index 8e130c589..fb5eecfc3 100644 --- a/src/Core/DotCompute.Memory/UnifiedBufferMemory.cs +++ b/src/Core/DotCompute.Memory/UnifiedBufferMemory.cs @@ -215,18 +215,11 @@ public void Resize(int newLength) // Update length and recalculate size var newSizeInBytes = newLength * Unsafe.SizeOf(); - // Use reflection to update readonly properties - - var lengthField = typeof(UnifiedBuffer).GetField("k__BackingField", - - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); - lengthField?.SetValue(this, newLength); - - - var sizeField = typeof(UnifiedBuffer).GetField("k__BackingField", - - System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Instance); - sizeField?.SetValue(this, newSizeInBytes); + // AOT-friendly: mutate the auto-property backing fields via UnsafeAccessor. + // Replaces FieldInfo.SetValue reflection with a source-gen-resolved extern shim + // that the JIT inlines into a direct field store. + UnifiedBufferBackingFields.LengthRef(this) = newLength; + UnifiedBufferBackingFields.SizeInBytesRef(this) = newSizeInBytes; // Reallocate host memory _hostArray = new T[newLength]; @@ -303,6 +296,27 @@ public async Task PrefetchToHostAsync() } } +/// +/// AOT-friendly accessors for the auto-property backing fields of . +/// Used by to mutate the otherwise-readonly +/// Length and SizeInBytes properties without runtime reflection. +/// +/// +/// resolves the field reference at source-gen time, +/// and the JIT inlines the accessor into a direct field store. This removes two +/// FieldInfo.SetValue calls from the Resize hot path and makes the code +/// trim/AOT-safe. +/// +/// +internal static class UnifiedBufferBackingFields +{ + [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "k__BackingField")] + public static extern ref int LengthRef(UnifiedBuffer buffer) where T : unmanaged; + + [UnsafeAccessor(UnsafeAccessorKind.Field, Name = "k__BackingField")] + public static extern ref long SizeInBytesRef(UnifiedBuffer buffer) where T : unmanaged; +} + /// /// Information about buffer memory allocation and usage. ///