perf: add onednn binary primitives for add/multiply achieving parity with torchsharp - #732
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds raw-array TensorPrimitives benchmarks; introduces in-place Sigmoid/ReLU APIs on CpuEngine with optional oneDNN unsafe fast paths; lowers parallel threshold and rewrites chunked parallel execution; implements eltwise and binary primitive caches and in-place execution in OneDnnProvider; FloatOperations now prefers oneDNN for large Add/Multiply and parallel Sum. Changes
Sequence DiagramsequenceDiagram
participant Caller as Caller
participant CpuEngine as CpuEngine
participant OneDnn as OneDnnProvider
participant FloatOps as FloatOperations
participant TensorPrims as TensorPrimitives
rect rgba(100,150,200,0.5)
Note over Caller,CpuEngine: In-place activation flow
Caller->>CpuEngine: SigmoidInPlace(tensor)
CpuEngine->>OneDnn: TrySigmoid(span)
OneDnn->>OneDnn: Lookup/Create eltwise primitive & mem
OneDnn->>OneDnn: Execute on stream (in-place)
alt OneDNN success
OneDnn-->>CpuEngine: true
else fallback
CpuEngine->>CpuEngine: Generic in-place sigmoid
end
CpuEngine-->>Caller: done
end
rect rgba(150,200,100,0.5)
Note over Caller,FloatOps: Binary add/multiply flow
Caller->>FloatOps: Add(aSpan, bSpan)
FloatOps->>OneDnn: TryBinary/Add(aSpan,bSpan,dstSpan)
OneDnn->>OneDnn: Lookup/Create binary primitive & mem
alt OneDNN success (large arrays)
OneDnn-->>FloatOps: true (dst written)
else fallback
FloatOps->>TensorPrims: TensorPrimitives.Add(aSpan,bSpan,dstSpan)
TensorPrims-->>FloatOps: result
end
FloatOps-->>Caller: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds oneDNN binary primitive support for element-wise addition and multiplication operations to achieve performance parity with TorchSharp. The implementation includes caching of oneDNN primitives, parallel Sum implementation, and optimization of FloatOperations to use oneDNN for large arrays (>=50K elements).
Changes:
- Added oneDNN binary primitives (TryAdd, TryMultiply) with primitive caching to OneDnnProvider
- Implemented parallel Sum with lock-based reduction for large arrays in FloatOperations
- Lowered ParallelThreshold from 65536 to 50000 to enable parallelism for 100K+ element arrays
- Added raw TensorPrimitives benchmarks for baseline performance comparison
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/AiDotNet.Tensors/NumericOperations/FloatOperations.cs | Integrated oneDNN for Add/Multiply operations on large arrays, added parallel Sum implementation, updated ParallelThreshold to 50000 |
| src/AiDotNet.Tensors/Helpers/OneDnnProvider.cs | Added binary primitive support (TryAdd, TryMultiply), eltwise operations (TryReLU, TrySigmoid), and caching infrastructure for both primitive types |
| src/AiDotNet.Tensors/Helpers/CpuParallelSettings.cs | Updated ParallelThreshold to 50000, refactored ParallelForChunks with improved chunking logic and null validation |
| src/AiDotNet.Tensors/Engines/CpuEngine.cs | Added oneDNN fast-path for SigmoidInPlace and ReLUInPlace operations with conditional compilation for non-NETFRAMEWORK targets |
| AiDotNetBenchmarkTests/TorchSharpCpuComparisonBenchmarks.cs | Added raw TensorPrimitives benchmarks for direct performance comparison with AiDotNet and TorchSharp implementations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AiDotNet.Tensors/Helpers/CpuParallelSettings.cs (1)
62-86: Validate minChunkSize to avoid divide-by-zero.
Public callers can passminChunkSize <= 0, which will break the new chunk math. Guard early.🛠️ Proposed fix
if (action is null) throw new ArgumentNullException(nameof(action)); + + if (minChunkSize <= 0) + throw new ArgumentOutOfRangeException(nameof(minChunkSize), "minChunkSize must be > 0.");
🤖 Fix all issues with AI agents
In `@src/AiDotNet.Tensors/NumericOperations/FloatOperations.cs`:
- Around line 929-947: The OneDnn unsafe path in Add (and similarly in Multiply)
uses fixed pointers into ReadOnlySpan/Span without validating that x.Length ==
y.Length == destination.Length, risking memory corruption; before entering the
NET8_0 block and before calling OneDnnProvider.TryAdd/TryMultiply, validate the
three span lengths (x, y, destination) and throw a clear
ArgumentException/ArgumentOutOfRangeException on mismatch (same validation
TensorPrimitives would have done), then proceed to the
OneDnnProvider.TryAdd/TryMultiply and fallback to TensorPrimitives.Add/Multiply
as written.
🧹 Nitpick comments (1)
src/AiDotNet.Tensors/Engines/CpuEngine.cs (1)
4118-4142: Consider extracting the oneDNN fast-path boilerplate.The memory-marshaling and fixed-pointer pattern is duplicated between
SigmoidInPlaceandReLUInPlace. If more unary activations are added (e.g., Tanh, LeakyReLU), this will grow.A helper could reduce duplication:
♻️ Optional helper to reduce boilerplate
// Helper method (private) private unsafe bool TryOneDnnUnary<T>(Tensor<T> tensor, Func<float*, int, bool> op) { if (typeof(T) != typeof(float) || !OneDnnProvider.IsAvailable) return false; var floatMem = (Memory<float>)(object)tensor.Data; if (MemoryMarshal.TryGetArray((ReadOnlyMemory<float>)floatMem, out var segment) && segment.Array is not null && segment.Offset == 0) { fixed (float* ptr = segment.Array) { return op(ptr, tensor.Length); } } return false; } // Usage in SigmoidInPlace: if (TryOneDnnUnary(tensor, OneDnnProvider.TrySigmoid)) return; // Usage in ReLUInPlace: if (TryOneDnnUnary(tensor, OneDnnProvider.TryReLU)) return;
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
…with torchsharp - Add oneDNN binary primitive support (dnnl_binary_add, dnnl_binary_mul) - Implement TryAdd and TryMultiply methods in OneDnnProvider with caching - Add parallel Sum implementation with lock for combining partial results - Optimize FloatOperations to use oneDNN for large arrays (>=50K elements) - Add CpuParallelSettings for configurable parallel threshold - Add raw TensorPrimitives benchmarks for baseline comparison Benchmark results: - TensorAdd (1M): 19.76μs vs TorchSharp 20.01μs - PARITY/WIN - TensorMultiply (1M): 20.41μs vs TorchSharp 19.22μs - PARITY - TensorAdd (100K): 9.99μs vs TorchSharp 9.18μs - PARITY - TensorMultiply (100K): 10.90μs vs TorchSharp 8.47μs - Close - TensorSum (1M): 23.54μs vs TorchSharp 31.57μs - WIN 34% - TensorMean (1M): 25.80μs vs TorchSharp 38.93μs - WIN 51% Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix parallel Sum lock contention by using array of partial sums instead of shared variable with lock. Each thread writes to its own slot, eliminating contention, and sums are combined sequentially after parallel work completes. - Add documentation noting floating-point non-determinism in parallel Sum - Fix cache eviction race condition in OneDnnProvider by adding entry first, then evicting if over limit. Uses GetOrAdd for atomic insertion and handles duplicate creation by disposing the duplicate. - Add span length validation to Add and Multiply methods before calling unsafe oneDNN operations to prevent memory corruption with mismatched span lengths. - Fix benchmark in-place modification by using separate destination array instead of writing back to source array which modified test data. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add [MethodImpl(MethodImplOptions.NoInlining)] attribute to MKL.NET helper methods to prevent JIT from trying to load MKL.NET assembly types when the containing method is compiled. This fixes FileNotFoundException on platforms where MKL.NET is not available. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
700468c to
b5a29c4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/AiDotNet.Tensors/Helpers/OneDnnProvider.cs (1)
114-134: Fix incorrect format tag constants.The format tag constants
DnnlFormatTagNCHWandDnnlFormatTagOIHWare set to11, but they should be5. In oneDNN, both NCHW (4D activations) and OIHW (4D weights) map to thednnl_abcdformat tag, which has the numeric value5.The binary constants (
DnnlBinaryAdd = 0x1fff0,DnnlBinaryMul = 0x1fff1) and argument indices (SRC=1, DST=17, WEIGHTS=33, SRC_1=2, SCRATCHPAD=80) are correct and match oneDNN'sdnnl_types.h.
Summary
Benchmark Results
Test plan
🤖 Generated with Claude Code