refactor: replace reflection with UnsafeAccessor in hot paths#142
Open
mivertowski wants to merge 1 commit intomainfrom
Open
refactor: replace reflection with UnsafeAccessor in hot paths#142mivertowski wants to merge 1 commit intomainfrom
mivertowski wants to merge 1 commit intomainfrom
Conversation
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<T> previously reflected into the view's internal parent reference on every GetParentBuffer() call. Now uses a strongly-typed CpuMemoryBufferViewAccessor. - UnifiedBuffer<T>.Length / SizeInBytes backing fields (Core/DotCompute.Memory/UnifiedBufferMemory.cs): Resize() previously used two FieldInfo.SetValue calls against the compiler-generated "<Length>k__BackingField" and "<SizeInBytes>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<T>._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) <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
FieldInfo.GetValue/SetValuesites for[UnsafeAccessor]-based extern shims (4 call sites, 3 distinct accessors). 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.src/reflection; most remaining sites target runtime-loaded plugins, generic-typed consumer attributes, or open generics whereTisn't known at the call site, and are correctly out of scope (see "Rejected candidates" below).Adoption sites
1.
CpuMemoryBufferView._parent—src/Backends/DotCompute.Backends.CPU/CpuMemoryManager.csCpuMemoryBufferTypedWrapper<T>.GetParentBuffer()previously reflected into the view's internal_parentreference on every call. The comment in the old code explicitly noted "uses reflection to access the private field, which is not ideal." Now delegates to a newCpuMemoryBufferViewAccessorhelper that uses[UnsafeAccessor(UnsafeAccessorKind.Field, Name = "_parent")].2.
UnifiedBuffer<T>.Length/SizeInBytesbacking fields —src/Core/DotCompute.Memory/UnifiedBufferMemory.csResize(int)previously used twoFieldInfo.SetValuecalls against the compiler-generated<Length>k__BackingFieldand<SizeInBytes>k__BackingFieldnames to mutate the otherwise-readonly auto-properties. Now delegates to a newUnifiedBufferBackingFieldshelper with genericUnsafeAccessormethods — leverages the .NET 9+ generic-parameter support.3.
MetalCompiledKernel._pipelineState— 2 call sites insrc/Backends/DotCompute.Backends.Metal/Execution/MetalExecutionEngine.GetPipelineStateFromKernelandMetalCommandExecutor.SetComputePipelineStatepreviously reflected to extract the nativeMTLComputePipelineStatehandle on every kernel dispatch. Both now share a single newMetalCompiledKernelAccessor.PipelineState()helper. The command executor also pattern-matches onMetalCompiledKerneldirectly instead of walkingkernel.GetType(), so mock kernels in tests still no-op cleanly via the fallback branch.Rejected candidates (investigated, not viable)
TypedMemoryBufferWrapper<T>._underlyingBuffer(MetalExtractMetalBuffer/UnwrapBuffer)Tunknown at the call site — needed a non-generic marker interface or InternalsVisibleTo + public accessor, out of scopeGeneratedKernelDiscoveryService(Name,FullName,Backends,VectorSize,IsParallelonKernelAttribute)KernelAttributecomes from arbitrary consumer assemblies; runtime (DotCompute.Runtime) doesn't referenceDotCompute.Generators.AttributesAotPluginRegistry/PluginSystem.CreatePluginInstanceTypes — not statically resolvableCudaKernelLauncher_devicePtr/DevicePointerlookupsCudaMemoryBuffer<T>with unknown T)CPU/Metal/CUDA RingKernelRuntimequeue reflection (TryDequeue/TryEnqueue/Count/Capacity)InputQueue/OutputQueuetyped asobject?; concrete queue types only known at dispatch timeIAcceleratorExtensions/ICompiledKernelExtensionsIAccelerator/ICompiledKernelimplementations by name; targets are interface-style membersSystemInfoManagerWMI paths (per task note)ComputeQueryableExtensions.ExecuteTyped<T>viaMakeGenericMethodMetalAccelerator.PerformCleanupreflectingBaseAccelerator._loggerBaseAcceleratoralready exposesprotected ILogger Logger; this is a gratuitous-reflection bug for a separate cleanup PRBuild / test status
dotnet build DotCompute.sln --configuration Release→ 0 errors, 0 warningsDotCompute.Backends.CPU.Tests→ 140/140 passDotCompute.Core.Tests→ 1692/1700 pass (6 flaky failures — main baseline shows 9 failures for the same suite; same 2 skips)DotCompute.Memory.TestsResizefilter → 13/13 pass (directly exercises theUnsafeAccessorrewrite ofLength/SizeInBytesbacking fields)DotCompute.Memory.Testsfull suite → 48 failures, identical set to main baseline (pre-existing FluentAssertions / buffer-tracking test flakes; not introduced by this PR)Test plan
CpuMemoryBufferViewAccessor)🤖 Generated with Claude Code