fix: fix Issue 412 - #433
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 a broad inference-optimization subsystem: platform detection, SIMD kernels, cache/loop optimizers, profiling, custom operator registry, optimized kernels (GEMM/Attention/Convolution), KV-cache paging/quantization, speculative decoding, serving integration, benchmarks, and extensive tests and docs. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-12-18T08:49:25.295ZApplied to files:
📚 Learning: 2025-12-18T08:49:53.103ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (1)
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 introduces a comprehensive Inference Optimization module to AiDotNet, providing hardware-accelerated kernels for critical AI inference operations with automatic platform detection and graceful fallback mechanisms.
Key Changes:
- Adds SIMD-optimized kernels (AVX2, AVX-512, SSE, NEON) for common operations like matrix multiplication, attention, and convolution
- Implements cache-aware CPU optimization utilities with loop tiling and prefetching
- Provides a custom operator registry system with priority-based selection and platform capability matching
- Includes performance profiling infrastructure and comprehensive benchmarking suite
Reviewed Changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/InferenceOptimization/README.md | Documentation covering features, usage examples, and integration guide |
| src/InferenceOptimization/ARCHITECTURE.md | Detailed architecture documentation explaining design patterns and data flow |
| src/InferenceOptimization/PlatformDetector.cs | Hardware capability detection for SIMD instructions and cache sizes |
| src/InferenceOptimization/CustomOperatorRegistry.cs | Thread-safe operator registry with automatic fallback support |
| src/InferenceOptimization/OptimizationInitializer.cs | System initialization and kernel registration entry point |
| src/InferenceOptimization/ICustomOperator.cs | Interface definitions for custom hardware-optimized operators |
| src/InferenceOptimization/Profiling/PerformanceProfiler.cs | Performance tracking with timing and memory statistics |
| src/InferenceOptimization/Kernels/SimdKernels.cs | Low-level SIMD operations for vector math |
| src/InferenceOptimization/Kernels/GemmKernel.cs | Cache-blocked matrix multiplication with parallelization |
| src/InferenceOptimization/Kernels/AttentionKernel.cs | Fused attention implementation for transformer models |
| src/InferenceOptimization/Kernels/ConvolutionKernel.cs | Optimized 2D convolution with depthwise and group variants |
| src/InferenceOptimization/CpuOptimization/CacheOptimizer.cs | Cache-aware algorithms with prefetching and tiling |
| src/InferenceOptimization/CpuOptimization/LoopOptimizer.cs | Loop optimization utilities including tiling and unrolling |
| src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs | Base infrastructure for future GPU kernel implementations |
| src/InferenceOptimization/Examples/BasicUsageExample.cs | Usage examples demonstrating all major features |
| AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs | Benchmarks for SIMD vector operations |
| AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs | Matrix multiplication performance benchmarks |
| AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs | Attention kernel performance benchmarks |
| AiDotNetBenchmarkTests/InferenceOptimization/README.md | Benchmark documentation and interpretation guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Phase 1 of PR #433 integration plan: - Move SimdKernels.cs to AiDotNet.Tensors/Engines/Simd/ - Move PlatformDetector.cs to AiDotNet.Tensors/Engines/ - Update namespaces from AiDotNet.InferenceOptimization to AiDotNet.Tensors.Engines - Fix nullability issues in PlatformCapabilities class - Update all InferenceOptimization files to use new namespace references This integrates core SIMD and platform detection into the unified engine architecture as specified in INTEGRATION_PLAN_PR433.md. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
🤖 PR Title Auto-Fixed Your PR title was automatically updated to follow Conventional Commits format. Original title: New title: Detected type: Valid types and their effects:
If the detected type is incorrect, you can manually edit the PR title. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| new Lazy<PerformanceProfiler>(() => new PerformanceProfiler()); | ||
|
|
||
| private readonly ConcurrentDictionary<string, OperationStats> _stats; | ||
| private readonly ConcurrentStack<ProfileScope> _scopeStack; |
Check failure
Code scanning / CodeQL
Container contents are never accessed Error
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix this issue, the unused field _scopeStack should be removed. This involves deleting its declaration at line 18 and the initialization at line 33 in the constructor. No code elsewhere in the provided file accesses _scopeStack (reads or writes), so removing it will not impact functionality. This cleanup reduces memory usage, improves clarity, and eliminates misleading code. Only the shown file, src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs, needs changing, and no new imports or method definitions are required.
| @@ -15,7 +15,6 @@ | ||
| new Lazy<PerformanceProfiler>(() => new PerformanceProfiler()); | ||
|
|
||
| private readonly ConcurrentDictionary<string, OperationStats> _stats; | ||
| private readonly ConcurrentStack<ProfileScope> _scopeStack; | ||
|
|
||
| /// <summary> | ||
| /// Gets the singleton instance of the profiler | ||
| @@ -30,7 +29,6 @@ | ||
| private PerformanceProfiler() | ||
| { | ||
| _stats = new ConcurrentDictionary<string, OperationStats>(); | ||
| _scopeStack = new ConcurrentStack<ProfileScope>(); | ||
| Enabled = false; | ||
| } | ||
|
|
| if (mask != null) | ||
| { | ||
| int maskIdx = batchIdx * seqLenQ * seqLenK + i * seqLenK + j; | ||
| if (mask.Data[maskIdx] == 0.0f) |
Check warning
Code scanning / CodeQL
Equality check on floating point values Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To resolve this issue, replace the direct floating-point equality check with a comparison that regards values close enough to zero (within a small epsilon tolerance) as zeros. Introduce a constant (e.g., const float EPSILON = 1e-6f;) near the top of the method or class. Then, replace
if (mask.Data[maskIdx] == 0.0f)with
if (MathF.Abs(mask.Data[maskIdx]) < EPSILON)This ensures that any mask value sufficiently close to zero will be treated as zero, making the masking robust to floating-point error.
Since this uses MathF.Abs, which is already used elsewhere in the method, no new imports are needed. Declare EPSILON as a local const float inside the method where the check occurs (to avoid scope issues and maintain clarity).
| @@ -100,8 +100,9 @@ | ||
| // Apply mask if provided | ||
| if (mask != null) | ||
| { | ||
| const float EPSILON = 1e-6f; | ||
| int maskIdx = batchIdx * seqLenQ * seqLenK + i * seqLenK + j; | ||
| if (mask.Data[maskIdx] == 0.0f) | ||
| if (MathF.Abs(mask.Data[maskIdx]) < EPSILON) | ||
| { | ||
| score = float.NegativeInfinity; | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (7)
src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs (1)
58-66: Consider restricting access level tointernalfor the Data property.This property has the same encapsulation and safety concerns as the
Dataproperty inVectorBase.cs:
Design inconsistency:
AsWritableSpan()(line 267) is markedinternal, but thisDataproperty provides equivalent mutable access aspublic.Encapsulation chain: This property exposes
_data.Data, creating a chain of exposure:TensorBase.Data→Vector<T>.Data→T[]. Any external code can now directly mutate the underlying tensor storage.Existing zero-copy alternatives:
AsSpan()(line 251) already provides zero-copy access for high-performance operations without exposing the raw array.If raw array access is truly required for the optimization infrastructure, consider making this
internalto match the access level ofAsWritableSpan()and limit exposure to trusted code.Apply this diff if the property should be internal:
- public T[] Data => _data.Data; + internal T[] Data => _data.Data;src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (2)
67-75: Race condition: mutatingexistinginsideAddOrUpdateis not thread-safe.The update function modifies the
existingobject in place, butConcurrentDictionary.AddOrUpdatedoesn't lock during the update delegate. Concurrent threads can interleave modifications, causing lost updates.Return a new
OperationStatsinstance instead:(_, existing) => { - existing.CallCount++; - existing.TotalTicks += elapsedTicks; - existing.MinTicks = Math.Min(existing.MinTicks, elapsedTicks); - existing.MaxTicks = Math.Max(existing.MaxTicks, elapsedTicks); - existing.TotalMemoryBytes += memoryBytes; - return existing; + return new OperationStats + { + OperationName = existing.OperationName, + CallCount = existing.CallCount + 1, + TotalTicks = existing.TotalTicks + elapsedTicks, + MinTicks = Math.Min(existing.MinTicks, elapsedTicks), + MaxTicks = Math.Max(existing.MaxTicks, elapsedTicks), + TotalMemoryBytes = existing.TotalMemoryBytes + memoryBytes + }; });
142-152: UseGC.GetAllocatedBytesForCurrentThread()for accurate per-operation memory tracking.
GC.GetTotalMemory(false)measures total managed heap size, not allocations by the current operation. Memory deltas can be negative if GC runs during measurement.public ProfileScope(PerformanceProfiler profiler, string operationName) { _profiler = profiler; _operationName = operationName; - _startMemory = GC.GetTotalMemory(false); + _startMemory = GC.GetAllocatedBytesForCurrentThread(); _stopwatch = Stopwatch.StartNew(); } public void Dispose() { _stopwatch.Stop(); - long endMemory = GC.GetTotalMemory(false); + long endMemory = GC.GetAllocatedBytesForCurrentThread(); long memoryDelta = endMemory - _startMemory; _profiler.RecordOperation(_operationName, _stopwatch.ElapsedTicks, memoryDelta); }src/InferenceOptimization/CustomOperatorRegistry.cs (1)
38-53: Race condition betweenAddOrUpdateandTryRemove.Another thread calling
GetOperatorbetween lines 49 and 52 could cache a stale operator selection. The registration and cache invalidation should be atomic.Consider using a lock to ensure atomicity:
+ private readonly object _registrationLock = new object(); + public void Register(ICustomOperator op) { if (op == null) throw new ArgumentNullException(nameof(op)); + lock (_registrationLock) + { _operators.AddOrUpdate( op.Name, _ => new List<ICustomOperator> { op }, (_, list) => { lock (list) { list.Add(op); list.Sort((a, b) => b.Priority.CompareTo(a.Priority)); } return list; }); // Clear cached selection to force re-evaluation _selectedOperators.TryRemove(op.Name, out _); + } }Alternatively, perform cache invalidation inside the
AddOrUpdateupdate function under the same lock as the list modification.src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (1)
31-45: Prefetch methods will throw on non-x86 platforms.Both
PrefetchandPrefetchNonTemporalunconditionally call SSE intrinsics without checkingSse.IsSupported. This will throwPlatformNotSupportedExceptionon ARM or platforms without SSE.Apply this diff to add platform guards:
[MethodImpl(MethodImplOptions.AggressiveInlining)] public static unsafe void Prefetch(void* address) { - // This hints the CPU to fetch data into cache - // Note: .NET JIT may or may not honor this depending on platform - System.Runtime.Intrinsics.X86.Sse.Prefetch0(address); + if (System.Runtime.Intrinsics.X86.Sse.IsSupported) + { + System.Runtime.Intrinsics.X86.Sse.Prefetch0(address); + } + // No-op on unsupported platforms (prefetch is a hint, not required) } [MethodImpl(MethodImplOptions.AggressiveInlining)] public static unsafe void PrefetchNonTemporal(void* address) { - System.Runtime.Intrinsics.X86.Sse.PrefetchNonTemporal(address); + if (System.Runtime.Intrinsics.X86.Sse.IsSupported) + { + System.Runtime.Intrinsics.X86.Sse.PrefetchNonTemporal(address); + } }src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
93-103:DetectCudaSupport()is misleading and may cause incorrect GPU kernel activation.This method returns
truefor any 64-bit Windows/Linux process, regardless of whether CUDA is actually installed or a compatible GPU exists. SinceGpuKernelBase.IsSupported()relies onHasCudaSupport, this could cause runtime failures when GPU kernels are selected but cannot execute.Consider renaming to
IsCudaCapablePlatform()and returningfalseuntil actual CUDA detection is implemented, or add a clear warning in the property documentation.private static bool DetectCudaSupport() { - // This would require native CUDA library calls - // For now, we'll check if we're on Windows/Linux x64 - if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows) || - RuntimeInformation.IsOSPlatform(OSPlatform.Linux)) - { - return Environment.Is64BitProcess; - } + // TODO: Implement actual CUDA detection via cudart library or nvml + // Returning false until proper detection is implemented to avoid + // false positives that could cause runtime failures in GPU kernels return false; }src/InferenceOptimization/Kernels/AttentionKernel.cs (1)
101-108: Floating-point equality comparison may yield unexpected results.Direct equality checks on floating-point values can fail due to precision issues. Use an epsilon-based comparison instead.
// Apply mask if provided if (mask != null) { int maskIdx = batchIdx * seqLenQ * seqLenK + i * seqLenK + j; - if (mask.Data[maskIdx] == 0.0f) + if (MathF.Abs(mask.Data[maskIdx]) < 1e-6f) { score = float.NegativeInfinity; } }
🧹 Nitpick comments (17)
tests/AiDotNet.Tests/StressTests/GpuStressTests.cs (1)
204-213: Degradation metric logic is sound; consider small threshold and message tweaksThe new
performanceDegradationcalculation correctly:
- Avoids division by zero (
firstQuartileAvg > 0), and- Treats only slowdowns (
lastQuartileAvg > firstQuartileAvg) as degradation, ignoring improvements.Two minor polish points you may want to consider:
Assertion vs. message semantics
Comment says “should not degrade by more than 20%”, but the check is:
Assert.True(performanceDegradation < 0.20, ...);This fails at exactly 20% degradation (0.20). If you really mean “more than 20%”, either:
- change to
<= 0.20, or- reword the message to “should not degrade by 20% or more”.
Zero‑baseline edge case
With millisecond timing, it’s possible (on very fast hardware) that
firstQuartileAvg == 0andlastQuartileAvg > 0. In that case,performanceDegradationstays 0 and you won’t detect a slowdown. If you care about that ultra‑fast edge case, you could optionally:
- fall back to an absolute check when
firstQuartileAvg == 0(e.g., assertlastQuartileAvg < someAbsoluteMsThreshold).Both are minor; current logic is otherwise correct and robust for typical hardware.
src/InferenceOptimization/OptimizationInitializer.cs (2)
60-78: Consider making console logging optional or configurable.The
LogPlatformInfomethod writes directly toConsole.WriteLine, which may not be appropriate for all usage scenarios (e.g., library integration, headless services, or when users want to control logging output). Consider adding a parameter to control logging or using a configurable logging abstraction.- public static void Initialize(bool enableProfiling = false) + public static void Initialize(bool enableProfiling = false, bool logPlatformInfo = true) { lock (_lock) { if (_initialized) return; // Enable profiling if requested PerformanceProfiler.Instance.Enabled = enableProfiling; // Register optimized kernels RegisterKernels(); // Print platform capabilities - LogPlatformInfo(); + if (logPlatformInfo) + LogPlatformInfo(); _initialized = true; } }
83-90: Verify GetPerformanceSummary behavior when profiling is disabled.When profiling is disabled,
PerformanceProfiler.Instance.GenerateReport()may return "No profiling data available." Consider documenting this behavior or returning a more informative message that indicates profiling was disabled.public static string GetPerformanceSummary() { if (!_initialized) return "Optimization system not initialized."; + + if (!PerformanceProfiler.Instance.Enabled) + return "Profiling is disabled. Call Initialize(enableProfiling: true) to enable profiling."; var report = PerformanceProfiler.Instance.GenerateReport(); return report; }AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs (1)
39-47: Consider simplifying matrix initialization.The matrix data initialization could be more concise using LINQ or a helper method, though the current approach is clear and explicit.
- for (int i = 0; i < _matrixA.Data.Length; i++) - { - _matrixA.Data[i] = (float)random.NextDouble(); - } - - for (int i = 0; i < _matrixB.Data.Length; i++) - { - _matrixB.Data[i] = (float)random.NextDouble(); - } + // Initialize with random data + Array.ForEach(_matrixA.Data, (ref float x) => x = (float)random.NextDouble()); + Array.ForEach(_matrixB.Data, (ref float x) => x = (float)random.NextDouble()); + + // Or using for loops for clarity: + for (int i = 0; i < _matrixA.Data.Length; i++) + _matrixA.Data[i] = (float)random.NextDouble(); + for (int i = 0; i < _matrixB.Data.Length; i++) + _matrixB.Data[i] = (float)random.NextDouble();AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs (1)
44-51: Consider separating benchmarks by operation type for clearer baseline comparisons.The
Baseline = trueonVectorAdd_Scalarapplies to all benchmarks in the class, soDotProduct_SIMDwill show a ratio relative toVectorAdd_Scalarrather thanDotProduct_Scalar. This could produce misleading comparisons.Consider using
[BenchmarkCategory]attributes to group each operation pair, or split into separate benchmark classes (e.g.,VectorAddBenchmark,DotProductBenchmark).src/InferenceOptimization/Kernels/GemmKernel.cs (1)
115-148: Consider hoistingfixedstatement outside parallel loop.The
fixedstatement inside theParallel.Forlambda creates a GC handle per parallel iteration, adding overhead. Since the arrays don't change during execution, pinning once before the parallel loop would be more efficient.private unsafe void GemmParallel(float[] A, float[] B, float[] C, int M, int N, int K) { + fixed (float* pA = A, pB = B, pC = C) + { // Parallelize over rows of A Parallel.For(0, (M + BlockSize - 1) / BlockSize, iBlock => { int i = iBlock * BlockSize; int iMax = Math.Min(i + BlockSize, M); - fixed (float* pA = A, pB = B, pC = C) - { for (int j = 0; j < N; j += BlockSize) { // ... inner loop unchanged } - } }); + } }src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
131-131: Mark private nested classes assealed.
ProfileScopeandEmptyDisposableare private and not intended for inheritance. Marking themsealedenables compiler optimizations and clarifies intent.- private class ProfileScope : IDisposable + private sealed class ProfileScope : IDisposable- private class EmptyDisposable : IDisposable + private sealed class EmptyDisposable : IDisposableAlso applies to: 160-160
src/InferenceOptimization/CustomOperatorRegistry.cs (2)
63-75: SimplifyGetOperatorby avoiding redundant dictionary access.
GetOrAddreturns the value directly, so accessing_selectedOperators[name]again on line 74 is redundant and could theoretically return a different value if the cache was modified concurrently.public ICustomOperator? GetOperator(string name) { if (string.IsNullOrEmpty(name)) throw new ArgumentException("Operator name cannot be null or empty", nameof(name)); - return _selectedOperators.GetOrAdd(name, key => + var result = _selectedOperators.GetOrAdd(name, key => { if (!_operators.TryGetValue(key, out var candidates)) return new NullOperator(); lock (candidates) { - // Find the highest priority supported operator var result = candidates.FirstOrDefault(op => op.IsSupported()); return result ?? new NullOperator(); } - }) is NullOperator ? null : _selectedOperators[name]; + }); + + return result is NullOperator ? null : result; }
151-155: Minor:Clear()is not atomic.A
GetOperatorcall between the twoClear()operations could see inconsistent state. If this method is only used in tests or initialization, this is acceptable; otherwise consider adding synchronization.src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (1)
304-313: Consider SIMD-optimizedExpfor softmax-heavy workloads.The scalar fallback is functionally correct, but this could become a bottleneck in attention mechanisms where
Expis called frequently for softmax. A polynomial approximation (e.g., Remez or minimax) could provide significant speedups while maintaining acceptable accuracy.src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (1)
110-128: Consider usingBuffer.MemoryCopyor SIMD for the copy loop.The element-by-element copy will be slower than
Buffer.MemoryCopyor SIMD-vectorized copy. The prefetching benefit may be negated by the slow copy. Note thatPrefetchalso needs the platform guard fix mentioned earlier.src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs (1)
62-109: Delegate overhead may negate unrolling benefits.The
Action<int>delegate invocations cannot be inlined by the JIT, so the unrolling benefit is limited to reducing loop control overhead. For performance-critical paths, consider usingSpan<T>-based APIs or direct inline code rather than delegate-based patterns.src/InferenceOptimization/Kernels/ConvolutionKernel.cs (1)
233-248: Parallelization may be suboptimal whengroupsis small.If
groupsis small (e.g., 2-4 for common group convolutions), parallelizing only over groups underutilizes available cores. Consider flattening the parallelization overgroups * batchSizeorgroups * batchSize * outChannelsPerGroupfor better load distribution.- // Process each group independently - Parallel.For(0, groups, g => + // Flatten parallelization for better load distribution + int totalWork = groups * batchSize * outChannelsPerGroup; + Parallel.For(0, totalWork, workIdx => { - for (int b = 0; b < batchSize; b++) - { - for (int oc = 0; oc < outChannelsPerGroup; oc++) - { - int globalOutChannel = g * outChannelsPerGroup + oc; + int g = workIdx / (batchSize * outChannelsPerGroup); + int remaining = workIdx % (batchSize * outChannelsPerGroup); + int b = remaining / outChannelsPerGroup; + int oc = remaining % outChannelsPerGroup; + int globalOutChannel = g * outChannelsPerGroup + oc; - GroupConv2DSingleOutput(input, kernel, output, b, globalOutChannel, g, - inChannelsPerGroup, inHeight, inWidth, - kernelH, kernelW, stride, padding, - outHeight, outWidth); - } - } + GroupConv2DSingleOutput(input, kernel, output, b, globalOutChannel, g, + inChannelsPerGroup, inHeight, inWidth, + kernelH, kernelW, stride, padding, + outHeight, outWidth); });src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (1)
136-187: Consider usingInterlockedfor better performance.The lock-based synchronization works correctly but may cause contention in high-throughput scenarios.
Interlockedoperations would be more efficient for simple counter updates.public static class GpuMemoryManager { private static long _allocatedBytes = 0; - private static readonly object _lock = new object(); public static long AllocatedBytes { - get - { - lock (_lock) - { - return _allocatedBytes; - } - } + get => Interlocked.Read(ref _allocatedBytes); } internal static void TrackAllocation(long bytes) { - lock (_lock) - { - _allocatedBytes += bytes; - } + Interlocked.Add(ref _allocatedBytes, bytes); } internal static void TrackDeallocation(long bytes) { - lock (_lock) - { - _allocatedBytes -= bytes; - } + Interlocked.Add(ref _allocatedBytes, -bytes); } public static string GetMemoryInfo() { - lock (_lock) - { - return $"GPU Memory Allocated: {_allocatedBytes / (1024.0 * 1024.0):F2} MB"; - } + var allocated = Interlocked.Read(ref _allocatedBytes); + return $"GPU Memory Allocated: {allocated / (1024.0 * 1024.0):F2} MB"; } }src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
75-91: Hardcoded cache sizes may lead to suboptimal optimization decisions.The cache estimation methods return fixed values regardless of the actual hardware. This could cause CacheOptimizer and tiling algorithms to make suboptimal decisions on systems with significantly different cache configurations (e.g., server CPUs with larger caches, or embedded systems with smaller ones).
Consider adding a TODO or logging a warning when these estimates are used, or investigating platform-specific APIs (e.g.,
GetLogicalProcessorInformationon Windows,/sys/devices/system/cpu/on Linux) for more accurate detection.
168-207: Consider makingPlatformCapabilitiesimmutable.The class uses mutable auto-properties which could be modified after detection. Since capabilities are detected once at startup and should not change, making the class immutable (e.g., using
initsetters or a constructor) would prevent accidental modification and make the intent clearer.src/InferenceOptimization/Kernels/AttentionKernel.cs (1)
221-244: Index calculation is correct for the expected input layout; consider performance optimization with batch copying.The source and destination index calculations are correct and properly handle the input layout
[batch, seq_len, d_model]whered_model = num_heads * dKand heads are interleaved within the feature dimension.The nested loop performs element-by-element copying with poor cache locality. To improve performance for large tensors, consider batch-copying contiguous
dK-sized segments per sequence per head instead of copying individual floats, or use SIMD operations to vectorize the memory transfers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/README.md(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs(1 hunks)INTEGRATION_PLAN_PR433.md(1 hunks)src/AiDotNet.Tensors/Engines/GpuEngine.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs(1 hunks)src/AiDotNet.Tensors/Engines/PlatformDetector.cs(1 hunks)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs(1 hunks)src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs(1 hunks)src/AiDotNet.Tensors/LinearAlgebra/VectorBase.cs(1 hunks)src/AiDotNet.csproj(1 hunks)src/InferenceOptimization/ARCHITECTURE.md(1 hunks)src/InferenceOptimization/CustomOperatorRegistry.cs(1 hunks)src/InferenceOptimization/Examples/OptimizationExample.cs(0 hunks)src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs(1 hunks)src/InferenceOptimization/ICustomOperator.cs(1 hunks)src/InferenceOptimization/Kernels/AttentionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/ConvolutionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/GemmKernel.cs(1 hunks)src/InferenceOptimization/OptimizationInitializer.cs(1 hunks)src/InferenceOptimization/README.md(4 hunks)tests/AiDotNet.Tests/StressTests/GpuStressTests.cs(1 hunks)
💤 Files with no reviewable changes (1)
- src/InferenceOptimization/Examples/OptimizationExample.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/AiDotNet.Tensors/Engines/GpuEngine.cssrc/InferenceOptimization/GpuOptimization/GpuKernelBase.cs
🧬 Code graph analysis (12)
src/AiDotNet.Tensors/LinearAlgebra/VectorBase.cs (4)
src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs (1)
T(282-287)src/Helpers/ConversionsHelper.cs (1)
T(119-153)src/AiDotNet.Tensors/LinearAlgebra/Vector.cs (2)
T(210-214)T(413-418)src/AiDotNet.Tensors/LinearAlgebra/Tensor.cs (4)
T(574-581)T(946-963)T(1618-1624)T(1739-1744)
src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs (1)
src/AiDotNet.Tensors/LinearAlgebra/VectorBase.cs (4)
T(146-149)T(338-342)T(355-358)T(370-376)
src/InferenceOptimization/OptimizationInitializer.cs (7)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (4)
PerformanceProfiler(12-165)PerformanceProfiler(30-35)GenerateReport(105-129)Clear(97-100)src/InferenceOptimization/CustomOperatorRegistry.cs (6)
CustomOperatorRegistry(11-156)CustomOperatorRegistry(24-28)Register(33-53)IsSupported(93-93)EstimatedSpeedup(94-94)Clear(151-155)src/InferenceOptimization/Kernels/GemmKernel.cs (3)
GemmKernel(14-185)IsSupported(23-27)EstimatedSpeedup(29-36)src/InferenceOptimization/Kernels/AttentionKernel.cs (4)
AttentionKernel(12-270)AttentionKernel(20-23)IsSupported(25-28)EstimatedSpeedup(30-34)src/InferenceOptimization/Kernels/ConvolutionKernel.cs (3)
ConvolutionKernel(11-298)IsSupported(17-20)EstimatedSpeedup(22-28)src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
PlatformDetector(12-162)GetCapabilitiesDescription(115-161)src/InferenceOptimization/ICustomOperator.cs (2)
IsSupported(29-29)EstimatedSpeedup(35-35)
src/InferenceOptimization/ICustomOperator.cs (4)
src/InferenceOptimization/CustomOperatorRegistry.cs (4)
ICustomOperator(58-75)ICustomOperator(80-83)IsSupported(93-93)EstimatedSpeedup(94-94)src/InferenceOptimization/Kernels/AttentionKernel.cs (6)
IsSupported(25-28)EstimatedSpeedup(30-34)Tensor(36-72)Tensor(191-219)Tensor(221-244)Tensor(246-269)src/InferenceOptimization/Kernels/ConvolutionKernel.cs (6)
IsSupported(17-20)EstimatedSpeedup(22-28)Tensor(30-33)Tensor(38-77)Tensor(124-160)Tensor(203-251)src/InferenceOptimization/Kernels/GemmKernel.cs (4)
IsSupported(23-27)EstimatedSpeedup(29-36)Tensor(38-69)Tensor(153-184)
src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (4)
src/InferenceOptimization/Kernels/GemmKernel.cs (3)
MethodImpl(74-109)MethodImpl(114-148)IsSupported(23-27)src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (2)
IsSupported(21-25)IsSupported(121-124)src/InferenceOptimization/Kernels/AttentionKernel.cs (1)
IsSupported(25-28)src/InferenceOptimization/Kernels/ConvolutionKernel.cs (1)
IsSupported(17-20)
AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs (4)
src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (1)
Tensor(33-33)src/InferenceOptimization/Kernels/GemmKernel.cs (3)
Tensor(38-69)Tensor(153-184)GemmKernel(14-185)src/InferenceOptimization/ICustomOperator.cs (1)
Tensor(46-46)src/InferenceOptimization/OptimizationInitializer.cs (2)
OptimizationInitializer(11-107)Initialize(19-37)
AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs (6)
src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (1)
Tensor(33-33)src/InferenceOptimization/Kernels/AttentionKernel.cs (6)
Tensor(36-72)Tensor(191-219)Tensor(221-244)Tensor(246-269)AttentionKernel(12-270)AttentionKernel(20-23)src/InferenceOptimization/Kernels/ConvolutionKernel.cs (4)
Tensor(30-33)Tensor(38-77)Tensor(124-160)Tensor(203-251)src/InferenceOptimization/Kernels/GemmKernel.cs (2)
Tensor(38-69)Tensor(153-184)src/InferenceOptimization/ICustomOperator.cs (1)
Tensor(46-46)src/InferenceOptimization/OptimizationInitializer.cs (2)
OptimizationInitializer(11-107)Initialize(19-37)
src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (2)
src/InferenceOptimization/CustomOperatorRegistry.cs (4)
ICustomOperator(58-75)ICustomOperator(80-83)IsSupported(93-93)EstimatedSpeedup(94-94)src/InferenceOptimization/ICustomOperator.cs (3)
IsSupported(29-29)EstimatedSpeedup(35-35)Tensor(46-46)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
src/Diagnostics/ProfilerScope.cs (1)
Profile(129-135)
src/InferenceOptimization/Kernels/ConvolutionKernel.cs (2)
src/InferenceOptimization/ICustomOperator.cs (3)
IsSupported(29-29)EstimatedSpeedup(35-35)Tensor(46-46)src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
PlatformDetector(12-162)
src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs (1)
src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
PlatformDetector(12-162)
src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (2)
IsSupported(21-25)IsSupported(121-124)
🪛 GitHub Check: SonarCloud Analysis
src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs
[warning] 19-19:
Make sure that using "unsafe" is safe here. (https://rules.sonarsource.com/csharp/RSPEC-6640)
[failure] 5-5:
The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)
[failure] 4-4:
The type or namespace name 'X86' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs
[warning] 131-131:
Fix this implementation of 'IDisposable' to conform to the dispose pattern. (https://rules.sonarsource.com/csharp/RSPEC-3881)
[warning] 160-160:
Fix this implementation of 'IDisposable' to conform to the dispose pattern. (https://rules.sonarsource.com/csharp/RSPEC-3881)
[warning] 18-18:
Remove this unread private field '_scopeStack' or refactor the code to use its value. (https://rules.sonarsource.com/csharp/RSPEC-4487)
[warning] 160-160:
Private classes which are not derived in the current assembly should be marked as 'sealed'. (https://rules.sonarsource.com/csharp/RSPEC-3260)
[warning] 131-131:
Private classes which are not derived in the current assembly should be marked as 'sealed'. (https://rules.sonarsource.com/csharp/RSPEC-3260)
src/AiDotNet.Tensors/Engines/PlatformDetector.cs
[failure] 3-3:
The type or namespace name 'X86' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)
[failure] 4-4:
The type or namespace name 'Arm' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)
[failure] 3-3:
The type or namespace name 'X86' does not exist in the namespace 'System.Runtime.Intrinsics' (are you missing an assembly reference?)
AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs
[failure] 146-146:
Unsafe code may only appear if compiling with /unsafe
[failure] 122-122:
Unsafe code may only appear if compiling with /unsafe
[failure] 100-100:
Unsafe code may only appear if compiling with /unsafe
[failure] 76-76:
Unsafe code may only appear if compiling with /unsafe
[failure] 54-54:
Unsafe code may only appear if compiling with /unsafe
🪛 LanguageTool
INTEGRATION_PLAN_PR433.md
[grammar] ~194-~194: Ensure spelling is correct
Context: ...through IEngine 2. SIMD Everywhere: CpuEngine automatically uses SIMD for float opera...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
AiDotNetBenchmarkTests/InferenceOptimization/README.md
[style] ~138-~138: To elevate your writing, try using an alternative expression here.
Context: ... matrix operations - Memory channels matter for parallel operations 4. **Thermal T...
(MATTERS_RELEVANT)
🪛 markdownlint-cli2 (0.18.1)
src/InferenceOptimization/ARCHITECTURE.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
80-80: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
108-108: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
180-180: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
202-202: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
242-242: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
AiDotNetBenchmarkTests/InferenceOptimization/README.md
86-86: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
91-91: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL Analysis
🔇 Additional comments (51)
src/AiDotNet.Tensors/LinearAlgebra/VectorBase.cs (1)
77-85: The publicDataproperty is intentionally exposed for the SIMD optimization infrastructure and cannot be made internal.The property is actively used in multiple high-performance kernel implementations (GemmKernel, ConvolutionKernel, AttentionKernel) specifically for
fixedpointer operations in unsafe code blocks, which is the required pattern for low-level SIMD operations. Thefixedstatement syntax requires direct array access and cannot useSpan<T>in this context.While
AsSpan()andAsWritableSpan()are used within VectorBase itself for internal operations passed to the_numOpsmethods, they do not serve the same purpose as the exposedDataproperty. The optimization kernels need raw array pointers for efficient unsafe pointer arithmetic in performance-critical code paths.This is a deliberate design choice as part of the SIMD optimization phase, not an encapsulation violation. The property's public exposure is necessary and correct.
Likely an incorrect or invalid review comment.
src/AiDotNet.Tensors/Engines/GpuEngine.cs (1)
1051-1052: LGTM! ILGPU.Algorithms is properly referenced.The builder pattern with
EnableAlgorithms()is the correct approach for enabling extended math operations (likeRoundToEven) in ILGPU. The explanatory comment is helpful. Verified thatILGPU.Algorithmsversion 1.5.3 is already referenced in the project file.src/AiDotNet.csproj (1)
6-6: LGTM! Necessary infrastructure for SIMD optimizations.Enabling unsafe blocks is required for the SIMD kernels and low-level optimizations introduced in this PR. The unsafe code is confined to performance-critical paths in SimdKernels and optimized kernel implementations.
AiDotNetBenchmarkTests/InferenceOptimization/README.md (1)
1-209: Excellent benchmark documentation!The README provides comprehensive guidance on running benchmarks, interpreting results, and understanding performance targets. The documentation covers all necessary aspects including troubleshooting, CI/CD integration, and platform-specific considerations.
src/InferenceOptimization/ARCHITECTURE.md (1)
1-436: Excellent architectural documentation!This document provides a comprehensive overview of the inference optimization module's architecture, including design goals, component responsibilities, data flow, and extensibility patterns. The level of detail is appropriate for both users and contributors.
INTEGRATION_PLAN_PR433.md (2)
1-257: Well-structured integration plan.This document provides a clear roadmap for integrating the InferenceOptimization module into the existing architecture. The identification of API mismatches (Dimensions vs Shape, Data access) and the proposed remediation steps are comprehensive.
34-39: The identified API mismatches are not present in the current code.The InferenceOptimization kernels are using valid APIs. The actual Tensor class exposes:
Shapeproperty (public) ✓Dataproperty (public, not protected) ✓new Tensor<float>(int[])constructor ✓The code does not use
.Dimensions, correctly uses.Shape, and accesses.Dataas a public property. No 130+ build errors would occur.src/InferenceOptimization/README.md (2)
1-322: Comprehensive and well-organized README.The documentation effectively covers both kernel-level and graph-level optimizations with clear examples, performance targets, and usage patterns. The organization makes it easy for users to find relevant information.
146-171: The code examples are correct and will compile with the current codebase.The Tensor API verification confirms:
Tensor<float>constructor withint[]parameter is public (public Tensor(int[] dimensions))Dataproperty is publicly accessible via the base class (public T[] Data => _data.Data;)- All kernel methods (
GemmKernel.Execute(),AttentionKernel.Execute()) exist with correct signaturesOptimizationInitializer.Initialize()andGetPerformanceSummary()methods are implemented as shownThe examples can be used as written without modification.
src/InferenceOptimization/OptimizationInitializer.cs (1)
11-37: LGTM! Thread-safe initialization pattern is correct.The initialization logic properly uses a lock and flag for thread-safe, idempotent initialization. The sequence of enabling profiling, registering kernels, and logging platform info is logical.
AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs (2)
18-48: LGTM! Benchmark setup is well-structured.The benchmark class properly initializes the optimization system and prepares test data with a fixed random seed for reproducibility. The use of BenchmarkDotNet attributes is appropriate.
50-70: NaiveGemm baseline implementation is correct.The triple-nested loop correctly implements matrix multiplication and provides a good baseline for comparison. Marking it with
Baseline = trueis appropriate.AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs (3)
17-57: LGTM! Benchmark setup correctly initializes attention tensors.The setup properly creates Q, K, V tensors with shape
[1, SequenceLength, FeatureDim]and initializes them with random data using a fixed seed for reproducibility.
59-121: NaiveAttention baseline implementation is correct.The implementation correctly follows the attention mechanism:
- Compute QK^T scores with proper scaling (1/√d_k)
- Apply softmax with numerical stability (subtract max before exp)
- Compute weighted sum with V
This provides an accurate baseline for comparison.
129-133: Verify multi-head attention divisibility requirement.The
MultiHeadAttentionbenchmark usesnumHeads: 8withFeatureDimvalues of 32 and 64. Both are divisible by 8, which is correct. However, if the parameter set changes in the future, this could cause issues.Consider adding a comment or validation to document the requirement:
[Benchmark] public Tensor<float> MultiHeadAttention() { + // FeatureDim must be divisible by numHeads (8) + // Current params (32, 64) satisfy this requirement return _attentionKernel.MultiHeadAttention(_q, _k, _v, numHeads: 8); }AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs (1)
25-40: LGTM!Setup correctly disables profiling to avoid measurement overhead and uses a deterministic seed for reproducible benchmark results.
src/InferenceOptimization/ICustomOperator.cs (2)
9-36: LGTM!Clean interface design with appropriate metadata (Name, Version, Priority) and capability querying (IsSupported, EstimatedSpeedup). The priority-based selection enables graceful fallbacks.
38-47: LGTM!The generic interface with
paramsarray provides flexibility for operators requiring different input counts (e.g., GEMM uses 2, Attention uses 3+).src/InferenceOptimization/Kernels/GemmKernel.cs (3)
16-17: LGTM!Block size of 64 is well-tuned for typical L1 cache sizes, and the parallel threshold provides reasonable heuristics for switching strategies.
153-184: LGTM!The transpose-B variant correctly validates dimensions and uses dot products for efficient row-by-row computation. The parallelization over result rows is appropriate.
56-68: Remove this review comment — the concern is incorrect.In C#/.NET,
new T[length]automatically zero-initializes all array elements to their default value (0.0f for float). This happens in VectorBase's constructor when it creates_data = new T[length]. The chain is:new Tensor<float>(...) → new Vector<T>(size) → VectorBase(length) → new T[length], and the runtime guarantees zero-initialization at the final step. The GEMM implementation correctly relies on this standard .NET behavior and does not require explicit zeroing.Likely an incorrect or invalid review comment.
src/InferenceOptimization/CustomOperatorRegistry.cs (1)
125-146: LGTM!
GetOperatorInfocorrectly locks each candidate list during iteration and returns a fresh dictionary, preventing external mutation of internal state.src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (6)
18-65: LGTM!The
VectorAddimplementation correctly handles AVX2, SSE, and NEON paths with proper alignment masking and scalar fallback for remaining elements.
70-113: LGTM!The
VectorMultiplyimplementation follows the same correct pattern asVectorAdd.
118-192: LGTM!The
DotProductimplementation is well-optimized with FMA support when available and correct horizontal reduction patterns for all SIMD paths.
197-248: LGTM!The
ScalarMultiplyAdd(AXPY-style) implementation correctly uses FMA when available and is properly integrated withGemmKernelfor SIMD-optimized inner loops.
253-299: LGTM!The
ReLUactivation is correctly implemented usingMaxwith zero vector for all SIMD paths.
318-383: LGTM!The
Sumreduction correctly reuses the same horizontal reduction patterns asDotProduct.src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (4)
50-79: LGTM!The
ComputeOptimalTilingmethod uses a reasonable heuristic based on L1 cache size with power-of-2 alignment for better memory access patterns.
84-105: LGTM!The blocked transpose correctly uses cache-friendly block iteration with proper index calculations.
133-168: LGTM!Morton/Z-order encoding is correctly implemented with standard bit-interleaving patterns.
173-195: LGTM!The cache miss estimation provides a useful heuristic for comparative analysis of access patterns.
src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs (7)
15-31: LGTM!The
Tile2Dmethod correctly implements 2D loop tiling with proper boundary handling.
36-57: LGTM!The
Tile3Dmethod correctly extends the tiling pattern to three dimensions.
114-121: LGTM!The
StripMinemethod provides a clean abstraction for chunked iteration.
126-135: LGTM!The
Fusemethod provides a useful pattern for executing multiple operations in a single pass to improve cache locality.
141-168: LGTM!The
OptimalOrder2Dmethod correctly provides selectable traversal order for cache-friendly access patterns.
173-195: LGTM!The
ParallelTile2Dmethod correctly distributes tiled work across threads with proper coordinate reconstruction.
200-216: LGTM!The
DetermineOptimalTileSizemethod provides a reasonable cache-aware tile size calculation.src/InferenceOptimization/Kernels/ConvolutionKernel.cs (5)
38-77: LGTM!The
Conv2Dmethod correctly validates input shapes, computes output dimensions, and parallelizes over batch and output channels.
79-119: LGTM!The
Conv2DSingleOutputhelper has correct index calculations for NCHW tensor layout with proper boundary handling for padding.
124-160: LGTM!The
DepthwiseConv2Dmethod correctly handles per-channel convolution with appropriate shape validation.
162-198: LGTM!The
DepthwiseConv2DSingleChannelcorrectly indexes the kernel for the [channels, 1, kernel_h, kernel_w] shape.
253-297: LGTM!The
GroupConv2DSingleOutputcorrectly handles group convolution indexing with per-group channel slicing.src/InferenceOptimization/GpuOptimization/GpuKernelBase.cs (4)
12-33: LGTM!The
GpuKernelBase<T>abstract class provides a clean foundation for GPU kernel implementations with appropriate defaults and capability checks.
38-92: Placeholder methods are appropriately stubbed for future implementation.The infrastructure is ready for CUDA/OpenCL integration. Based on learnings, when implementing actual GPU operations, use standard .NET exception types (
InvalidOperationException,ArgumentException,OutOfMemoryException) with message-based filtering rather than library-specific exceptions for better version compatibility.
98-107: LGTM!The
GpuDeviceInfoclass provides a clean data container for GPU device properties.
119-131: LGTM!The
CudaKernelBase<T>correctly specializes the base class for CUDA-specific capability checks. Note that actual CUDA detection requires native library checks (currently stubbed inPlatformDetector).src/InferenceOptimization/Kernels/AttentionKernel.cs (3)
174-183: Edge case: softmax with all-Infinityscores produces NaN.If all elements in a row are
float.NegativeInfinity(e.g., all positions masked),sumwill be0.0fafter the loop, and the normalization is skipped. This leaves the row as all zeros, which is correct. However, ifmaxValremainsfloat.NegativeInfinity, theexp(row[j] - maxVal)computation could produceNaNfor non-masked values.The current code handles this correctly by checking
IsNegativeInfinitybefore the exp calculation, but consider adding a comment to clarify this edge case behavior for maintainability.
36-72: Input validation and parallel batch processing look correct.The method properly validates tensor dimensions, checks Q/K feature dimension alignment, and verifies K/V sequence length consistency. The parallel batch processing via
Parallel.Foris appropriate for independent batch computations.
246-269:ReshapeFromMultiHeadinverse operation appears consistent.The inverse reshape correctly reconstructs the original tensor layout by reversing the head splitting. The indexing mirrors
ReshapeForMultiHeadappropriately.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/InferenceOptimization/ARCHITECTURE.md (1)
17-64: Code block lacks language identifier.The architecture diagram uses a fenced code block without a language identifier. Adding
textwould improve rendering consistency.-``` +```text ┌─────────────────────────────────────────────────────────────────┐src/InferenceOptimization/CustomOperatorRegistry.cs (1)
40-57: Race condition still exists betweenAddOrUpdateandTryRemove.While the code now creates new lists to avoid mutation races, there's still a window between completing
AddOrUpdate(line 54) andTryRemove(line 57) where another thread callingGetOperatorcould cache the old operator selection. This is the same issue flagged in past reviews.Consider using a single lock for both operations, or accept this as a benign race (stale cache entry will be correct on next lookup after cache is invalidated).
+ private readonly object _registrationLock = new object(); + public void Register(ICustomOperator op) { if (op == null) throw new ArgumentNullException(nameof(op)); - _operators.AddOrUpdate( - op.Name, - _ => new List<ICustomOperator> { op }, - (_, existingList) => - { - List<ICustomOperator> newList; - lock (existingList) - { - newList = new List<ICustomOperator>(existingList) { op }; - } - newList.Sort((a, b) => b.Priority.CompareTo(a.Priority)); - return newList; - }); - - _selectedOperators.TryRemove(op.Name, out _); + lock (_registrationLock) + { + _selectedOperators.TryRemove(op.Name, out _); + _operators.AddOrUpdate( + op.Name, + _ => new List<ICustomOperator> { op }, + (_, existingList) => + { + var newList = new List<ICustomOperator>(existingList) { op }; + newList.Sort((a, b) => b.Priority.CompareTo(a.Priority)); + return newList; + }); + } }src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (2)
31-42: SSE support check has been added - previous concern addressed.The
Prefetchmethod now correctly guards the SSE intrinsic call with both a conditional compilation directive and a runtimeSse.IsSupportedcheck, providing safe fallback behavior on non-x86 platforms.
48-57: SSE support check has been added - previous concern addressed.The
PrefetchNonTemporalmethod now correctly guards the SSE intrinsic call with both conditional compilation and runtime support check.src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
1-6: Conditional compilation for intrinsics namespaces - previous concern addressed.The intrinsics namespaces are now properly guarded with
#if NET5_0_OR_GREATER, allowing the file to compile on .NET Framework 4.7.1.
97-113: CUDA detection clarification is helpful, but property name may still mislead.The documentation now clearly states this is "platform capability" not actual CUDA availability. However, the
HasCudaSupportproperty name inPlatformCapabilitiesmay still mislead consumers into thinking CUDA is actually available.Consider renaming to
IsCudaCapablePlatformorCanSupportCudato better reflect the actual check:-public bool HasCudaSupport { get; set; } +public bool IsCudaCapablePlatform { get; set; }
🧹 Nitpick comments (9)
src/InferenceOptimization/Kernels/GemmKernel.cs (1)
74-75:AggressiveInliningis ineffective on large methods.
MethodImplOptions.AggressiveInlininghas no practical effect onGemmBlockedandGemmParallelbecause these methods are too large to be inlined by the JIT. Consider removing the attribute to avoid misleading hints, or extract the innermost loop into a separate inlinable helper.- [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe void GemmBlocked(float[] A, float[] B, float[] C, int M, int N, int K)- [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe void GemmParallel(float[] A, float[] B, float[] C, int M, int N, int K)Also applies to: 114-115
src/InferenceOptimization/Kernels/ConvolutionKernel.cs (1)
268-283: Consider parallelizingGroupConv2Dover batch dimension as well.Currently parallelization is only over
groups. For workloads with large batches but few groups (common with groups=2 or groups=4), this underutilizes available threads.- Parallel.For(0, groups, g => + // Parallelize over groups and batches for better utilization + Parallel.For(0, groups * batchSize, idx => { + int g = idx / batchSize; + int b = idx % batchSize; - for (int b = 0; b < batchSize; b++) + for (int oc = 0; oc < outChannelsPerGroup; oc++) { - for (int oc = 0; oc < outChannelsPerGroup; oc++) - { - int globalOutChannel = g * outChannelsPerGroup + oc; + int globalOutChannel = g * outChannelsPerGroup + oc; - GroupConv2DSingleOutput(input, kernel, output, b, globalOutChannel, g, - inChannelsPerGroup, inHeight, inWidth, - kernelH, kernelW, stride, padding, - outHeight, outWidth); - } + GroupConv2DSingleOutput(input, kernel, output, b, globalOutChannel, g, + inChannelsPerGroup, inHeight, inWidth, + kernelH, kernelW, stride, padding, + outHeight, outWidth); } });src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (3)
12-25: Block size constants are reasonable defaults.The L1/L2/L3 block size constants are appropriate for typical cache configurations. However, consider whether these should be dynamically derived from
PlatformDetector.Capabilities(likeComputeOptimalTilingdoes) for consistency across the API.
122-140: Prefetch frequency is too high - consider batch prefetching.The current implementation calls
Prefetchfor every single element, which adds significant overhead. Prefetch instructions are typically most effective when issued once per cache line (16 floats = 64 bytes), not per element. Additionally, copying one element at a time misses SIMD optimization opportunities.Consider prefetching once per cache line and using vectorized copies:
public static unsafe void CopyWithPrefetch(float* src, float* dst, int length) { - const int prefetchDistance = 64; // Prefetch 64 elements ahead + const int prefetchDistance = 64; // Prefetch 64 elements ahead + const int cacheLineElements = 16; // 64 bytes / 4 bytes per float int i = 0; - // Main loop with prefetching - for (; i < length - prefetchDistance; i++) + // Main loop with prefetching once per cache line + for (; i < length - prefetchDistance; i += cacheLineElements) { Prefetch(src + i + prefetchDistance); - dst[i] = src[i]; + for (int j = 0; j < cacheLineElements && i + j < length - prefetchDistance; j++) + { + dst[i + j] = src[i + j]; + } } // Remaining elements without prefetch for (; i < length; i++) { dst[i] = src[i]; } }
185-207: Cache miss estimation hardcodessizeof(float).The
elementsPerLinecalculation usessizeof(float), but the method parameters don't indicate the element type. If this utility is used withdoubleor other types, the estimation will be inaccurate.Consider adding an
elementSizeparameter for flexibility:-public static double EstimateCacheMisses(int dataSize, int accessStride, int cacheSize, int cacheLineSize) +public static double EstimateCacheMisses(int dataSize, int accessStride, int cacheSize, int cacheLineSize, int elementSize = 4) { // Simple cache miss estimation model - int elementsPerLine = cacheLineSize / sizeof(float); + int elementsPerLine = cacheLineSize / elementSize;src/Inference/InferenceOptimizer.cs (2)
277-287: Unusedmodelparameter inCreateNeuralDraftModel.Since this method now always throws
NotSupportedException, themodelparameter is never used. Consider removing it or suppressing the warning if kept for API consistency.-private IDraftModel<T>? CreateNeuralDraftModel(NeuralNetworkBase<T> model) +private IDraftModel<T>? CreateNeuralDraftModel(NeuralNetworkBase<T> _) {Or remove from switch expression:
-DraftModelType.SmallNeural => CreateNeuralDraftModel(model), +DraftModelType.SmallNeural => throw new NotSupportedException( + "DraftModelType.SmallNeural requires a pre-trained companion model..."),
212-223: Consider usingUnsafe.SizeOf<T>()for more flexible element sizing.The type checking approach works for common numeric types, but
System.Runtime.CompilerServices.Unsafe.SizeOf<T>()provides accurate sizing for any value type without maintaining a type list. This eliminates the need for a fallback value and handles future or custom numeric types automatically.Note:
Unsafe.SizeOf<T>()doesn't require a try/catch—it will return the CLR layout size for any unmanaged type. For reference types, it returns the pointer size.src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
79-95: Cache size estimation uses hardcoded values and ignores architecture parameter.The
archparameter is unused in all three estimation methods. Consider either removing it for clarity or implementing architecture-specific estimates (e.g., ARM chips often have different cache configurations than x86).-private static int EstimateL1CacheSize(Architecture arch) +private static int EstimateL1CacheSize(Architecture arch) => arch switch { - // Typical L1 cache size is 32KB per core - return 32 * 1024; -} + Architecture.Arm or Architecture.Arm64 => 64 * 1024, // Apple M-series, Cortex-A have larger L1 + _ => 32 * 1024 // Typical x86 +};
178-218: Consider makingPlatformCapabilitiesproperties immutable after construction.All properties have public setters, which allows external code to modify detected capabilities. Consider using init-only setters (
{ get; init; }) to prevent accidental modification after initialization.-public Architecture Architecture { get; set; } -public string OSDescription { get; set; } = string.Empty; +public Architecture Architecture { get; init; } +public string OSDescription { get; init; } = string.Empty; // ... and so on for other propertiesNote:
initsetters require C# 9+ / .NET 5+, so you may need conditional compilation for net471 compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
AiDotNetBenchmarkTests/AiDotNetBenchmarkTests.csproj(1 hunks)examples/JitCompiler/BasicUsageExample.cs(3 hunks)src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs(1 hunks)src/AiDotNet.Tensors/Engines/PlatformDetector.cs(1 hunks)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs(1 hunks)src/Inference/InferenceOptimizer.cs(5 hunks)src/InferenceOptimization/ARCHITECTURE.md(1 hunks)src/InferenceOptimization/CustomOperatorRegistry.cs(1 hunks)src/InferenceOptimization/Kernels/AttentionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/ConvolutionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/GemmKernel.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/InferenceOptimization/Kernels/AttentionKernel.cs
- src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/InferenceOptimization/Kernels/ConvolutionKernel.cs
🔇 Additional comments (18)
AiDotNetBenchmarkTests/AiDotNetBenchmarkTests.csproj (1)
9-9: ✅ Appropriate use of unsafe blocks in benchmark project.Enabling
AllowUnsafeBlocksis justified and necessary: the benchmark project will test SIMD kernels, GPU operations, and other performance-critical unsafe code introduced in this PR. The change aligns with the project's broader optimization objectives.examples/JitCompiler/BasicUsageExample.cs (1)
208-208: Good use of discard pattern for unused compiled delegates.The discard pattern (
_) appropriately communicates that the compiled functions are intentionally unused, as this example focuses solely on demonstrating cache behavior. This improves code clarity and aligns with the existing pattern inCustomOptionsExample.Also applies to: 222-222, 235-235
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
144-163: Good conditional compilation for accurate memory tracking.The use of
#if NET6_0_OR_GREATERto switch betweenGC.GetAllocatedBytesForCurrentThread()andGC.GetTotalMemory(false)is a good approach for cross-platform compatibility. TheMath.Max(0, ...)guard for memory delta correctly handles GC collection during measurement.src/InferenceOptimization/ARCHITECTURE.md (1)
1-14: Comprehensive architecture documentation.The documentation thoroughly covers design goals, component responsibilities, data flow, and extensibility patterns. This will be valuable for onboarding and maintenance.
src/InferenceOptimization/Kernels/GemmKernel.cs (2)
38-69: GEMM implementation looks correct with good optimization strategies.The implementation correctly:
- Validates input dimensions and shapes
- Uses cache-blocking for L1 cache efficiency
- Switches to parallel execution for large matrices
- Leverages SIMD through
SimdKernels.ScalarMultiplyAdd
153-184:GemmTransposeBis well-optimized for attention-style access patterns.Using
SimdKernels.DotProducton contiguous rows (since B is logically transposed) enables efficient SIMD utilization. This is a good pattern for Q*K^T computations in attention.src/InferenceOptimization/CustomOperatorRegistry.cs (1)
93-100:NullOperatorsentinel pattern is a clean design.Using a private sentinel type to represent "no operator found" avoids null checks in the caching layer while still returning
nullto callers. Good pattern.src/InferenceOptimization/Kernels/ConvolutionKernel.cs (2)
35-68: Good dispatcher implementation forExecute.The
Executemethod now properly dispatches to the appropriate convolution variant based on kernel shape, addressing the previous review concern aboutNotImplementedException. The config tensor for stride/padding is a flexible approach.
114-154: Conv2D implementation is correct with proper NCHW indexing.The index calculations correctly handle the NCHW tensor layout, and boundary checks for padding are properly implemented. The parallelization over
batch * outChannelsprovides good work distribution.src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (3)
62-91: Tiling computation logic is sound.The heuristic for computing optimal tile sizes based on L1 cache constraints is reasonable. The formula correctly accounts for three matrix tiles fitting in L1, and the power-of-2 rounding helps memory alignment.
96-117: Blocked transpose implementation is correct.The boundary handling with
Math.Minis correct, and the block size of 32 is reasonable for cache efficiency. For future optimization, the inner loop could benefit from SIMD vectorization.
145-180: Morton encoding implementation is correct.The bit interleaving and compaction logic follows the standard Morton encoding algorithm correctly. The aggressive inlining hints are appropriate for these small helper functions.
src/Inference/InferenceOptimizer.cs (4)
142-143: Memory-aware sequence length calculation is a good addition.Passing the model dimensions (
numLayers,numHeads,headDim) toEstimateMaxSequenceLengthenables accurate memory budget calculations for the KV cache.
176-207: Memory-aware sequence length estimation is well-implemented.The formula correctly accounts for KV cache memory requirements (keys and values across all layers, heads, and dimensions). The bounds checking and use of
MathHelper.Clampfor .NET Framework compatibility are appropriate.
230-241: Custom draft model validation is well-implemented.The early validation with a clear error message guiding users to call
SetCustomDraftModel()beforeInitialize()provides good UX for the API.
398-401: Custom draft model setter is well-documented and implemented.The method provides a clean API for injecting custom draft models with proper null validation and comprehensive documentation.
src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
222-243:GetBestSimdSet()implementation is correct.The method properly checks SIMD capabilities in descending order of capability (AVX-512 → AVX2 → AVX → SSE variants for x86, NEON with Dot Product → NEON for ARM) and returns an appropriate string descriptor.
56-64:HasArmDpdetection is incorrect — it should useDp.IsSupportedrather thanAdvSimd.Arm64.IsSupported.
AdvSimd.Arm64.IsSupporteddetects general ARM64 Advanced SIMD (NEON) support, not the ARMv8.2 dot product (SDOT/UDOT) instructions. TheSystem.Runtime.Intrinsics.Arm.Dpclass with itsIsSupportedproperty (available in .NET 8+) specifically detects dot product capability. Update the check to useDp.IsSupportedif the project targets .NET 8 or later.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
183-190: Consider makingOperationStatsproperties immutable.The public setters allow callers to mutate
OperationStatsobjects returned byGetStats(), which directly modifies dictionary entries. While unlikely to cause issues in practice, init-only setters would better encapsulate the profiler's internal state.If the project targets C# 9+, apply this diff:
public class OperationStats { - public string OperationName { get; set; } = string.Empty; - public long CallCount { get; set; } - public long TotalTicks { get; set; } - public long MinTicks { get; set; } - public long MaxTicks { get; set; } - public long TotalMemoryBytes { get; set; } + public string OperationName { get; init; } = string.Empty; + public long CallCount { get; init; } + public long TotalTicks { get; init; } + public long MinTicks { get; init; } + public long MaxTicks { get; init; } + public long TotalMemoryBytes { get; init; }src/InferenceOptimization/Kernels/ConvolutionKernel.cs (1)
118-158: Consider explicit SIMD vectorization for innermost loops.The PR objectives mention "SIMD vectorization kernels" with AVX2/AVX-512 and NEON implementations. While the current implementation relies on parallelization and auto-vectorization, explicit SIMD intrinsics for the innermost accumulation loops could further improve performance, particularly for the channel-wise operations.
Techniques like im2col transformation or vectorized accumulation could leverage the available SIMD capabilities detected by
PlatformDetector.Capabilities.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs(1 hunks)src/InferenceOptimization/CustomOperatorRegistry.cs(1 hunks)src/InferenceOptimization/Kernels/ConvolutionKernel.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/InferenceOptimization/CustomOperatorRegistry.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/InferenceOptimization/Kernels/ConvolutionKernel.cs
🧬 Code graph analysis (2)
src/InferenceOptimization/Kernels/ConvolutionKernel.cs (4)
src/InferenceOptimization/CustomOperatorRegistry.cs (4)
ICustomOperator(63-81)ICustomOperator(86-89)IsSupported(99-99)EstimatedSpeedup(100-100)src/InferenceOptimization/Kernels/GemmKernel.cs (4)
IsSupported(23-27)EstimatedSpeedup(29-36)Tensor(38-69)Tensor(153-184)src/InferenceOptimization/ICustomOperator.cs (3)
IsSupported(29-29)EstimatedSpeedup(35-35)Tensor(46-46)src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
PlatformDetector(14-172)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
src/Diagnostics/ProfilerScope.cs (1)
Profile(129-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
🔇 Additional comments (9)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
14-33: Excellent fixes addressing all previous review feedback.The implementation now correctly:
- Uses
GC.GetAllocatedBytesForCurrentThread()on .NET 6+ with fallback for older frameworks (lines 146, 158)- Returns new
OperationStatsobjects inAddOrUpdateto avoid mutation (lines 68-76)- Guards against division by zero in computed properties (lines 193, 197, 199)
- Removes the unused
_scopeStackfieldThe thread-safe singleton pattern, conditional memory tracking, and immutable update pattern are all well-implemented.
Also applies to: 49-78, 133-167, 183-200
src/InferenceOptimization/Kernels/ConvolutionKernel.cs (8)
13-15: LGTM!Metadata properties are correctly defined for the custom operator interface.
17-20: LGTM!Convolution is supported on all platforms with graceful performance degradation based on available SIMD capabilities.
22-28: LGTM!Platform-based speedup estimation aligns with the optimization goals and follows the established pattern from other kernels.
30-68: Well-implemented dispatcher with proper validation.The Execute method now properly routes to specific convolution methods based on kernel shape and optional configuration. The previous NotImplementedException concern has been fully addressed.
70-116: LGTM!Conv2D correctly validates output dimensions and uses safe parallelization. The validation at lines 97-100 properly addresses the previous review comment.
118-158: LGTM!The unsafe convolution implementation correctly handles padding boundaries and uses proper index calculations for NCHW layout.
201-237: LGTM!The depthwise convolution helper correctly implements per-channel processing with proper bounds checking. The kernel indexing is correct for the expected
[channels, 1, kernelH, kernelW]layout.
292-336: LGTM!The group convolution helper correctly implements group-wise processing with proper index calculations for both input (global channels) and kernel (per-group channels).
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.
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/Inference/PagedAttention/PagedAttentionKernel.cs (1)
105-109: Causal mask condition is always false (dead code).The loop iterates
posfrom0toseqLen - 1, sopos > seqLen - 1can never be true. This means the causal mask is never applied inComputeAttention.For single-token queries (common in incremental decoding), causal masking typically masks future positions relative to the query position. If the intent is to mask positions beyond the current query position during multi-token scenarios, the condition should compare against the query's position index, not
seqLen - 1.- // Apply causal mask - if (causalMask && pos > seqLen - 1) - { - score = float.NegativeInfinity; - } + // For single-token incremental decoding, all cached positions are valid. + // Causal masking is implicitly satisfied since we only have past tokens. + // If batch queries are supported, pass queryPosition and use: + // if (causalMask && pos > queryPosition) score = float.NegativeInfinity;src/NeuralNetworks/Attention/FlashAttention.cs (3)
514-534: Backward pass does not supportqueryOffsetparameter.The
Forwardmethod accepts aqueryOffsetfor KV-cached decoding, butBackwarddoes not propagate this parameter. The backward pass hardcodes causal masking asj > i(lines 618, 790), which will produce incorrect gradients whenqueryOffset != 0.If backward is intended to be used with cached decoding scenarios, it should accept and use
queryOffsetconsistently with the forward pass.public static (Tensor<T> GradQuery, Tensor<T> GradKey, Tensor<T> GradValue) Backward( Tensor<T> gradOutput, Tensor<T> query, Tensor<T> key, Tensor<T> value, Tensor<T> output, - FlashAttentionConfig? config = null) + FlashAttentionConfig? config = null, + int queryOffset = 0)
616-622: Backward causal mask check ignores potentialqueryOffset.This check
j > ishould bej > queryOffset + ito match the forward pass logic whenqueryOffsetis non-zero. Otherwise, gradients will be computed incorrectly for cached decoding scenarios.
788-794: Same issue inBackwardCore4D: causal mask check should incorporatequeryOffset.Line 790 uses
j > ibut should usej > queryOffset + ifor consistency with the forward pass.
♻️ Duplicate comments (7)
AiDotNetBenchmarkTests/InferenceOptimization/README.md (1)
86-98: Add language identifiers to fenced code blocks.The code blocks at lines 86 and 91 lack language identifiers, which affects syntax highlighting and rendering.
Apply this diff:
-``` +```text Speedup = Baseline Time / Optimized TimeExample output:
-+text
| Method | MatrixSize | Mean | Error | StdDev | Ratio |</blockquote></details> <details> <summary>src/Inference/SpeculativeDecoding/TreeStepStatistics.cs (1)</summary><blockquote> `6-6`: **Breaking change: public API reduced to internal.** Same as `StepStatistics`, this visibility change breaks external consumers. Ensure this internalization aligns with your API versioning policy. </blockquote></details> <details> <summary>src/Inference/SpeculativeDecoding/TreeSpeculativeConfig.cs (1)</summary><blockquote> `6-6`: **Breaking change: public API reduced to internal.** </blockquote></details> <details> <summary>src/Inference/SpeculativeDecoding/NeuralDraftModel.cs (1)</summary><blockquote> `16-16`: **Breaking change: public API reduced to internal.** </blockquote></details> <details> <summary>src/Inference/SpeculativeDecoding/SpeculativeResult.cs (1)</summary><blockquote> `8-8`: **Breaking change: public API reduced to internal.** </blockquote></details> <details> <summary>src/Inference/PagedAttention/BlockTable.cs (1)</summary><blockquote> `24-24`: **Breaking change: public API reduced to internal.** Both `BlockTable` and `BlockTableManager<T>` are now internal, which will break external consumers that depend on paged attention infrastructure. Also applies to: 239-239 </blockquote></details> <details> <summary>src/Inference/SpeculativeDecoding/DraftResult.cs (1)</summary><blockquote> `9-9`: **Breaking change: public API reduced to internal.** </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (34)</summary><blockquote> <details> <summary>docs/INFERENCE_MVP_PHASES.md (2)</summary><blockquote> `3-3`: **Minor style refinement (optional).** "Small number of carefully chosen inference entrypoints" is acceptable but could be slightly more specific per LanguageTool feedback: consider "a few carefully chosen inference entrypoints" or "specific entry points (e.g., session start)." --- `20-100`: **Phases 0–4 are well-defined; consider documenting inter-phase dependencies.** Each phase has clear outcomes and acceptance criteria. However, there are implicit dependencies between phases (e.g., Phase 0 diagnostics inform Phase 1 attention rewrites, Phase 2 paging depends on Phase 1 attention rewrites). Consider adding a brief "Dependencies" section per phase or a dependency matrix to help implementers understand sequencing risks. </blockquote></details> <details> <summary>docs/PR433_FACADE_INFERENCE_PLAN.md (3)</summary><blockquote> `24-30`: **Facade constraints are clearly stated; consider adding a conformance checkpoint.** The constraints are well-articulated (Sections 1.1–1.2). To reduce risk of scope creep or accidental public API expansion during implementation, consider adding a note that each phase should include a conformance audit: verify no new public types/methods were added beyond the stated surface (builder, result, session). I can help draft a simple conformance checklist if needed. --- `29-29`: **Minor style refinements (optional).** Static analysis flags two style opportunities (both optional and low-impact): - Line ~29: "not compatible with caching" could be "incompatible" (slightly less wordy). - Line ~172: Three successive "If" sentences — consider rewording one for variety (e.g., "When uncertain, default to causal..."). These are cosmetic and do not affect clarity. Also applies to: 172-172 --- `450-512`: **Testing plan and acceptance criteria are thorough; ensure integration tests cover facade only.** Section 5 (Testing Plan) and Section 6 (Acceptance Criteria Checklist) are comprehensive. A strength: Section 5.2 emphasizes that integration tests should "only use the public facade" — this is crucial for validating the facade design. One note: Section 5.2 references testing `AttentionLayer<T>`, `SelfAttentionLayer<T>`, and `GraphAttentionLayer<T>` coverage, but Section 2.3 notes these layer types are not yet consistently recognized by the optimizer. Ensure that acceptance criteria include "All attention layer types either optimize safely *or* are explicitly skipped with diagnostics" (not silently ignored). </blockquote></details> <details> <summary>src/Normalizers/NoNormalizer.cs (1)</summary><blockquote> `125-130`: **LGTM! Feature dimension handling is correct.** The logic correctly derives the feature count from the last tensor dimension, which aligns with standard ML conventions (e.g., `[batch, sequence, features]`). The safe handling of scalar tensors (`Shape.Length == 0`) is also appropriate. **Optional: Consider documenting the dimension assumption.** For improved clarity, you could add a remark in the XML documentation explaining that Tensor inputs treat the last dimension as the feature dimension for parameter bookkeeping. </blockquote></details> <details> <summary>tests/AiDotNet.Serving.Tests/ServingIntegrationTests.cs (1)</summary><blockquote> `245-257`: **Consider extracting magic number to improve test clarity.** The hardcoded `100.0` offset distinguishes the variant's behavior from the base model. For improved readability, consider extracting it as a named constant at the test method level: ```diff + const double VariantOffset = 100.0; + var numOps = MathHelper.GetNumericOperations<double>(); var variant = new ServableModelWrapper<double>( modelName: variantName, inputDimension: 3, outputDimension: 1, predictFunc: input => { var sum = numOps.Zero; for (int i = 0; i < input.Length; i++) { sum = numOps.Add(sum, input[i]); } - return new Vector<double>(new[] { sum + 100.0 }); + return new Vector<double>(new[] { sum + VariantOffset }); });Then update the assertion to reference the same constant:
- Assert.Equal(106.0, result.Predictions[0][0], 5); + Assert.Equal(6.0 + VariantOffset, result.Predictions[0][0], 5); // 6.0 = sum of 1+2+3src/NeuralNetworks/Transformer.cs (1)
654-660: Architecture values are read but never used.These values are deserialized from the stream but stored in unused local variables. Since
_transformerArchitectureis readonly and cannot be reassigned here, these reads serve no purpose beyond consuming data from the stream.Consider either:
- Removing these reads if the architecture is already properly initialized elsewhere
- Using these values to validate against the current
_transformerArchitecturestate- Documenting why these values must be read but not used (e.g., for backward compatibility with older serialization formats)
- // Read Transformer-specific architecture details - int numHeads = reader.ReadInt32(); - int numEncoderLayers = reader.ReadInt32(); - int numDecoderLayers = reader.ReadInt32(); - int maxSequenceLength = reader.ReadInt32(); - int vocabularySize = reader.ReadInt32(); - T dropoutRate = NumOps.FromDouble(reader.ReadDouble()); + // Read Transformer-specific architecture details (for serialization format compatibility) + reader.ReadInt32(); // numHeads + reader.ReadInt32(); // numEncoderLayers + reader.ReadInt32(); // numDecoderLayers + reader.ReadInt32(); // maxSequenceLength + reader.ReadInt32(); // vocabularySize + reader.ReadDouble(); // dropoutRatetests/AiDotNet.Tests/UnitTests/Serving/ContinuousBatchingTests.cs (1)
492-698: Well-structured speculative decoding tests with good policy coverage.The test suite covers the main speculative decoding policies (ForceOn, ForceOff, Auto) and failure handling scenarios. The mock draft models (DeterministicDraftModel and ThrowingDraftModel) are appropriately designed for their test purposes.
Consider adding test coverage for:
- Mixed batch scenarios (some sequences use speculation, others don't)
- Edge case where draft model returns fewer tokens than requested
- Verification of acceptance rate calculations after multiple steps
src/NeuralNetworks/NeuralNetworkBase.cs (1)
1311-1350: Well-designed serialization identifier with metadata support.The implementation correctly:
- Uses stable ordering (OrderBy with Ordinal) for deterministic serialization
- Falls back gracefully when metadata is absent
- Handles both ILayerSerializationMetadata and LayerBase activation types
- Uses safe null-coalescing for AssemblyQualifiedName
Consider using StringBuilder for the string concatenation in lines 1344-1347 if metadata dictionaries are expected to be large (>5-10 entries). For typical layer metadata (2-5 entries), the current approach is fine.
if (metadata.Count == 0) { return typeName; } +var sb = new StringBuilder(typeName); // Stable ordering for deterministic serialization. foreach (var kvp in metadata.OrderBy(k => k.Key, StringComparer.Ordinal)) { - typeName += $";{kvp.Key}={kvp.Value}"; + sb.Append($";{kvp.Key}={kvp.Value}"); } -return typeName; +return sb.ToString();src/Helpers/InferenceDiagnostics.cs (1)
63-69: Consider trim performance impact in high-frequency scenarios.The
TrimIfNeeded()method uses awhileloop to dequeue entries until the count is belowMaxEntries. In high-frequency diagnostic scenarios, this could introduce some overhead sinceCountonConcurrentQueuecan be expensive (O(n) in some implementations). However, given that:
- Diagnostics are opt-in via environment variable
- The queue is bounded to 1024 entries
- This is best-effort trimming
The implementation is reasonable for its intended diagnostic use case.
If you observe performance issues, consider adding a counter to avoid calling
Countrepeatedly:+private static int _approximateCount = 0; + internal static void RecordDecision(string area, string feature, bool enabled, string reason) { // ... Entries.Enqueue(/* ... */); + Interlocked.Increment(ref _approximateCount); TrimIfNeeded(); } private static void TrimIfNeeded() { - while (Entries.Count > MaxEntries && Entries.TryDequeue(out _)) + while (_approximateCount > MaxEntries && Entries.TryDequeue(out _)) { + Interlocked.Decrement(ref _approximateCount); } }src/NeuralNetworks/Layers/EmbeddingLayer.cs (1)
759-766: Consider defensive null check for tensor access.The serialization metadata implementation correctly exposes vocabulary size and embedding dimension. However, accessing
_embeddingTensor.Shapewithout a null check could throw if called before initialization.Consider adding a defensive check:
Dictionary<string, string> ILayerSerializationMetadata.GetSerializationMetadata() { + if (_embeddingTensor == null) + throw new InvalidOperationException("Cannot retrieve metadata before layer initialization."); + return new Dictionary<string, string> { ["VocabularySize"] = _embeddingTensor.Shape[0].ToString(System.Globalization.CultureInfo.InvariantCulture), ["EmbeddingDimension"] = _embeddingTensor.Shape[1].ToString(System.Globalization.CultureInfo.InvariantCulture) }; }src/AiDotNet.Serving/Controllers/InferenceController.cs (1)
202-206: Consider standardizing adapter header naming.The code checks for both
X-AiDotNet-LoraandX-AiDotNet-Adapterheaders. Having two names for the same purpose could cause confusion.Consider:
- Documenting which header is preferred (or if both are supported for backward compatibility)
- Logging which header was used for debugging
- Returning an error if both headers are present with different values
if (!Request.Headers.TryGetValue("X-AiDotNet-Lora", out var adapterValues) && !Request.Headers.TryGetValue("X-AiDotNet-Adapter", out adapterValues)) { return modelName; } +// Optional: check for conflicting headers +if (Request.Headers.TryGetValue("X-AiDotNet-Lora", out var loraValue) && + Request.Headers.TryGetValue("X-AiDotNet-Adapter", out var adapterValue) && + loraValue.ToString() != adapterValue.ToString()) +{ + _logger.LogWarning("Conflicting adapter headers: X-AiDotNet-Lora={Lora}, X-AiDotNet-Adapter={Adapter}", loraValue, adapterValue); +}tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (2)
64-78: Consider defensive casting for statistics dictionary values.The direct cast
(int[])statsAfterFirst["KVCache_SequenceLengths"]will throw if the key is missing or the type differs. Consider using a safer pattern for test robustness.- var lengthsAfterFirst = (int[])statsAfterFirst["KVCache_SequenceLengths"]; + Assert.True(statsAfterFirst.TryGetValue("KVCache_SequenceLengths", out var lengthsObj), + "Expected KVCache_SequenceLengths in inference statistics"); + var lengthsAfterFirst = Assert.IsType<int[]>(lengthsObj);
221-236: Unused helper method.
AssertTensorsNotEqualis defined but not used in the current tests. Consider removing it or adding a test that uses it to validate its implementation.src/Helpers/DeserializationHelper.cs (1)
612-621: Broad exception swallowing may hide real errors.Catching all exceptions and returning
nullcan mask issues likeTypeLoadException,SecurityException, orMissingMethodExceptionthat indicate configuration problems rather than expected "no default constructor" cases.try { return (TInterface?)Activator.CreateInstance(type); } - catch + catch (MissingMethodException) { // Some implementations (e.g., optimizers) require constructor arguments. // Treat them as optional on deserialization and let callers provide sensible defaults. return null; }src/Serving/ContinuousBatching/ContinuousBatcher.cs (2)
608-622: Vocab size fallback may cause silent dimension mismatches.If the probe fails, falling back to
50000without logging could lead to hard-to-debug errors later if the actual vocab size differs. Consider recording this fallback via diagnostics.catch { // Fallback to a common default; speculative decoding will be disabled if the shapes don't line up. + InferenceDiagnostics.RecordDecision( + "Serving.ContinuousBatching", + "SpeculativeDecoding", + enabled: true, + reason: "VocabSizeProbeFailedUsingDefault(50000)"); return 50000; }
690-692: Inefficient Random allocation per sample call.Creating a new
Random()instance for each token sample is inefficient and may produce correlated sequences when called in rapid succession (same seed from time-based default). Consider using a field-level or thread-localRandom.+ [ThreadStatic] + private static Random? _samplingRandom; + private int SampleFromLogits(Tensor<T> logits, GenerationRequest<T> request) { // ... existing code ... // Sample from distribution - var random = new Random(); + _samplingRandom ??= new Random(); + var random = _samplingRandom; float r = (float)random.NextDouble();src/Inference/KVCache.cs (2)
535-568: Memory usage estimation can be wrong when storage backend differs fromConfig.DataType
GetCurrentMemoryUsagederivesbytesPerElementpurely fromConfig.DataType. In configurations whereConfig.DataType == CacheDataType.Int8but_useInt8Storageisfalse(e.g., unsupportedT), the cache actually storesTensor<T>in full precision while you still report 1 byte per element. Similar mismatches can happen if the precision/quantization config and storage backend drift.To avoid under/over-reporting memory, consider basing
bytesPerElementon the active storage backend instead (int8 vs Half vsT), e.g. by:
- Using 1 / 2 bytes when
_useInt8Storage/_useFp16Storageis true, and- Falling back to
Unsafe.SizeOf<T>()(or a small switch ontypeof(T)) for the full-precision path.This keeps statistics consistent with actual allocations.
391-440:Updatecan write into unallocated caches in non‑int8 modes
Updateonly callsEnsureCacheAllocated(layerIndex)inside the_useInt8Storagebranch. In FP16/full‑precision modes, if the layer hasn’t been preallocated (PreAllocate=false) andUpdateis called before anyAppend,_keyCache[layerIndex]/_keyCacheFp16[layerIndex]remain null and the first write will throw aNullReferenceException.
Appendalready defends itself withEnsureCacheAllocated(layerIndex);before writes. For consistency and clearer failure modes, either:
- Always call
EnsureCacheAllocated(layerIndex)at the start ofUpdate(regardless of storage mode), or- At least guard with
IsLayerAllocated(layerIndex)and throw a descriptiveInvalidOperationExceptioninstead of relying on a null dereference.This makes speculative-decoding style updates safer in non‑int8 configurations.
Also applies to: 776-806
src/Inference/CachedMultiHeadAttention.cs (1)
266-313: Activation application and derivative wiring look self‑consistentThe forward paths now:
- Apply the configured activation function after the output projection (
_lastOutput = ApplyActivation(output)), and- Return the activated tensor.
The backward path correspondingly:
- Uses
ApplyActivationDerivative(_lastOutput, outputGradient)to obtainactivationGradient, and- Derives
_outputBiasGradientfromactivationGradient.Sum([0, 1]).Given the existing comment that backward is “simplified” and not a full manual gradient for all parameters, this change at least keeps the activation handling consistent between forward and backward for the output bias. No further adjustment seems necessary unless you plan to extend the attention-layer gradients beyond this simplified stub.
Also applies to: 405-427
src/Models/Results/PredictionModelResult.cs (2)
1027-1066: Double‑checked locking on optimization init could be tightened but is acceptableBoth
EnsureStatelessInferenceOptimizationsInitializedandEnsureSequenceOptimizationsInitializeduse a double‑checked locking pattern:
- Fast path: check
_inferenceOptimizationsInitialized/_sequenceInitializedwithout a lock.- Slow path: lock, perform optimization, then set the flag in
finally.In practice this works because:
- The optimizer/model references are only ever written before setting the flag, and
- The worst case is reading a stale
nulland falling back to the non‑optimized path for a call or two.If you want stricter thread-safety, you could:
- Mark the boolean flags as
volatile, or- Drop the outer fast-path check and always read them under the lock (small overhead, simpler semantics).
Given the current behaviour only affects whether some calls see the optimized model or fall back transiently, this is more of a robustness/clarity improvement than a correctness bug.
Also applies to: 1240-1301
1007-1134: InferenceSession / InferenceSequence design cleanly encapsulates stateful inferenceThe new session/sequence façade:
- Keeps stateful internals (KV-cache, optimized models) behind
BeginInferenceSession() → InferenceSession → InferenceSequence.- Uses per-sequence
InferenceOptimizer<T>/optimized models, with lazy initialization protected by_sequenceLock.- Ensures
Reset()andDispose()clear sequence-local cache via_sequenceOptimizer?.ClearCache()while making disposal non-throwing.- In
InferenceSequence.Predict, always normalizes/denormalizes like the top-levelPredict, and falls back to the unoptimizedModel.Predictif no optimizations apply or if types don’t match.The behaviour differences between stateless
Predict()(no stateful features) and session-basedInferenceSequence.Predict()(allowed to use KV-cache, etc.) are clearly separated and safe. This structure should integrate well with serving-style workloads.Also applies to: 1136-1310
src/InferenceOptimization/CustomOperatorRegistry.cs (4)
46-62: Potential race inRegisterwhen locking on the update target.The
AddOrUpdatefactory locks onexistingList, butAddOrUpdatemay call the update factory multiple times if there are concurrent updates, and each call receives the current value at that moment. If two threads race, one might lock on a list that's already been replaced. While the current code creates a new list (avoiding mutation of the locked list), the lock itself is on a transient object that may not provide the intended mutual exclusion.Consider using a dedicated lock object per operator name (e.g., stored in a separate
ConcurrentDictionary<string, object>) to ensure consistent serialization of updates for the same name.+ private readonly ConcurrentDictionary<string, object> _operatorLocks = new(); + public void Register(ICustomOperator op) { if (op == null) throw new ArgumentNullException(nameof(op)); void BumpVersion() => _operatorVersions.AddOrUpdate(op.Name, 1, (_, v) => v + 1); + var lockObj = _operatorLocks.GetOrAdd(op.Name, _ => new object()); + lock (lockObj) + { - _operators.AddOrUpdate( - op.Name, - _ => new List<ICustomOperator> { op }, - (_, existingList) => - { - List<ICustomOperator> newList; - lock (existingList) - { - newList = new List<ICustomOperator>(existingList) { op }; - } - newList.Sort((a, b) => b.Priority.CompareTo(a.Priority)); - return newList; - }); + _operators.AddOrUpdate( + op.Name, + _ => new List<ICustomOperator> { op }, + (_, existingList) => + { + var newList = new List<ICustomOperator>(existingList) { op }; + newList.Sort((a, b) => b.Priority.CompareTo(a.Priority)); + return newList; + }); - BumpVersion(); + BumpVersion(); + } }
95-106: Potential stale list reference inSelectOperatorOrNull.The
candidateslist retrieved from_operatorscould be replaced by a concurrentRegistercall before thelock(candidates)is acquired. Locking on the old list won't synchronize with operations on the new list.If you adopt the per-name lock approach suggested for
Register, use the same lock object here to ensure consistent synchronization.
183-188:Clear()is non-atomic and may leave transient inconsistent state.Concurrent callers of
GetOperatororRegisterduringClear()could observe partial state (e.g., operators exist but versions are cleared). For a singleton registry, this is unlikely to cause issues in typical usage, but document the expectation or use a global lock if atomicity is required.
119-126:NullOperatorallocations per cache miss.Each call to
SelectOperatorOrNullthat finds no operator creates a newNullOperatorinstance. Consider caching a static singletonNullOperatorto reduce allocations.+ private static readonly NullOperator NullOperatorSentinel = new(); + private ICustomOperator SelectOperatorOrNull(string name) { if (!_operators.TryGetValue(name, out var candidates)) - return new NullOperator(); + return NullOperatorSentinel; lock (candidates) { var result = candidates.FirstOrDefault(op => op.IsSupported()); - return result ?? new NullOperator(); + return result ?? NullOperatorSentinel; } }tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (1)
86-89: In-place mutation assertion couples test to implementation detail.Asserting that the original
modelwas mutated whencloneModel: falseis fragile. If the implementation changes to always clone internally, this test will fail even though the behavior is still correct. Consider focusing on the output model's state rather than the input model's mutation.src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
79-95: Cache size estimates are hardcoded and ignore thearchparameter.The
EstimateL1CacheSize,EstimateL2CacheSize, andEstimateL3CacheSizemethods accept anArchitectureparameter but return the same values regardless. Consider removing the unused parameter or implementing architecture-specific estimates (e.g., ARM typically has smaller L3 or no L3).- private static int EstimateL1CacheSize(Architecture arch) + private static int EstimateL1CacheSize(Architecture _) { // Typical L1 cache size is 32KB per core return 32 * 1024; }
154-159: OpenCL detection is a stub.
DetectOpenCLSupportalways returnsfalse. If this is intentional for MVP, consider adding a TODO comment or documenting this limitation.private static bool DetectOpenCLSupport() { - // This would require OpenCL library calls - // For now, we'll return false (requires additional implementation) + // TODO: Implement actual OpenCL detection by loading OpenCL.dll/libOpenCL.so + // For now, return false until OpenCL support is added. return false; }src/Inference/InferenceOptimizer.cs (3)
117-118: Use consistent diagnostics instead ofConsole.WriteLine.Line 117 uses
Console.WriteLinefor the clone failure warning, but subsequent code usesInferenceDiagnostics.RecordException. Use the diagnostics API consistently for all logging.- Console.WriteLine($"Warning: model cloning failed for inference optimizations: {ex.Message}. Skipping inference optimizations for this model instance."); InferenceDiagnostics.RecordException( area: "InferenceOptimizer", feature: "CloneForRewrite", ex: ex, - reason: "Clone failed; skipping all inference optimizations to avoid mutating user model."); + reason: $"Clone failed ({ex.Message}); skipping all inference optimizations to avoid mutating user model.");
370-375: Loop index manipulation after layer replacement is error-prone.Decrementing
iafter replacing aSelfAttentionLayerto re-process it asMultiHeadAttentionLayerworks but is subtle and could lead to infinite loops if the conversion keeps producingSelfAttentionLayer. Consider using a separate worklist or recursion limit.
344-489:ApplyAttentionOptimizationsis a large method (~145 lines).Consider extracting helper methods for each layer type conversion (e.g.,
RewriteMultiHeadAttention,RewriteFlashAttention,RewriteSelfAttention) to improve readability and testability.src/Inference/PagedCachedMultiHeadAttention.cs (1)
262-308: Consider optimizing SplitHeads and MergeHeads with bulk operations.Both
SplitHeadsandMergeHeadsuse nested loops for tensor reshaping. While these are data movement operations (not compute-intensive), they can still benefit from optimization:
- Use
Span<T>.CopyToorBuffer.BlockCopyfor contiguous chunks- Leverage tensor reshape/view operations if available
- Improve cache locality with better access patterns
Example optimization for
SplitHeads:private Tensor<T> SplitHeads(Tensor<T> x) { int batchSize = x.Shape[0]; int seqLen = x.Shape[1]; var reshaped = new Tensor<T>([batchSize, _headCount, seqLen, _headDimension]); + // If Tensor<T> supports reshape/view without copying, use that: + // return x.Reshape([batchSize, seqLen, _headCount, _headDimension]) + // .Transpose([0, 2, 1, 3]); + + // Otherwise, optimize data movement with contiguous copies: for (int b = 0; b < batchSize; b++) { - for (int s = 0; s < seqLen; s++) + for (int h = 0; h < _headCount; h++) { - for (int h = 0; h < _headCount; h++) + int baseOffset = h * _headDimension; + for (int s = 0; s < seqLen; s++) { - int baseOffset = h * _headDimension; - for (int d = 0; d < _headDimension; d++) - { - reshaped[b, h, s, d] = x[b, s, baseOffset + d]; - } + // Copy contiguous chunk if possible + // e.g., x.GetSlice(...).CopyTo(reshaped.GetSlice(...)) } } } return reshaped; }Apply similar optimizations to
MergeHeadsat lines 286-308.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (62)
AiDotNetBenchmarkTests/InferenceOptimization/README.md(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs(1 hunks)docs/INFERENCE_MVP_PHASES.md(1 hunks)docs/PR433_FACADE_INFERENCE_PLAN.md(1 hunks)docs/PR433_REVIEW_WORKFLOW.md(1 hunks)src/AiDotNet.Serving/Controllers/InferenceController.cs(1 hunks)src/AiDotNet.Serving/Models/IServableModelInferenceOptions.cs(1 hunks)src/AiDotNet.Serving/Models/ServableModelWrapper.cs(4 hunks)src/AiDotNet.Serving/Services/ModelStartupService.cs(2 hunks)src/AiDotNet.Tensors/Engines/PlatformDetector.cs(1 hunks)src/Configuration/InferenceOptimizationConfig.cs(5 hunks)src/Helpers/DeserializationHelper.cs(5 hunks)src/Helpers/InferenceDiagnostics.cs(1 hunks)src/Inference/CachedMultiHeadAttention.cs(10 hunks)src/Inference/InferenceOptimizer.cs(11 hunks)src/Inference/KVCache.cs(21 hunks)src/Inference/KVCacheConfig.cs(4 hunks)src/Inference/PagedAttention/BlockManager.cs(3 hunks)src/Inference/PagedAttention/BlockTable.cs(2 hunks)src/Inference/PagedAttention/PagedAttentionKernel.cs(5 hunks)src/Inference/PagedAttention/PagedKVCache.cs(4 hunks)src/Inference/PagedCachedMultiHeadAttention.cs(1 hunks)src/Inference/SpeculativeDecoding/DraftResult.cs(1 hunks)src/Inference/SpeculativeDecoding/IDraftModel.cs(1 hunks)src/Inference/SpeculativeDecoding/NGramDraftModel.cs(1 hunks)src/Inference/SpeculativeDecoding/NeuralDraftModel.cs(1 hunks)src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs(2 hunks)src/Inference/SpeculativeDecoding/SpeculativeDecodingConfig.cs(1 hunks)src/Inference/SpeculativeDecoding/SpeculativeDecodingStats.cs(1 hunks)src/Inference/SpeculativeDecoding/SpeculativeResult.cs(1 hunks)src/Inference/SpeculativeDecoding/StepStatistics.cs(1 hunks)src/Inference/SpeculativeDecoding/TreeSpeculativeConfig.cs(1 hunks)src/Inference/SpeculativeDecoding/TreeSpeculativeDecoder.cs(1 hunks)src/Inference/SpeculativeDecoding/TreeSpeculativeResult.cs(1 hunks)src/Inference/SpeculativeDecoding/TreeStepStatistics.cs(1 hunks)src/InferenceOptimization/CustomOperatorRegistry.cs(1 hunks)src/InferenceOptimization/Kernels/AttentionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/ConvolutionKernel.cs(1 hunks)src/Models/Results/PredictionModelResult.cs(4 hunks)src/NeuralNetworks/Attention/FlashAttention.cs(13 hunks)src/NeuralNetworks/Attention/FlashAttentionLayer.cs(2 hunks)src/NeuralNetworks/Layers/DropoutLayer.cs(2 hunks)src/NeuralNetworks/Layers/EmbeddingLayer.cs(2 hunks)src/NeuralNetworks/Layers/GraphAttentionLayer.cs(2 hunks)src/NeuralNetworks/Layers/ILayerSerializationMetadata.cs(1 hunks)src/NeuralNetworks/Layers/LayerBase.cs(2 hunks)src/NeuralNetworks/Layers/LayerNormalizationLayer.cs(2 hunks)src/NeuralNetworks/Layers/MultiHeadAttentionLayer.cs(2 hunks)src/NeuralNetworks/Layers/PositionalEncodingLayer.cs(2 hunks)src/NeuralNetworks/Layers/SelfAttentionLayer.cs(2 hunks)src/NeuralNetworks/NeuralNetworkBase.cs(3 hunks)src/NeuralNetworks/Transformer.cs(2 hunks)src/Normalizers/NoNormalizer.cs(2 hunks)src/Serving/ContinuousBatching/BatchScheduler.cs(1 hunks)src/Serving/ContinuousBatching/ContinuousBatcher.cs(7 hunks)src/Serving/ContinuousBatching/ContinuousBatcherConfig.cs(1 hunks)tests/AiDotNet.Serving.Tests/ServingIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Attention/FlashAttentionTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/KVCacheTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Serving/ContinuousBatchingTests.cs(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/InferenceOptimization/Kernels/ConvolutionKernel.cs
🪛 LanguageTool
docs/INFERENCE_MVP_PHASES.md
[style] ~7-~7: Specify a number, remove phrase, use “a few”, or use “some”
Context: ...redictionModelResult` (inference), with a small number of carefully chosen inference entrypoints ...
(SMALL_NUMBER_OF)
docs/PR433_FACADE_INFERENCE_PLAN.md
[style] ~29-~29: Consider using “incompatible” to avoid wordiness.
Context: ...ntly change results for models that are not compatible with caching. ### 1.3 Non-goals (for t...
(NOT_ABLE_PREMIUM)
[style] ~172-~172: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...ssification), prefer non-causal. - If uncertain, default to causal only insid...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
AiDotNetBenchmarkTests/InferenceOptimization/README.md
[style] ~138-~138: To elevate your writing, try using an alternative expression here.
Context: ... matrix operations - Memory channels matter for parallel operations 4. **Thermal T...
(MATTERS_RELEVANT)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
- GitHub Check: Codacy Security Scan
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Inference/KVCache.cs (2)
808-860: Sliding-window eviction can produce negative sequence lengths and out-of-bounds writes whennewSeqLen > WindowSize.In
HandleSlidingWindowEviction(around Line 808),evictCountis computed asnewLen - _config.WindowSizeandkeepCountascurrentLen - evictCount. WhennewSeqLenexceedsWindowSize(e.g.,currentLen = 0, WindowSize = 4, newSeqLen = 10),keepCountbecomes negative and is written into_sequenceLengths[layerIndex][b]. The subsequentAppendthen uses this negativecurrentLento computetargetPos = currentLen + s, leading directly to negative indices and undefined behaviour.You should clamp eviction to never exceed
currentLenand explicitly rejectnewSeqLen > WindowSize(or handle it by dropping all history), ensuring_sequenceLengthsnever goes negative andtargetPosstays in[0, MaxSequenceLength).Example fix:
for (int b = 0; b < batchSize; b++) { - int currentLen = _sequenceLengths[layerIndex][b]; - int newLen = currentLen + newSeqLen; - - if (newLen > _config.WindowSize) - { - int evictCount = newLen - _config.WindowSize; + int currentLen = _sequenceLengths[layerIndex][b]; + int newLen = currentLen + newSeqLen; + + if (newLen > _config.WindowSize) + { + if (newSeqLen > _config.WindowSize) + { + throw new InvalidOperationException( + $"Sliding window size {_config.WindowSize} is smaller than incoming segment length {newSeqLen}."); + } + + int evictCount = newLen - _config.WindowSize; + if (evictCount > currentLen) + { + evictCount = currentLen; + } // Shift cache entries - int keepCount = currentLen - evictCount; + int keepCount = currentLen - evictCount; if (keepCount > 0) { ... } - _sequenceLengths[layerIndex][b] = keepCount; + _sequenceLengths[layerIndex][b] = keepCount; _evictions += evictCount; } }This guarantees
keepCount >= 0, prevents negative sequence lengths, and avoids negative target indices inAppend.
391-440:Updatelacks shape validation and allocation in non‑int8 mode, risking index and null-reference errors.
Update(around Line 391) assumes:
keys/valuesare 4D tensors with[batch, heads, numPositions, headDim], and- The layer’s cache has already been allocated when
_useInt8Storageis false.But there is:
- No call to
ValidateInputShapes(keys, values), so a mismatchedkeys.Shape[2] < positions.Lengthwill causeTensorindex errors atkeys[new[] { b, h, p, d }].- No
EnsureCacheAllocated(layerIndex)in the non‑int8 path, so callingUpdatebefore anyAppendcan dereference_keyCache[layerIndex]/_valueCache[layerIndex]while they are still null.You can harden this by validating shapes and always ensuring allocation before writing:
public void Update(int layerIndex, int[] positions, Tensor<T> keys, Tensor<T> values) { ValidateLayerIndex(layerIndex); - - int batchSize = keys.Shape[0]; - int numPositions = positions.Length; + ValidateInputShapes(keys, values); + + int batchSize = keys.Shape[0]; + int numPositions = positions.Length; + + // Ensure backing storage exists for this layer for all backends + EnsureCacheAllocated(layerIndex); - if (_useInt8Storage) + if (_useInt8Storage) { - EnsureCacheAllocated(layerIndex); EnsureInt8Scales(layerIndex, keys, values, batchSize, numPositions); }This keeps behaviour for valid callers while failing fast with clear errors for malformed shapes or invalid call ordering.
🧹 Nitpick comments (6)
docs/PR433_FACADE_INFERENCE_PLAN.md (2)
549-576: Define concrete throughput regression threshold for MVP-1 speculation policy.Section 9.2 states: "Serving throughput under batching load does not regress materially when
EnableSpeculativeDecoding=true(policy must disable speculation under heavy load).""Materially" is undefined. Is < 5% regression acceptable? < 10%? < 20%? Without a target, reviewers will have no principled way to validate MVP-1 acceptance, and implementers will over-optimize or under-optimize.
Recommendation: Define an explicit threshold (e.g., "throughput regression < 5% under any batching load when policy is Auto") and add it to Section 6 acceptance criteria as an MVP-1 gate.
335-382: Define validation gates and transition criteria for quantization stability (Section 4.2).Section 4.2.3 states: "Off by default until kernels are proven stable; once stable, enable by default for serving workloads with opt-out."
This leaves open questions:
- How is "stability" measured? (e.g., numerical agreement tests, zero-failure operational windows?)
- Who decides when to flip the default? (e.g., CI automation, manual review, metrics threshold?)
- What telemetry/diagnostics are required to make that decision?
- Is there a rollback plan if real-world serving surfaces issues post-default-enable?
Without these gates, quantization features can get stuck "off by default" indefinitely or enabled prematurely.
Recommendation: Define explicit validation criteria (e.g., "1000+ hours of serving simulation without accuracy regression > 0.1%") and a decision review process (e.g., "team review + automated testing green-light required before flipping MVP-2→MVP-3 default"). Add to testing/acceptance plan.
tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (1)
259-274: Consider removing unused helper method.The
AssertTensorsNotEqualhelper is defined but never called in this test file. Consider removing it to reduce dead code, or add tests that exercise this assertion if tensor inequality validation is needed.src/AiDotNet.Serving/Controllers/InferenceController.cs (1)
244-257: Consider adding security rationale documentation.The character validation correctly prevents path traversal and injection attacks by restricting adapter IDs to alphanumeric characters, dashes, underscores, and dots.
Consider adding a summary comment to document the security purpose:
+/// <summary> +/// Validates that an adapter ID contains only safe characters to prevent +/// path traversal and injection attacks. Allows alphanumeric, dash, underscore, and dot. +/// </summary> private static bool IsSafeAdapterId(string adapterId)src/Inference/KVCache.cs (1)
106-159: DataType and actual storage backend can diverge, leading to misleading memory estimates.When
config.DataTypeisInt8orFloat16butTis not convertible to the chosen backend type,_useInt8Storage/_useFp16Storageremain false and the cache falls back to full‑precisionTensor<T>storage. However,GetCurrentMemoryUsage()still uses_config.DataTypeto choosebytesPerElement(e.g.,CacheDataType.Int8 => 1,CacheDataType.BFloat16 => 2), which will under‑report memory when backing storage is actuallyTensor<float>/Tensor<double>.If this scenario is expected to be possible at runtime, consider either:
- Forcing
config.DataTypeback to a full‑precision value when_useInt8Storage/_useFp16Storageare left false, or- Computing
bytesPerElementbased on the actual allocated backend (sizeof(sbyte),sizeof(Half),sizeof(T)), independent ofconfig.DataType.This will keep
GetCurrentMemoryUsage()aligned with reality and avoid surprising telemetry when quantization/FP16 are requested but not applicable for the chosenT.Also applies to: 557-565
src/Models/Results/PredictionModelResult.cs (1)
1009-1312: Session/sequence API for stateful inference is reasonable, with a couple of behavioural nuances to be aware of.
BeginInferenceSession()now exposes a nestedInferenceSessionfacade (Lines 1009–1027) that keepsPredictionModelResultinternals hidden while allowing callers to create multipleInferenceSequenceinstances. EachInferenceSequenceowns its ownInferenceOptimizer<T>and optimizedNeuralNetworkBase<T>(Lines 1153–1156), initialized lazily viaEnsureSequenceOptimizationsInitializedunder a private_sequenceLock(Lines 1242–1302).- For sequences, the full
InferenceOptimizationConfigis honoured, including stateful features (KV‑cache, paged KV‑cache, batching, speculative decoding), with an important detail: whenAttentionMaskingis left atAuto, the session path forces it toCausalin a copied config (Lines 1260–1280). That’s a sensible default for autoregressive serving, but worth documenting externally so users know why their Auto masking behaves causally in sessions.InferenceSequence.Reset()andDispose()both delegate to_sequenceOptimizer?.ClearCache()under lock (Lines 1204–1229), which is aligned with the idea that the optimizer owns all sequence‑local cache state. The sequence reuses the same optimized model instance across resets, so the heavy rewrite work happens only once per sequence.Two minor considerations (non‑blocking):
InferenceSession.Dispose()only marks the session as disposed (Lines 1124–1127); existingInferenceSequenceinstances created from that session remain usable. If you intendDispose()to terminate all child sequences, you might later want to track them and optionally invalidate on session disposal.- Inside
InferenceSequence.Predict, there is intentionally no JIT path (Lines 1199–1201). That’s fine for consistency, but it may be worth calling out in docs so users don’t expect JIT and session optimizations to stack.Functionally, the session/sequence abstractions look sound and should provide a clean entry point for future streaming/serving features.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs(1 hunks)docs/PR433_FACADE_INFERENCE_PLAN.md(1 hunks)src/AiDotNet.Serving/Controllers/InferenceController.cs(2 hunks)src/AiDotNet.Serving/Models/ServableModelWrapper.cs(4 hunks)src/Inference/InferenceOptimizer.cs(11 hunks)src/Inference/KVCache.cs(21 hunks)src/Inference/PagedAttention/PagedKVCache.cs(5 hunks)src/Inference/PagedCachedMultiHeadAttention.cs(1 hunks)src/InferenceOptimization/Kernels/AttentionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/ConvolutionKernel.cs(1 hunks)src/Models/Results/PredictionModelResult.cs(6 hunks)tests/AiDotNet.Tests/InferenceOptimization/ConvolutionKernelValidationTests.cs(1 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/PagedAttentionTests.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/InferenceOptimization/Kernels/ConvolutionKernel.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
🔇 Additional comments (38)
docs/PR433_FACADE_INFERENCE_PLAN.md (2)
1-10: Overall document quality: clear structure and phasing, but requires clarifications on a few architectural decisions and thresholds.The plan successfully:
- Reorganizes prior feedback (Phases vs MVPs mapping is now explicit in Section 9.0).
- Breaks down a large infrastructure change into actionable phases.
- Provides concrete acceptance criteria for each phase.
- Addresses new gaps (quantization, speculation scheduling, multi-LoRA).
However, clarifications on architectural decisions (Section 8), fallback behavior (Section 4.1/9.1), throughput thresholds (Section 9.2), and validation gates (Section 4.2) are needed before implementation starts to avoid rework.
Recommended next steps:
- Answer or escalate Section 8 questions before MVP-0 kickoff.
- Apply the suggested refactors to clarify acceptance criteria mapping, fallback diagnostics, regression thresholds, and quantization validation gates.
- Consider a brief sync with the implementation team to confirm junior-engineer actionability on quantization/speculation sections (4.2–4.3).
8-22: Unable to verify the review comment. The repository could not be accessed to confirm the document's structure, section organization, or specific content claims (Section 8 questions, Phase/MVP mapping, acceptance criteria alignment, etc.).tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (6)
15-22: LGTM!Class setup is clean with well-named constants that establish appropriate test dimensions.
23-41: LGTM!The test correctly verifies stateless behavior by asserting that repeated predictions with identical input produce identical output, even when KV-cache is enabled.
43-74: LGTM!The serialization test correctly verifies that deserialization overwrites the target instance's configuration with the serialized configuration. The nullable check on line 69 is appropriate.
76-127: LGTM!The test correctly validates sequence independence and KV-cache growth behavior. Proper resource management with
usingstatement on line 92.
129-154: LGTM!The reset test correctly verifies that calling Reset() restores the sequence to its initial state by comparing predictions before and after reset.
156-176: LGTM!The clone test properly validates deep copy semantics by verifying that modifications to the clone do not affect the original model's parameters.
src/AiDotNet.Serving/Models/ServableModelWrapper.cs (1)
11-11: LGTM! Clean integration of inference options.The addition of
EnableBatchingandEnableSpeculativeDecodingconfiguration is well-implemented with sensible defaults and explicit interface implementations.Also applies to: 18-19, 29-30, 36-38, 45-46, 63-64, 149-150
src/AiDotNet.Serving/Controllers/InferenceController.cs (3)
138-142: Proper HTTP status code for oversized unbatched requests.The 413 Payload Too Large response is semantically appropriate for requests exceeding the unbatched item limit.
158-166: Good improvement to error messaging.The enhanced error message now clearly indicates both the adapter-resolved name and the original model name when lookup fails, improving debuggability. This properly addresses the previous review feedback.
168-198: Well-implemented batching bypass with appropriate safeguards.The unbatched processing path properly addresses previous performance concerns with a 1000-item limit, clear error messages, and appropriate logging. The 413 status code is correctly returned for oversized requests.
tests/AiDotNet.Tests/InferenceOptimization/ConvolutionKernelValidationTests.cs (1)
8-21: Conv2D in‑channels mismatch test is well‑targeted and specific.The test builds a minimal mismatch (
inputin‑channels 3 vskernel.Shape[1] == 2) and asserts bothArgumentExceptionand that the message mentions the exact invariant ("kernel.Shape[1] == inChannels"). This tightly couples to the validation you want to guarantee and should catch regressions in the shape checks or error messaging.tests/AiDotNet.Tests/UnitTests/Inference/PagedAttentionTests.cs (1)
853-870: NET471 skip wrapper around the 4 GB PagedAttentionServer integration test looks appropriate.Guarding
PagedAttentionServer_ForModel_CreatesValidServerwith a skipped[Fact]onNET471avoids known single‑object size limits there while still exercising the large‑allocation path (4 GBavailableBytes) on newer targets. The conditional is tight and doesn’t affect the rest of the paged attention test suite.src/Inference/PagedAttention/PagedKVCache.cs (1)
84-107: Allocation safety and upfront-capacity changes inPagedKVCacheimprove robustness.
- The
totalElements > int.MaxValuecheck plusOutOfMemoryExceptioncatch (Lines 84–105) avoids invalid casts tointand turns large-allocation failures into a clearInvalidOperationExceptionwith guidance about reducingNumBlocks/memory or switching runtime.AllocateSequence’s switch toBlocksForTokens(Math.Max(1, initialTokens))andMath.Max(1, blocksNeeded)(Lines 137–141) guarantees at least one block per sequence so position‑0 writes are always backed, whileCurrentLengthstill starts atinitialTokens.These are solid defensive changes for high‑memory configurations and paged-serving scenarios.
Also applies to: 130-155
tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (1)
11-137: Inference optimizer tests give good coverage of rewrite and fallback behaviour.The suite validates:
MultiHeadAttentionLayer<float>→FlashAttentionLayer<float>when FlashAttention is enabled and KV‑cache is disabled.MultiHeadAttentionLayer<float>→PagedCachedMultiHeadAttention<float>for text generation with KV‑cache enabled, plusInferenceModeandKernelon rewritten layers.SelfAttentionLayer<float>→CachedMultiHeadAttention<float>when KV‑cache (non‑paged) is enabled, explicitly checking in‑place rewrite whencloneModel: false.- Speculative decoding fallbacks to an
NGramDraftModel<float>whenDraftModelType.SmallNeuralorDraftModelType.Customcannot be satisfied, asserting both type/name andVocabSize.These tests exercise the critical rewrite surfaces and the publicly visible outcome (
optimizer.DraftModel) rather than just echoing config, which should catch regressions in optimization logic reliably.src/Models/Results/PredictionModelResult.cs (1)
461-479: Inference optimization config round‑trip and stateless Predict path are wired correctly.
InferenceOptimizationConfigis now stored on construction (Line 840), serialized via[JsonProperty](Line 461) and restored inDeserialize(Line 2033), while all transient runtime fields (JitCompiledFunction,_inferenceOptimizer,_inferenceOptimizedNeuralModel,_inferenceOptimizationsInitialized) are explicitly reset (Lines 2046–2050). That cleanly preserves configuration across save/load without resurrecting stale runtime state.Predictnow first attempts a stateless inference‑optimized path for neural models (Lines 963–983), guarded by:
InferenceOptimizationConfig != null,Model is NeuralNetworkBase<T>,normalizedNewData is Tensor<T>.
It usesEnsureStatelessInferenceOptimizationsInitializedto lazily build a cloned, optimizedNeuralNetworkBase<T>with a stripped‑down config that disables stateful/session features (Lines 1071–1085). If no optimizations apply or a type mismatch occurs, it falls back to the existing model path and still respects the downstream JIT path when_inferenceOptimizedNeuralModelis null.- The initialization logic uses a double‑checked lock on
_inferenceOptimizationLockand a boolean flag_inferenceOptimizationsInitialized(Lines 1029–1068), which is appropriate to avoid redundant optimizer work in multi‑threaded call sites.Overall, the new stateless inference optimization layer is integrated with existing JIT/normal paths in a way that’s backward compatible and failure‑tolerant.
Also applies to: 710-852, 958-1007, 2006-2051
src/InferenceOptimization/Kernels/AttentionKernel.cs (1)
31-100: Fused attention kernel shape/mask validation and multi‑head handling look solid.
ExecuteInternalenforces 3D[batch, seq_len, features]tensors for Q/K/V and checks:
- Shared batch size (
k.Shape[0] == v.Shape[0] == q.Shape[0]),kfeature dim matchesq(k.Shape[2] == q.Shape[2]),vsequence length matchesk(v.Shape[1] == k.Shape[1]),- Mask rank and
[*, seq_len_q, seq_len_k]consistency, with a clear contract aroundmaskBatchModulo(Lines 53–89).ProcessBatchcomputesQKᵀusingSimdKernels.DotProduct, applies a scale of1/√d_k, then applies masking by indexing into a flat[maskBatch, seqLenQ, seqLenK]layout using either per‑batch (batchIdx) or modulo‑broadcasted indices for multi‑head (Lines 119–139). The mask check has been correctly changed to an epsilon‑based comparison (MathF.Abs(mask.Data[maskIdx]) < 1e-6f) to avoid raw float equality issues (Lines 134–135), and masked entries are driven to-∞soApplySoftmaxlater zeroes them.ApplySoftmaxis numerically stable (row‑wise max subtraction) and treats-∞as exact zeros before normalization, ensuring fully‑masked rows produce all‑zero distributions without NaNs (Lines 175–215).MultiHeadAttentionvalidates compatible shapes (d_model % numHeads == 0, all three tensors sharing[B, *, d_model], andK/Vsentence lengths equal) and usesReshapeForMultiHead/ReshapeFromMultiHeadthat implement an inverse mapping between[B, S, H*d_k]and[B*H, S, d_k](Lines 219–279, 281–329).- The mask handling in the multi‑head path explicitly supports both
[B, SQ, SK](broadcast across heads viamaskBatchModulo = batchSize) and[B*H, SQ, SK](per‑head masks withmaskBatchModulo = 0), with validation guarding any other batch dimension (Lines 259–272), which matches typical attention mask usage.The implementation is clear, well‑guarded against shape mismatches, and should be safe to use in optimized paths.
Also applies to: 102-217, 219-329
src/InferenceOptimization/Kernels/ConvolutionKernel.cs (7)
35-68: LGTM! Clean routing logic and input validation.The Execute method properly validates inputs, safely extracts configuration parameters with clamping, and routes to the appropriate convolution implementation based on kernel shape.
73-119: LGTM! Comprehensive validation and efficient parallelization.All necessary validations are in place (tensor dimensions, kernel channel matching, positive output dimensions), and the parallelization strategy across batch and output channels is appropriate for CPU convolution.
121-161: LGTM! Safe and correct convolution kernel implementation.The unsafe code properly validates boundaries before accessing array elements, and the index calculations for row-major tensor layout are correct.
166-215: LGTM! Complete validation and proper depthwise convolution structure.All depthwise convolution constraints are properly validated (kernel shape requirements, positive output dimensions), and the parallelization over batch×channels is optimal for this operation.
217-253: LGTM! Correct depthwise convolution kernel.The implementation properly applies a single kernel per channel without cross-channel mixing, with appropriate boundary checks.
258-319: LGTM! Complete group convolution validation and correct structure.All group convolution constraints are validated (positive groups, channel divisibility, kernel shape, output dimensions), and the per-group parallelization is appropriate.
321-365: LGTM! Correct group convolution kernel with proper offset handling.The implementation correctly computes group-specific channel offsets and applies the convolution within each group's channel range.
src/Inference/PagedCachedMultiHeadAttention.cs (5)
21-112: LGTM! Proper validation and initialization.The constructor properly validates the embedding dimension/head count divisibility constraint, and the class documentation clearly states the batchSize==1 limitation.
208-244: LGTM! Efficient stateless path with optimized operations.The method properly leverages optimized tensor multiplication (addressing previous review feedback) and FlashAttention for compute-intensive operations.
247-302: LGTM! Appropriate use of optimized operations and manual transformations.ComputeQkv uses optimized tensor multiplication. The manual loops in SplitHeads/MergeHeads are justified for tensor layout transformations that don't have specialized kernel support.
304-401: LGTM! Thread-safe weight caching with acceptable double-check pattern.The caching implementation is thread-safe. The double-checked locking at line 378 is acceptable in C# because the worst case (multiple threads passing the outer check) results only in redundant work within the lock, and cache invalidation is properly synchronized.
403-434: LGTM! Consistent inference-only layer contract.State management, training restrictions, and metadata are all properly implemented and consistent with the layer's inference-only design.
src/Inference/InferenceOptimizer.cs (8)
84-132: LGTM! Robust cloning strategy with proper error handling.The method correctly avoids mutating the user's original model by handling clone failures gracefully and recording diagnostics. The conditional cloning based on optimization applicability is efficient.
184-272: LGTM! Comprehensive KV-cache initialization with proper configuration.The initialization properly estimates memory requirements, configures sliding windows when enabled, and resolves data types based on platform capabilities and configuration.
274-369: LGTM! Robust paged KV-cache initialization with bounded retries.The bounded retry mechanism (maxAttempts=1024 with SpinWait) properly addresses the previous infinite loop concern, and the fallback to contiguous KV-cache ensures graceful degradation.
371-527: LGTM! Comprehensive and safe layer rewriting logic.The multi-stage rewriting (SelfAttention → MultiHead → Cached/Paged) is correct, and the index decrement at line 411 to re-process converted layers is safe because TryConvertSelfAttentionToMultiHead always returns MultiHeadAttentionLayer, preventing infinite loops.
529-584: LGTM! Safe and correct SelfAttention to MultiHead conversion.The conversion properly validates all constraints, creates a correct identity output projection, and preserves the original Q/K/V parameters and bias.
619-673: LGTM! Correct memory estimation with appropriate bounds.The memory calculation properly accounts for K and V tensors, batch size, and element size, with sensible clamping to prevent unreasonable sequence length estimates.
675-758: LGTM! Robust speculative decoding initialization with comprehensive fallbacks.The multi-layered error handling and fallback to NGram draft models ensures initialization succeeds even when preferred draft models are unavailable, with thorough diagnostic recording.
801-924: LGTM! Resilient cache cleanup with graceful degradation.The ClearCache implementation properly handles allocation failures by gracefully degrading to stateless mode rather than failing, and SetCustomDraftModel provides a clean public API for custom draft model injection.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
src/Inference/Quantization/QuantizedDenseLayer.cs (2)
18-69: Consider extracting shared initialization logic to reduce duplication.Both constructors share identical logic for extracting weights/biases from the source layer, quantizing weights, and copying biases (lines 30-43 vs 55-68). Extract this into a private helper method to improve maintainability.
+ private void InitializeFromSource(DenseLayer<float> source) + { + var weights = source.GetWeights(); + var biases = source.GetBiases(); + if (weights == null || biases == null) + throw new ArgumentException("Dense layer must expose weights and biases.", nameof(source)); + + var q = Int8WeightOnlyQuantization.QuantizePerRow(weights); + _weightsInt8 = q.Weights; + _rowScales = q.Scales; + + _biases = new float[biases.Length]; + Array.Copy(biases.ToArray(), _biases, biases.Length); + }Also, the bias copy loop can be simplified with
Array.Copyor similar.
108-121: Consider parallelizing the batch loop for larger batch sizes.The batch-independent loop structure at lines 108-121 is a good candidate for
Parallel.Forto improve throughput on multi-core systems. This is optional since the current implementation is correct and may be intentionally kept simple as a reference.- for (int b = 0; b < batchSize; b++) + Parallel.For(0, batchSize, b => { for (int o = 0; o < _outputSize; o++) { float sum = _biases[o]; float scale = _rowScales[o]; int wBase = o * _inputSize; for (int i = 0; i < _inputSize; i++) { sum += flat[b, i] * (_weightsInt8[wBase + i] * scale); } output[b, o] = sum; } - } + });docs/PR433_FACADE_INFERENCE_PLAN.md (2)
527-531: Critical design question (Section 8, line 529) should be decided before Phase B implementation.The question "Should a session support multiple independent sequences, or is 'one session = one sequence' acceptable for now?" is fundamental to the
BeginInferenceSession()API and cannot be left open during Phase B implementation. This affects:
- Cache isolation strategy (per-session vs per-sequence).
- Session API surface (e.g., whether to add
session.CreateSequence()/sequenceId).- Serving integration (routing of concurrent requests).
Currently labeled as a "remaining question" (line 528), but it should be a resolved decision before Phase B coding begins.
Do you want me to help finalize this design decision? The options are:
- Option A (recommended for MVP-0): "One session = one sequence"; multiple concurrent sequences use multiple sessions. Simpler implementation, session owns a single KV-cache and sequence state.
- Option B (multi-sequence): One session can manage multiple independent sequences; internal routing by sequence ID. Higher complexity, but better for serving use-cases with many short-lived requests.
Once decided, update lines 527–531 to move the decision from "remaining questions" to "resolved decisions" (Section 7).
544-552: Minor: Clarify MVP phase split in Phase C/D table entries.Lines 548–549 list Phases C–D as "MVP-0 / MVP-1" without indicating which components belong to each MVP. For example:
- Phase C (paged KV-cache): Is config plumbing MVP-0 and actual paged cache usage MVP-1? Or is all of paging MVP-0?
- Phase D (EnableBatching): Is it all MVP-0, or is serving-focused batching deferred to MVP-1?
The detailed MVP sections (9.1–9.4) clarify the breakdown, but the table would be clearer if it were explicit (e.g., "Phase C: MVP-0 (config + validation), MVP-1 (usage in sessions)" or similar).
Optional improvement: Break table entries for Phases C–D into sub-rows or add clarifying notes (e.g., "Paging backend selection + cached attention bridge" for MVP-0, deferring "actual serving throughput" to MVP-1 if appropriate).
src/LoRA/Adapters/MultiLoRAAdapter.cs (2)
449-465: Deterministic ordering implementation looks good.The use of
OrderBy(k => k, StringComparer.Ordinal)ensures stable, culture-independent sorting for deterministic serialization. The null guard for_taskAdaptersis appropriate defensive programming.Minor performance note: Sorting keys on every
GetParameters()call has O(n log n) overhead. For typical use cases with few tasks (< 10), this is negligible. If profiling reveals this as a bottleneck in scenarios with many tasks, consider caching the sorted key list and invalidating onAddTask()/RemoveTask().
705-741: BuildLayerTypeIdentifier implementation is functional but could be more robust.The method creates stable type identifiers by combining type names with metadata. The use of
AssemblyQualifiedName(line 722, 726) ensures stability across application restarts.Minor considerations:
Identifier length:
AssemblyQualifiedNameincludes version, culture, and public key token, producing very long strings. For serialization metadata, this might be acceptable, but consider whetherFullNamewould suffice for your use case.Parsing robustness: The format uses semicolons and equals signs as delimiters (line 737). While the current implementation should work because metadata values from
GetSerializationMetadata()useUri.EscapeDataString, adding explicit escaping here would make the format more self-documenting and robust against future changes.Example format:
TypeName;Key1=Value1;Key2=Value2
IfValue1contains;or=, parsing could break unless properly escaped at this level.tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (1)
187-207: Strong coverage for sessions / KV-cache / Multi‑LoRA; small test nitThe integration tests exercise the key behaviors (stateless Predict, config round‑trip, per‑sequence KV‑cache growth/independence, Reset semantics, Multi‑LoRA isolation, and
NeuralNetworkBase.Clonedeep copy) in a very targeted way; this is a solid safety net for the new inference session surface.Minor nit: in
NeuralNetworkBase_Clone_DoesNotShareParameters, you callmodel.GetParameters()andclone.GetParameters()inside the loop for every index. Caching these vectors once before the loop would avoid repeated allocations/calls, even if it doesn’t matter much in tests.Also applies to: 242-333
src/Models/Results/PredictionModelResult.cs (2)
1071-1085: Stateless inference config may miss newer stateless optimizations
CreateStatelessInferenceConfigonly forwardsEnableFlashAttentionandAttentionMaskingand forcibly disables KV‑cache, paging, batching, and speculation. That’s correct for avoiding stateful/session features, but it means any future (or current) stateless optimizations configured onInferenceOptimizationConfig(e.g.,EnableWeightOnlyQuantizationand related knobs you do copy in the session config) will never be applied on plainPredict.To keep behavior consistent, consider building the stateless config by cloning the full config and only flipping the explicitly stateful flags to
false, so new stateless options automatically flow into the stateless path:private static InferenceOptimizationConfig CreateStatelessInferenceConfig(InferenceOptimizationConfig config) => new InferenceOptimizationConfig { // copy everything from config EnableFlashAttention = config.EnableFlashAttention, AttentionMasking = config.AttentionMasking, EnableWeightOnlyQuantization = config.EnableWeightOnlyQuantization, // ...any other stateless flags... // and explicitly disable stateful/session-centric features: EnableKVCache = false, EnablePagedKVCache = false, EnableBatching = false, EnableSpeculativeDecoding = false, };Also applies to: 1330-1353
464-475: Inference optimization initialization uses hand‑rolled double‑checked lockingBoth the stateless and per‑sequence optimization initializers use a custom double‑checked locking pattern on
_inferenceOptimizationsInitialized/_sequenceInitialized. Functionally this is fine (worst case you just fall back to the non‑optimized model on a race), but it’s harder to reason about than necessary and depends on subtle .NET memory‑model guarantees.For clarity and future maintenance, consider:
- Using
Lazy<NeuralNetworkBase<T>?>orLazyInitializer.EnsureInitializedfor_inferenceOptimizedNeuralModel/_sequenceOptimizedNeuralModel, or- Marking the flags as
volatileand documenting that these fields are purely an optimization guard.That would make the initialization intent explicit and reduce the chance of subtle races if this code evolves.
Also applies to: 1029-1068, 1279-1376
src/Serving/ContinuousBatching/ContinuousBatcher.cs (1)
371-424: Speculative decoding integration looks sound; consider tightening vocab fallback and diagnosticsThe speculative path is well‑guarded:
ShouldUseSpeculativeDecodinghandles config, policy, load, and low‑acceptance backoff.RunDecodeStepSpeculativealways falls back cleanly toRunDecodeStepon any failure (decoder init, runtime exception, etc.).- The per‑step flags and reasons (
LastStepUsedSpeculation,LastStepSpeculationTokens,LastStepSpeculationReason) are very helpful for tests and observability.Two small refinements you might consider:
Vocab fallback in
DetectVocabSize(lines 626–639): returning50000on probe failure can produce a silent mismatch between draft model vocab and target logits ifSpeculativeDecoderdoesn’t guard for this; you’re already treating any probe exception as “disable speculation”, so it might be safer to also treat an inconclusive shape probe as a hard disable instead of assuming 50k.
LastStep*fields when there is no work: whenbatch.Count == 0,Step()returns early and the fields keep their previous values. If you expect external callers to rely on these for per‑step introspection (beyond tests), you might want to explicitly reset them on the no‑work path so they always describe the most recent actual decode step.Both are minor; the current behavior is functionally correct and always falls back to the baseline path on failures.
Also applies to: 426-503, 514-626
src/AiDotNet.Serving/Controllers/InferenceController.cs (2)
135-145: 413 mapping for large unbatched requests is reasonable but tightly coupled to message textCatching
ArgumentExceptionand mapping the “maximum allowed when batching is disabled” case to HTTP 413 makes sense for signaling “payload too large” to clients. The only caveat is that it relies on a substring match against the exception message, which is easy to break if the message text is ever refactored.If you want to decouple this a bit, consider either:
- Defining a small custom exception type for this case (e.g.,
UnbatchedRequestTooLargeException) and catching that explicitly, or- Hoisting the message substring into a private constant used both where you throw and where you check.
Not a blocker; current behavior is clear and localized.
Also applies to: 156-218
156-218: Adapter‑aware model resolution and validation look solid
PredictWithType<T>now:
- Resolves an adapter‑aware
effectiveModelNameviaResolveModelNameWithAdapter,- Tries
GetModel<T>(effectiveModelName)first, then falls back tomodelName,- Produces a clearer error message listing both attempted names.
ResolveModelNameWithAdapter:
- Gracefully handles missing headers or
Requestby routing to the base model,- Validates adapter IDs (length and allowed characters) via
IsSafeAdapterId,- Logs invalid IDs at Debug/Warning appropriately and routes back to the base model,
- Logs the chosen adapter variant when routing succeeds.
This matches the safety/observability goals for per‑request adapter routing without impacting existing callers that don’t send headers.
Also applies to: 220-270
src/Helpers/DeserializationHelper.cs (2)
332-412: Consider extracting MultiLoRAAdapter parsing to a separate method.The nested local functions and multi-step parsing make this block significantly more complex than other layer constructions. Extracting to a helper method would improve readability and testability.
Example refactoring:
else if (genericDef == typeof(AiDotNet.LoRA.Adapters.MultiLoRAAdapter<>)) { - // (inline 80 lines of parsing logic) + instance = CreateMultiLoRAAdapter<T>(type, inputShape, outputShape, additionalParams); }Then define:
private static object CreateMultiLoRAAdapter<T>(Type type, int[] inputShape, int[] outputShape, Dictionary<string, object>? additionalParams) { // Move all parsing logic here }
623-652: TryCreateActivationInstance needs better error handling.
Type.GetType(typeName, throwOnError: false)will returnnullfor types not in the current assembly or not fully qualified. This could silently fail for valid activation types that require assembly-qualified names.Consider searching loaded assemblies or requiring fully-qualified type names:
- var type = Type.GetType(typeName, throwOnError: false); + // Search in current assembly and AiDotNet assemblies + var type = Type.GetType(typeName, throwOnError: false) + ?? Assembly.GetExecutingAssembly().GetType(typeName) + ?? Assembly.GetAssembly(typeof(IActivationFunction<>))?.GetType(typeName); if (type == null) { + // Log or collect diagnostic for missing type return null; }Alternatively, document that activation type names must be assembly-qualified.
tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (1)
139-172: Consider using Assert.InRange for numerical comparison.The manual tolerance check on line 170 works but could be more concise with xUnit's
Assert.InRange:for (int i = 0; i < y.Length; i++) { - Assert.True(Math.Abs(baseline[i] - y[i]) < 1e-1f, $"Mismatch at {i}: {baseline[i]} vs {y[i]}"); + Assert.InRange(y[i], baseline[i] - 1e-1f, baseline[i] + 1e-1f); }This provides clearer failure messages automatically.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
docs/PR433_FACADE_INFERENCE_PLAN.md(1 hunks)src/AiDotNet.Serving/Controllers/InferenceController.cs(2 hunks)src/Configuration/InferenceOptimizationConfig.cs(5 hunks)src/Helpers/DeserializationHelper.cs(5 hunks)src/Inference/InferenceOptimizer.cs(11 hunks)src/Inference/PagedCachedMultiHeadAttention.cs(1 hunks)src/Inference/Quantization/Int8WeightOnlyQuantization.cs(1 hunks)src/Inference/Quantization/QuantizedDenseLayer.cs(1 hunks)src/LoRA/Adapters/MultiLoRAAdapter.cs(6 hunks)src/Models/Results/PredictionModelResult.cs(6 hunks)src/NeuralNetworks/Layers/ILayerSerializationExtras.cs(1 hunks)src/NeuralNetworks/NeuralNetworkBase.cs(6 hunks)src/Serving/ContinuousBatching/ContinuousBatcher.cs(7 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Serving/ContinuousBatchingTests.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/NeuralNetworks/NeuralNetworkBase.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (43)
src/Inference/Quantization/QuantizedDenseLayer.cs (1)
71-79: LGTM!The inference-only contract is clearly enforced: training properties return appropriate values, training methods throw
NotSupportedExceptionwith clear messages, and the design is consistent with the stated purpose.Also applies to: 132-153
src/Inference/Quantization/Int8WeightOnlyQuantization.cs (2)
7-21: LGTM!The
QuantizedWeightsstruct is well-designed as an immutable readonly struct with clear semantics. Usingsbyte[]for weights andfloat[]for per-row scales aligns with standard per-row quantization patterns.
23-61: LGTM!The per-row quantization logic is correct:
- Symmetric INT8 range [-127, 127] avoids asymmetric representation issues
- Scale computation handles zero-weight rows gracefully (line 46)
- Post-round clamping is defensive and correct
This is a one-time model-loading operation, so the straightforward implementation is appropriate.
docs/PR433_FACADE_INFERENCE_PLAN.md (3)
503-515: Excellent: MVP-phase mapping table resolves previous feedback.The mapping table clearly links each acceptance criterion to phases (A–E) and MVP milestones (MVP-0/1/2/3), directly addressing the previous feedback request to make the acceptance criteria hierarchical and phase-aware. This eliminates ambiguity and helps reviewers validate implementation scope quickly.
344-344: Unable to verify the contradiction in the review comment due to repository access limitations. Manual verification of lines 344 and 374–375 indocs/PR433_FACADE_INFERENCE_PLAN.mdand corresponding codebase examination is required to confirm whether KV-cache INT8 quantization is currently supported or remains planned work.
133-136: I was unable to access the repository to verify the specific file and lines mentioned in the review comment. The repository clone operation failed, preventing me from:
- Confirming the exact current state of lines 133–136 and 560–561
- Checking whether failure-category taxonomy, logging-level mapping, or diagnostics scope details are documented elsewhere in the document
- Verifying the claim about prior feedback requesting these specifics
- Cross-referencing context to assess the completeness of the diagnostics section
To properly verify this review comment, I would need direct access to the
docs/PR433_FACADE_INFERENCE_PLAN.mdfile. Please provide the file content or ensure repository access is available.src/NeuralNetworks/Layers/ILayerSerializationExtras.cs (1)
1-21: LGTM! Clean interface design for serialization extras.The interface provides a focused API for managing non-trainable parameters during serialization, complementing the existing
ILayer<T>.ParameterCountfor trainable parameters. The internal visibility is appropriate, and the documentation clearly explains the use case for frozen base weights in adapter scenarios.src/LoRA/Adapters/MultiLoRAAdapter.cs (4)
492-510: LGTM! Consistent deterministic ordering.The ordering in
SetParametersmatchesGetParameters, which is essential for correct serialization round-tripping. The same stable ordering ensures parameters are written and read in the same sequence.
630-649: LGTM! Gradient ordering consistent with parameters.The gradient ordering matches
GetParameters/SetParameters, ensuring consistent parameter-gradient alignment. The logic correctly populates gradients only for the current adapter (line 641), with zeros for inactive tasks, which is appropriate for the multi-task training model.
652-679: LGTM! Correct ILayerSerializationExtras implementation.The implementation correctly exposes frozen base layer parameters as "extra" parameters for serialization:
- When
_freezeBaseLayeris true, base layer parameters are excluded fromParameterCountbut exposed viaGetExtraParameters()for complete serialization.- When false, no extra parameters exist since base layer parameters are already included in the main parameter vector.
The validation in
SetExtraParameters(lines 671-676) appropriately guards against parameter count mismatches.
681-703: Verify metadata serialization format and type conversion.The metadata serialization uses a pipe-separated format with URI escaping. There are concerns worth verifying:
Line 694 type conversion:
Convert.ToDouble(_taskAdapters[t].Alpha)assumesTcan be converted todouble. SinceAlphais typed asT(generic), verify thatThas appropriate constraints ensuring numeric convertibility at compile-time. IfTlacks constraints, this creates a runtime conversion risk.Serialization format round-tripping: The combination of pipe-separated values and URI escaping creates a nested encoding scheme. Verify that corresponding deserialization code correctly:
- Splits on
|after URI unescaping- Handles edge cases (empty task lists, special characters in task names like
"task|with|pipes")- Validates metadata completeness
Ensure comprehensive unit tests cover round-trip serialization with edge cases.
tests/AiDotNet.Tests/UnitTests/Serving/ContinuousBatchingTests.cs (1)
492-737: Speculative decoding tests and helpers are well‑designedThe new tests do a good job of exercising the speculative policies:
ForceOnvsForceOffbehavior,- Auto backoff on low acceptance rate,
ThroughputFirstdisabling under larger batches,- Permanent disable after a draft failure.
The nested
DeterministicDraftModelandThrowingDraftModelkeep the scenarios deterministic and clearly tied to behavior. No functional issues spotted; this is a solid test harness for the new speculative path.Also applies to: 876-936
src/Helpers/DeserializationHelper.cs (4)
516-566: Parameter parsing implementation looks solid.The parsing logic correctly handles multiple types (int, long, double, bool, string) and uses InvariantCulture for numeric parsing. The delimiter-based parsing is straightforward and robust.
96-108: LGTM: Explicit constructor selection avoids ambiguity.Using
GetConstructorwith explicit parameter types prevents runtime ambiguity when DenseLayer has multiple constructors. The activation creation viaTryCreateActivationInstanceis appropriate.
452-502: Activation fallback logic handles edge cases well.The multi-tiered approach (vector activation → scalar activation → enum-based factory → error) ensures compatibility with different serialization formats while maintaining clear error messages.
654-665: ResolveDefaultHeadCount heuristic is reasonable.The preference order
[8, 4, 16, 12, 6, 2, 1]covers common transformer configurations. Fallback to 1 ensures the method always succeeds.tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (4)
14-33: Test validates attention rewrite behavior correctly.The test confirms FlashAttention replacement when enabled, and verifies the original layer type is removed. Using
cloneModel: falseis appropriate for testing in-place rewrites.
36-64: Comprehensive validation of KV-cache integration.The test correctly verifies:
- PagedCachedMultiHeadAttention layer creation
- InferenceMode flag enabled
- Kernel attachment
The loop checking each layer's properties (lines 56-63) ensures the optimizer wired all layers correctly.
92-113: Fallback test correctly validates NGram draft model selection.Line 111 now uses
Assert.Contains("NGramDraftModel", ...)which properly validates the fallback behavior rather than checking the config value. This addresses the past review comment.
174-263: Test helper models are well-designed.The helper methods create minimal models with:
- Deterministic parameter initialization for reproducible tests
- Appropriate complexity for unit testing
- Clear naming and structure
The deterministic weight initialization patterns (e.g.,
(i % 17) - 8) / 8.0f) ensure consistent test behavior across runs.src/Inference/PagedCachedMultiHeadAttention.cs (6)
114-207: Forward implementation correctly addresses past review feedback.The method now:
- Caches converted weights via
EnsureKernelWeightCache()(line 154)- Uses
ArrayPoolfor temporary buffers (lines 161-163)- Increments
_currentPositiononly after successful token processing (line 196)The implementation is efficient and correct for inference.
209-255: Stateless path correctly uses optimized matrix operations.Lines 227 and 251-253 use
Tensor.Multiplyfor matrix operations instead of manual nested loops, addressing past review feedback. The remaining manual loop (lines 233-243) applies bias and activation element-wise, which is appropriate.
257-303: Head splitting/merging operations are correct.The manual loops correctly reshape tensors between [B,S,E] and [B,H,S,D] formats. Using explicit indexing ensures correct data layout for FlashAttention.
305-321: Weight conversion handles transpose correctly.
MatrixToFloatForKerneltransposes the matrix during conversion (lines 311-318), which aligns with the comment on line 152 explaining the kernel expects a different layout. The implementation is correct.
377-402: Kernel weight caching is thread-safe and correctly invalidated.The cache implementation:
- Uses double-checked locking (line 379, then 384-390)
- Invalidates cache when parameters change (line 374)
- Null-coalescing assignment ensures thread-safe lazy init
This correctly addresses the past review feedback about caching weight conversions.
49-49: Inference-only constraints are correctly enforced.
SupportsTrainingreturnsfalse(line 49), andBackward/UpdateParametersthrowNotSupportedException. This consistency prevents training attempts and addresses past review feedback.Also applies to: 411-426
src/Configuration/InferenceOptimizationConfig.cs (6)
119-186: KV-cache configuration properties are well-designed.The new properties provide comprehensive control over KV-cache behavior:
- Sensible defaults (paged cache enabled, FP16 auto-selection)
- Clear documentation explaining trade-offs
- Industry-standard values (block size 16, window size 1024)
The beginner-friendly documentation helps users understand the impact of each setting.
189-209: Attention settings provide appropriate defaults.Enabling Flash Attention by default (line 198) optimizes memory bandwidth, and the
Automasking mode (line 208) allows intelligent causal mask selection based on model type. This is good UX.
346-356: Validation logic correctly guards new configuration properties.The added validation ensures:
KVCacheWindowSize > 0when sliding window is enabledPagedKVCacheBlockSize > 0when paged cache is enabledError messages are descriptive and follow existing patterns.
442-463: Speculation configuration provides runtime flexibility.The
Autodefaults for both policy (line 449) and method (line 462) allow the optimizer to make intelligent runtime decisions. The documentation on lines 404-407 clearly states the MVP fallback behavior for SmallNeural drafts.
466-485: Weight quantization defaults are appropriately conservative.Disabling weight-only quantization by default (line 484) is the right choice for an experimental feature. The documentation clearly explains the opt-in nature and safe fallback behavior.
489-629: New enums are well-documented and extensible.The enum definitions provide:
- Clear, descriptive value names
- Comprehensive XML documentation for each value
Autooptions for intelligent defaults- Future-proofing with Medusa/Eagle placeholders
The documentation quality is excellent, especially for beginners.
src/Inference/InferenceOptimizer.cs (11)
85-134: OptimizeForInference API is well-designed and safe.The public API:
- Returns both optimized model and success flag for caller inspection
- Safely clones model before rewrites to avoid mutating user's original (lines 107-112)
- Handles clone failures gracefully with diagnostics (lines 114-125)
- Skips unnecessary cloning when no rewrites needed (line 105)
The defensive error handling ensures user models are never corrupted.
276-340: Paged KV-cache initialization is robust.The implementation:
- Falls back to contiguous cache when no paged layers present (lines 291-295)
- Uses bounded retry for sequence allocation (via
TryAllocatePagedSequenceId)- Records diagnostics on allocation failure (lines 317-321)
- Cleans up state on failure (lines 322-326)
This addresses past review feedback about infinite loops and provides good observability.
342-360: Bounded retry correctly addresses past review feedback.The
TryAllocatePagedSequenceIdmethod now usesmaxAttempts = 1024withSpinWaitfor backoff, preventing infinite loops when allocation consistently fails. This directly addresses the past review comment.
391-536: Attention optimization logic is comprehensive and correct.The rewrite logic:
- Properly resolves causal masking (lines 393-398)
- Handles SelfAttention → MultiHead → Cached/Paged progression (lines 411-430)
- Transfers parameters correctly with
SetParameters(e.g., lines 455, 468, 486)- Re-processes converted layers by decrementing loop index (lines 419-420)
The layer replacement preserves model semantics while enabling optimizations.
577-632: SelfAttention to MultiHead conversion correctly adds identity projection.The conversion creates an identity output weight matrix (lines 618-624) and copies the bias (lines 627-628), ensuring the transformation is semantically equivalent. This allows downstream rewrites to apply consistently.
655-665: Causal mask inference heuristic is pragmatic.The
InferCausalFromModelmethod (lines 655-665) defaults to causal masking when KV-cache or speculative decoding is enabled, which aligns with industry expectations for autoregressive models. This avoids requiring users to set TaskType explicitly.
538-575: Weight quantization is safely applied with proper fallback.The implementation:
- Restricts to
floattype only (lines 546-550)- Preserves activation functions during conversion (lines 559-561)
- Catches and logs quantization failures without crashing (lines 566-568)
- Records diagnostics for observability (lines 542-543, 568, 573)
The conservative approach ensures safety for experimental features.
726-778: Speculation initialization has robust fallback logic.The method implements a comprehensive fallback chain:
- Custom draft model if provided via
SetCustomDraftModel(lines 733-744)- Requested draft model type (lines 746-751)
- NGram fallback if requested type unavailable (line 753)
- NGram fallback on exception (lines 762-777)
The diagnostics at each step (lines 737, 742, 754-758, 764, 768, 773) provide excellent observability.
800-806: CreateNeuralDraftModel correctly signals unavailability.The method returns
nullwith diagnostic recording (line 804) rather than throwing, which aligns with the MVP's fallback-to-NGram behavior. The documentation (lines 792-799) clearly explains why SmallNeural is unavailable.
852-903: ClearCache correctly handles paged cache reallocation.The method:
- Uses bounded retry via
TryAllocatePagedSequenceId(line 867) - addresses past feedback- Falls back to disabling inference mode on allocation failure (lines 876-885)
- Performs best-effort cleanup with try-catch (lines 857-864)
- Re-wires layers with new sequence ID (lines 892-901)
The defensive approach ensures the session remains usable even under resource pressure.
946-972: SetCustomDraftModel provides clean API for advanced scenarios.The method:
- Simple, single-purpose API
- Comprehensive documentation with code example (lines 960-965)
- Proper null validation (line 971)
This enables advanced users to provide custom draft models while keeping the API surface minimal.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Inference/KVCache.cs (1)
285-301: Consider optimizing 4D indexing to reduce allocations.The code creates a new index array (
new[] { b, h, targetPos, d }) for every element access. This pattern is repeated throughout the file (GetCached lines 358-374, Update lines 419-435, CopyBatchState lines 616-636, HandleSlidingWindowEviction lines 833-853) and creates significant allocation overhead.If the
Tensor<T>API supports flat or span-based indexing, consider refactoring to use a more efficient indexing approach. For example:// Compute flat index once int flatIndex = ComputeFlatIndex(b, h, targetPos, d, shape); cache[flatIndex] = value;This would reduce allocations and improve performance, especially in tight loops.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Inference/KVCache.cs(22 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (5)
src/Inference/KVCache.cs (3)
106-164: Multi-backend initialization logic is sound.The constructor correctly initializes Int8 and FP16 storage backends based on
config.DataTypeand type compatibility. The type checks and converter lambdas handle boxing/unboxing appropriately for generic type conversions.
645-740: Int8 quantization helpers are correctly implemented.The quantization logic correctly addresses the past review comment:
- Line 709:
RescaleInt8Layernow iterates over all batches in the layer (_sequenceLengths[layerIndex].Length), not just thebatchSizeparameter. This ensures all cached entries are consistently rescaled when the layer scale increases.Additional observations:
- The scale-only-grows strategy (lines 692-697) is correct for dynamic quantization. Scales are reset to zero on
Clear()(lines 490-494).QuantizeToInt8clips to[-127, 127]rather than the fullsbyterange[-128, 127](lines 731-732). This maintains symmetry around zero, which is standard practice in symmetric quantization.Based on learnings, the past review concern about partial batch rescaling has been resolved.
865-871: LGTM.The allocation check correctly branches on the active storage backend (Int8, FP16, or full-precision) and returns the appropriate null check.
tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (2)
23-232: Excellent integration test coverage.The test suite comprehensively validates key integration scenarios:
- Stateless prediction: Verifies KV-cache doesn't leak state between standalone
Predict()calls (lines 23-41).- Serialization: Confirms
InferenceOptimizationConfiground-trips correctly (lines 43-74).- Sequence independence: Validates KV-cache lengths grow independently per sequence (lines 76-127).
- Reset behavior: Ensures
Reset()restores initial sequence state (lines 129-154).- Int8 quantization: Confirms quantized storage is used when configured (lines 156-179).
- Multi-LoRA isolation: Verifies task selection is isolated per sequence (lines 181-210).
- Clone independence: Ensures cloned networks don't share parameter storage (lines 212-232).
All tests follow clear Arrange-Act-Assert patterns with deterministic inputs and appropriate assertions.
234-394: Well-designed test helpers.The helper methods provide clean abstractions for:
- Deterministic model creation with controlled parameter initialization (lines 267-358).
- Flexible tensor construction with predictable values (lines 360-368).
- Tolerance-based and difference-based tensor assertions (lines 370-394).
The deterministic parameter patterns (e.g.,
((i % 23) - 11) / 11.0fon line 353) ensure reproducible test behavior while providing non-trivial values.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Inference/PagedAttention/PagedAttentionKernel.cs (1)
510-525: Clarify misleading comment about array rooting.The comment at lines 514-516 about "rooted array so GC cannot collect it while spans are in use" is misleading. The
ToArray()method creates a new array copy that is GC-managed like any other array. The returned array is rooted by normal variable assignment (e.g.,keyT,valueTinUpdateCache), not by any special mechanism. While the code is functionally correct, the comment may confuse readers about GC semantics.Consider clarifying the comment:
if (typeof(T) == typeof(float)) { - // Safe: runtime-verified T == float. - // Return a rooted array so GC cannot collect it while spans are in use. + // Safe: runtime-verified T == float. + // ToArray() creates a defensive copy that can be safely stored. return (T[])(object)source.ToArray(); }
🧹 Nitpick comments (10)
tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (1)
100-123: Consider using TryGetValue pattern for clearer test failures.Lines 100, 110, 116, and 122 use direct dictionary indexing to access statistics. If the expected keys are missing, this will throw
KeyNotFoundExceptioninstead of a clear assertion failure. Other tests in this file (lines 175-178, 199-202, 224-225) useTryGetValuewith explicit assertions, which produces more descriptive failures.Apply this pattern for consistency:
-var lengthsAfterFirst = (int[])statsAfterFirst["KVCache_SequenceLengths"]; +Assert.True(statsAfterFirst.TryGetValue("KVCache_SequenceLengths", out var lengthsAfterFirstObj)); +var lengthsAfterFirst = (int[])lengthsAfterFirstObj;Apply similar changes at lines 110, 116, and 122.
src/Serving/ContinuousBatching/ContinuousBatcherConfig.cs (1)
36-64: Consider clarifying the interaction between speculation configuration properties.The configuration surface now has four related properties:
EnableSpeculativeDecoding,SpeculationPolicy,SpeculativeMethod, andUseTreeSpeculation. The documentation mentions that "Some speculative methods may implicitly enable this internally" (line 62), but doesn't specify which methods or how these properties interact.Consider adding XML documentation that explicitly describes:
- The precedence order when these properties conflict
- Which
SpeculativeMethodvalues implicitly enable tree speculation- Whether
EnableSpeculativeDecoding = falseoverrides all other settingstests/AiDotNet.Tests/UnitTests/Inference/SpeculativeDecodingTests.cs (1)
656-688: Test may be brittle due to reliance on internal adaptive thresholds.This test asserts that
decoder.Config.NumDraftTokens < 4after generating 24 tokens with a low acceptance rate. The test assumes that the adaptive draft length logic will reduceNumDraftTokens, but this depends on internal thresholds (e.g., minimum work count, acceptance rate thresholds) that are not explicitly documented in the test.If the adaptive logic's thresholds change (e.g., requiring more than 24 tokens to adapt), this test could fail unexpectedly. Consider either:
- Generating more tokens to ensure the threshold is definitely exceeded
- Adding a comment explaining the minimum tokens required for adaptation
- Verifying that
AdjustDraftLengthwas actually called using a mock or test hooktests/AiDotNet.Tests/UnitTests/Serving/ContinuousBatchingTests.cs (3)
588-648: Test relies on auto-backoff occurring within 12 iterations.The test loops up to 12 times waiting for auto-backoff to trigger due to low acceptance rate. The success of this test depends on internal thresholds in
ContinuousBatcher(e.g., minimum draft tokens before checking acceptance rate, acceptance rate threshold).If these thresholds change or if the 12-iteration limit is insufficient, the test will fail with
Assert.True(sawAutoBackoff)(line 647). Consider either:
- Increasing the iteration count to provide more margin
- Adding diagnostic output showing why backoff didn't occur
- Making the thresholds configurable for testing
726-774: Clarify semantics ofLastStepUsedSpeculationwhen speculation fails.Line 769 asserts
usedSpeculationFirst = truewith the comment "ForceOn decision, even though it failed internally". This reveals thatLastStepUsedSpeculationreflects the decision to use speculation rather than whether speculation succeeded.This could be confusing for users monitoring speculation metrics. When a draft model throws an exception,
LastStepUsedSpeculation = truebut the batcher actually falls back to baseline decoding. Consider:
- Renaming to
LastStepAttemptedSpeculationorLastStepSpeculationDecision- Adding a separate
LastStepSpeculationSucceededflag- Updating documentation to clarify that the flag reflects the decision, not the outcome
914-954: Consider extracting shared test helpers to a common test utilities class.
DeterministicDraftModelappears in bothSpeculativeDecodingTests.cs(lines 691-729) andContinuousBatchingTests.cs(lines 914-954). Similarly structured mock implementations could be consolidated into a shared test utility class to reduce duplication and improve maintainability.src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs (2)
61-63:AcceptanceRatecalculation may be inconsistent in mixed-mode usage.The
AcceptanceRateproperty (line 61-63) determines the calculation based on which counters are non-zero, rather than checking the_config.UseTreeSpeculationflag. If a decoder is used in classic mode first (incrementing_totalDraftTokens), then switched to tree mode,AcceptanceRatewill continue using the classic calculation even though tree mode is active.Consider either:
- Checking
_config.UseTreeSpeculationexplicitly to determine which counters to use- Documenting that
ResetStatistics()should be called when switching modes- Preventing mode switching after the first generation
320-335: Tree-to-classic statistics conversion may lose valuable diagnostic information.Lines 320-335 convert tree-based statistics to the classic
StepStatisticsformat. The conversion maps:
TreeNodes→DraftTokensBestPathLength→AcceptedTokensThis loses tree-specific metrics like
PathsExploredandTreeNodesthat could be valuable for debugging and performance analysis. Consider either:
- Extending
StepStatisticsto include optional tree-specific fields- Adding a separate
TreeStepStatisticstype that preserves all tree metrics- Documenting that tree statistics are approximate when converted to classic format
src/Serving/ContinuousBatching/ContinuousBatcher.cs (2)
426-503: Consider making speculation policy thresholds configurable.The speculation decision logic uses several hardcoded thresholds:
- Line 459:
batch.Count == 1for ThroughputFirst- Line 473:
MaxBatchSize / 2for Auto policy- Line 492:
AcceptanceRate < 0.25for auto-backoff- Line 494: Cooldown period of
25iterationsThese values significantly impact speculation behavior but cannot be tuned without code changes. Consider:
- Adding configuration properties for key thresholds (e.g.,
AutoBackoffAcceptanceThreshold,AutoBackoffCooldownIterations)- Documenting the rationale for these specific values
- Making batch size thresholds a fraction or percentage of
MaxBatchSize
633-647: Vocab size fallback may cause silent failures.Lines 642-646 return a fallback vocab size of
50000when detection fails. The comment states "speculative decoding will be disabled if the shapes don't line up," but this isn't guaranteed—if the model happens to have a similar vocab size, the mismatch might not be immediately detected.Consider either:
- Propagating the exception instead of returning a fallback (let
EnsureSpeculativeDecoderhandle it at line 538-543)- Using a more conservative approach like returning
-1or0to explicitly signal detection failure- Logging a warning when using the fallback value
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/PR433_PHASE_AUDIT.md(1 hunks)src/Helpers/DeserializationHelper.cs(5 hunks)src/Inference/InferenceOptimizer.cs(13 hunks)src/Inference/PagedAttention/PagedAttentionKernel.cs(8 hunks)src/Inference/PagedCachedMultiHeadAttention.cs(1 hunks)src/Inference/Quantization/Int8WeightOnlyQuantization.cs(1 hunks)src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs(8 hunks)src/Serving/ContinuousBatching/ContinuousBatcher.cs(7 hunks)src/Serving/ContinuousBatching/ContinuousBatcherConfig.cs(1 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/PagedAttentionTests.cs(4 hunks)tests/AiDotNet.Tests/UnitTests/Inference/SpeculativeDecodingTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Serving/ContinuousBatchingTests.cs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/PR433_PHASE_AUDIT.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Inference/Quantization/Int8WeightOnlyQuantization.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
🔇 Additional comments (29)
tests/AiDotNet.Tests/UnitTests/Inference/PagedAttentionTests.cs (3)
2-2: LGTM!The new using directive supports the quantized attention pathway test added below.
782-840: Verify that Forward methods handle empty KV cache correctly.This test allocates sequences with length 1 but does not pre-populate the KV cache with data before calling
ForwardandForwardQuantized(lines 828-829). Other tests in this file (e.g.,PagedAttentionKernel_ComputeAttention_ProducesOutputat lines 682-686) write keys and values to the cache before computing attention.Confirm that the
ForwardandForwardQuantizedmethods are designed to work with an empty cache, or consider populating the cache with test data to match the pattern used elsewhere.
914-931: Good approach to handle platform-specific memory limitations.The conditional compilation correctly skips the test on .NET Framework 4.7.1 where large contiguous allocations can exceed single-object limits. The skip message is clear and the
Integrationtrait categorization is appropriate for non-NET471 targets.tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (4)
23-41: LGTM!The test correctly verifies stateless prediction behavior when inference optimizations are configured. The logic is sound: identical inputs should produce identical outputs on successive calls.
43-74: LGTM!The test properly verifies that serialization preserves the
InferenceOptimizationConfig. The approach of creating a result with one config, deserializing into a result with a different config, and asserting the original config is restored is a solid validation of the serialization contract.
129-279: LGTM!All remaining test methods are well-structured and correctly validate their respective behaviors:
- Reset functionality properly restores initial state
- KV-cache quantization modes are verified through statistics
- Paged KV-cache and attention configurations are validated
- Multi-LoRA task isolation is correctly tested with non-equal output assertions
- Clone operation properly creates independent parameter copies
The tests after line 156 already use the
TryGetValuepattern consistently for accessing statistics dictionaries.
281-441: LGTM!The helper methods are well-implemented:
- Factory methods properly construct deterministic test fixtures with appropriate null checking (line 291)
- Deterministic parameter initialization ensures reproducible tests
- Model construction helpers (
CreateDeterministicMultiLoRAModel,CreateDeterministicAttentionOnlyModel) correctly set up complex scenarios with explicit parameter values- Assertion helpers (
AssertTensorsEqual,AssertTensorsNotEqual) provide clear, tolerance-aware comparisonssrc/Helpers/DeserializationHelper.cs (4)
51-56: LGTM: Flexible serialization format.The parsing logic enables embedding constructor parameters directly in the layer type identifier, which is a clean approach for serialization.
452-501: LGTM: Backward-compatible activation construction.The implementation correctly handles multiple activation types (vector/scalar) with appropriate fallbacks for backward compatibility. The logic flow is sound.
516-566: LGTM: Robust parsing with culture-invariant conversions.The parsing logic is defensive and correctly uses
InvariantCulturefor numeric conversions, preventing culture-dependent deserialization issues.
693-711: Excellent improvement: precise exception handling.This change correctly addresses the past review feedback. The exception handling now:
- Returns
nullonly for expected failures (missing parameterless constructor viaMissingMethodException)- Wraps and rethrows unexpected exceptions, preserving diagnostic information
- Handles both direct and wrapped
MissingMethodExceptioncasesThis makes debugging much easier and prevents silent failures for serious errors like
TypeLoadExceptionor security exceptions.Based on past review comments, this implementation resolves the previously flagged issue.
src/Inference/PagedAttention/PagedAttentionKernel.cs (5)
28-28: LGTM: Visibility changed to internal.The visibility change from public to internal appropriately restricts the API surface and aligns with the PR's goal of providing an internal optimization infrastructure.
Also applies to: 531-531, 556-556
298-324: LGTM: Cache extension logic is correct.The method properly ensures the cache has sufficient capacity before writing by computing the required length and extending the sequence if needed. Error handling is explicit and provides clear diagnostics.
338-391: LGTM: ArrayPool usage is properly managed.The Forward method correctly uses ArrayPool with try/finally to ensure buffers are returned even if exceptions occur. The logic flow through Q/K/V projections, cache update, attention computation, and output projection is clean and correct.
393-443: LGTM: Quantized forward path is well-structured.The ForwardQuantized method validates weight dimensions upfront, properly manages ArrayPool buffers with try/finally, and correctly calls the quantized matrix-vector multiplication helper. The implementation mirrors the non-quantized path appropriately.
460-484: LGTM: Quantized matvec implementation is correct.The MatVecMulInt8 method correctly implements per-row quantized matrix-vector multiplication: validates dimensions, computes the dot product of quantized weights with the input vector, and applies the per-row scale factor. The implementation aligns with standard int8 weight-only quantization.
src/Inference/PagedCachedMultiHeadAttention.cs (5)
22-52: LGTM: Internal visibility and thread-safe weight caching.The class is appropriately marked internal, and the kernel weight caching infrastructure uses proper locking (
_kernelWeightsLock) to ensure thread-safe access to cached weights. The separation of float and int8 quantized caches is clean.
121-235: LGTM: Forward method correctly manages state and resources.The Forward method properly handles the stateless fallback, enforces the documented batchSize==1 constraint, manages ArrayPool buffers with try/finally, and increments
_currentPositiononly after successful token processing (line 224, after kernel forward and output population). The implementation addresses previous review feedback about position increment timing.
405-437: LGTM: Weight cache initialization is thread-safe.The EnsureKernelWeightCache method uses a correct double-check locking pattern: the early return (lines 407-410) provides fast-path performance, and the lock-protected null-coalescing assignments (lines 414-417) ensure thread-safe initialization. Quantized cache population is properly gated by EnableWeightOnlyQuantization and type checks.
237-283: LGTM: Stateless forward path uses optimized tensor operations.The ForwardStateless and ComputeQkv methods correctly leverage tensor.Multiply operations for matrix multiplications (lines 255, 279-281), addressing previous feedback about manual loop inefficiency. The remaining manual loops (lines 261-271) are appropriately used only for bias addition and element-wise activation.
333-349: LGTM: Matrix transposition is intentional and documented.The MatrixToFloatForKernel method intentionally transposes the matrix during conversion to match the layout expected by PagedAttentionKernel.MatVecMul, as documented in the Forward method comments (lines 159-160). The implementation correctly produces [outDim, inDim] row-major layout from [inDim, outDim] input.
src/Inference/InferenceOptimizer.cs (7)
85-134: LGTM: Defensive model cloning with graceful fallback.The OptimizeForInference method implements defensive cloning: it only clones when layer rewrites may occur (line 107), gracefully handles clone failures by returning the original model (lines 114-124), and records diagnostics for troubleshooting. This prevents unintended mutations of user models while enabling optimizations when safe.
276-340: LGTM: Paged KV-cache initialization with graceful fallback.The InitializePagedKVCache method properly discovers paged attention layers, creates the paged cache infrastructure, uses bounded retry for sequence allocation (line 315), and gracefully handles allocation failures by recording diagnostics and cleaning up state (lines 317-326). The fallback to non-paged cache (line 294) ensures robustness.
342-371: LGTM: Bounded retry prevents infinite loops.The TryAllocatePagedSequenceId methods implement bounded retry with maxAttempts=1024 (line 344) and use SpinWait for backoff (lines 345, 355), addressing previous review feedback about infinite loop risk. The preferred-id variant (lines 362-371) correctly falls back to auto-allocation if the preferred ID is unavailable.
391-538: LGTM: Comprehensive attention layer rewrites.The ApplyAttentionOptimizations method systematically rewrites attention layers based on configuration, handling SelfAttention → MultiHead → Paged/Cached/Flash conversions. The decrement-and-continue pattern (lines 419-420) after SelfAttention conversion ensures the newly converted layer is re-evaluated under MultiHeadAttention rules, which is intentional and correct despite being slightly clever.
540-577: LGTM: Safe weight-only quantization with fallback.The ApplyWeightOnlyQuantization method correctly gates quantization by configuration and type constraints (T==float), uses try/catch to handle conversion failures gracefully (lines 559-571), and records diagnostics for both success and failure paths. This ensures quantization doesn't break inference when conversion fails.
676-707: LGTM: Memory-aware sequence length estimation.The EstimateMaxSequenceLength method correctly computes memory requirements per token (accounting for K/V, layers, heads, and batch size), calculates the maximum sequence length that fits within the memory budget, and applies reasonable bounds [128, 32768] to prevent extreme values. The implementation is sound.
854-905: LGTM: Cache clearing with bounded retry and graceful degradation.The ClearCache method correctly clears the non-paged cache (line 856) and handles paged cache with best-effort cleanup (lines 859-866). It uses bounded retry via TryAllocatePagedSequenceId (line 869), addressing previous infinite loop concerns, and gracefully degrades to disabled inference mode if re-allocation fails (lines 878-886) rather than crashing.
src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs (1)
33-33: Verify that internalizingSpeculativeDecoder<T>is intentional.Changing this class from
publictointernalis a breaking change for any external consumers who were directly instantiating or referencingSpeculativeDecoder<T>. If this class was previously part of the public API, consider:
- Documenting this as a breaking change in release notes
- Providing a migration path or alternative public API
- Confirming that this aligns with the project's API stability guarantees
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/PR433_PHASE_AUDIT.md (2)
5-5: Convert emphasis-based subsection markers to proper markdown headings.The document uses bold emphasis (
**text**) for subsection labels throughout, which violates markdown linting rules (MD036) and reduces document structure clarity. Convert all such subsection markers to proper markdown headings using###.Apply this pattern to resolve all 33 MD036 violations:
-**Audit basis** +### Audit basisExamples of required conversions:
-**Audit basis** +### Audit basis -**Current confidence summary** +### Current confidence summary -**Implemented evidence** +### Implemented evidence -**Existing verification** +### Existing verification -**Status:** Closed for MVP. +### Status +Closed for MVP. -**Verification added** +### Verification added -**Phase intent:** ... +### Phase intent +... -**Phase 0 — Baseline Safety & Diagnostics** +## Phase 0 — Baseline Safety & DiagnosticsNote: Use
##for top-level phases (e.g., Phase 0–9) and###for subsections (e.g., Audit basis, Implemented evidence, Status).Also applies to: 25-25, 31-31, 38-38, 50-50, 57-57, 67-67, 78-78, 86-86, 94-94, 104-104, 113-113, 122-122, 132-132, 140-140, 150-150, 160-160, 166-166, 180-180, 187-187, 194-194, 204-204, 211-211, 224-224, 233-233, 241-241, 250-250, 257-257, 266-266, 274-274, 346-346, 349-349, 356-356
333-336: Vary introductory phrasing in the P1 recommendations list.Items 4–7 in the P1 section all begin with "Add," creating repetitive sentence structure. Vary the phrasing for readability.
Example rewrite:
4) Add integration test for paged selection (Phase 2) to prove the optimizer chooses `PagedCachedMultiHeadAttention` when enabled. -5) Add integration test for FP16 auto selection (Phase 3). -6) Add concurrency test for sessions (Phase 4). -7) Add adapter/task-switch KV reset test (Phase 9). +5) Verify FP16 auto selection via integration test (Phase 3). +6) Validate concurrent multi-sequence session behavior (Phase 4). +7) Test adapter/task-switch KV reset mechanics (Phase 9).tests/AiDotNet.Tests/UnitTests/Helpers/InferenceDiagnosticsTests.cs (1)
9-48: Consider expanding test coverage (optional).The current tests cover the core enable/disable behavior effectively. For more comprehensive coverage, consider adding tests for:
RecordExceptionmethod- Environment variable value "true" (case-insensitive)
- Multiple diagnostic entries in a single snapshot
- Boundary behavior at
MaxEntries(1024)These are nice-to-have improvements that can be deferred if the current coverage meets your MVP needs.
tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (1)
100-114: Consider usingTryGetValuefor statistics access to avoid potential runtime exceptions.Direct indexer access on the statistics dictionary (e.g.,
statsAfterFirst["KVCache_SequenceLengths"]) will throwKeyNotFoundExceptionif the key is missing, which could make test failures harder to diagnose. UsingTryGetValuewith an assertion would provide clearer failure messages.- var statsAfterFirst = seqA.GetInferenceStatistics(); - var lengthsAfterFirst = (int[])statsAfterFirst["KVCache_SequenceLengths"]; - int lenAfterFirst = lengthsAfterFirst[0]; + var statsAfterFirst = seqA.GetInferenceStatistics(); + Assert.True(statsAfterFirst.TryGetValue("KVCache_SequenceLengths", out var lengthsObj), "Expected KVCache_SequenceLengths in statistics"); + var lengthsAfterFirst = (int[])lengthsObj; + int lenAfterFirst = lengthsAfterFirst[0];tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (1)
146-167: Consider adding explicit type assertion for consistency.Unlike the
SmallNeuralUnavailabletest (line 142), this test doesn't explicitly verify the fallback type. AddingAssert.Contains("NGramDraftModel", optimizer.DraftModel!.GetType().Name)would make the fallback behavior assertion explicit.Assert.True(anyApplied); Assert.NotNull(optimizer.DraftModel); + Assert.Contains("NGramDraftModel", optimizer.DraftModel!.GetType().Name); Assert.True(optimizer.DraftModel!.VocabSize > 0);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/PR433_PHASE_AUDIT.md(1 hunks)src/Helpers/InferenceDiagnostics.cs(1 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Helpers/InferenceDiagnosticsTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Helpers/InferenceDiagnostics.cs
🧰 Additional context used
🧬 Code graph analysis (1)
tests/AiDotNet.Tests/UnitTests/Helpers/InferenceDiagnosticsTests.cs (1)
src/Helpers/InferenceDiagnostics.cs (2)
InferenceDiagnostics(10-89)RecordDecision(23-38)
🪛 LanguageTool
docs/PR433_PHASE_AUDIT.md
[style] ~336-~336: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...urrency test for sessions (Phase 4). 7) Add adapter/task-switch KV reset test (Phas...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/PR433_PHASE_AUDIT.md
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
25-25: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
31-31: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
38-38: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
50-50: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
57-57: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
67-67: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
78-78: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
86-86: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
94-94: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
104-104: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
113-113: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
122-122: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
132-132: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
140-140: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
150-150: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
160-160: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
166-166: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
180-180: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
187-187: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
194-194: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
204-204: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
211-211: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
224-224: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
233-233: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
241-241: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
250-250: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
257-257: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
266-266: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
274-274: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
346-346: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
349-349: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
356-356: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (31)
docs/PR433_PHASE_AUDIT.md (5)
1-358: Document is comprehensive, well-structured, and actionable.The phase-by-phase audit is thorough, uses concrete code evidence and test references, and provides a clear, prioritized gap-closure plan. The 95% confidence assessment is realistic and honest about Phase 8 shortfalls. Verification matrix is practical.
Recommendation: After addressing the markdown formatting issues above, consider adding a brief "How to use this document" section at the top to guide reviewers on navigating the 9 phases and gap-closure plan, especially for readers unfamiliar with the inference MVP roadmap.
292-309: P0 gap-closure items are well-specified and actionable.The three P0 requirements (Phase 7 dynamic speculation, Phase 8 quantization broadening, Phase 5 arbitration) include concrete interface sketches, algorithmic requirements (acceptance rate EMA, monotonic backoff), and test criteria. These are implementable without further clarification.
332-340: P1 and P2 recommendations are reasonable and appropriately scoped.Both sections correctly identify non-blocking improvements (integration tests, diagnostics assertions) that enhance confidence without blocking MVP delivery. Prioritization is sound.
344-358: Verification matrix is practical and well-organized.The build and test commands are accurate, filters are appropriate, and the caveat about unrelated test failures is transparent. This section enables maintainers to run targeted validation efficiently.
280-286: Unresolved PR threads are appropriately documented.The note on code-scanning alert threads is transparent and provides clear guidance on the workflow for resolving them via manual comments. Ensure these remain visible and are tracked for follow-up.
tests/AiDotNet.Tests/UnitTests/Helpers/InferenceDiagnosticsTests.cs (1)
7-48: LGTM! Test structure and cleanup pattern are solid.Both tests follow a consistent and defensive pattern: capturing the original environment variable, modifying it for the test, and restoring it in a finally block. The double Clear() (before the test and in finally) ensures thorough cleanup.
tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (14)
1-24: LGTM!The imports and constants are well-organized. The computed
FlatSizeconstant keeps dimensions consistent across test models.
25-43: LGTM!The test correctly validates that predictions are reproducible (stateless) when using the same input token.
45-76: LGTM!The test effectively validates that serialization/deserialization round-trips preserve the
InferenceOptimizationConfigproperties.
131-156: LGTM!The test correctly validates that
Reset()restores the sequence to its initial state, ensuring reproducible predictions.
190-241: LGTM!The KV-cache quantization and precision tests correctly use
TryGetValuefor statistics access and properly validate the expected storage modes.
243-267: LGTM!The test correctly verifies that speculative decoding configuration is tracked but the decoding path is not triggered during a simple
Predictcall.
269-314: LGTM!The paged KV-cache and weight-only quantization tests correctly validate the initialization and configuration flags using proper assertion patterns.
316-345: LGTM!The test effectively validates that multi-LoRA task selection is isolated per sequence, with outputs differing between tasks.
347-398: LGTM!The test thoroughly validates that switching LoRA tasks resets the KV-cache state and logs the appropriate diagnostics. The
try/finallypattern ensures proper cleanup of the environment variable and diagnostics state.
400-420: LGTM!The test correctly validates that
Clone()produces a deep copy where parameter modifications in the clone do not affect the original model.
422-453: LGTM!The helper methods are well-structured, with appropriate null checking and clean separation between model creation and result wrapping.
455-569: LGTM!The multi-LoRA model helpers use deterministic parameter initialization with intentionally different LoRA deltas (zeros for taskA, 0.05f for taskB) to ensure predictable and distinguishable outputs between tasks.
571-642: LGTM!The remaining helper methods are well-implemented.
CreateDeterministicAttentionOnlyModelprovides consistent test models, and the assertion helpers provide clear, informative failure messages.
174-180: ConcurrentPredictcalls lack synchronization on shared sequence state.The
InferenceSequence.Predictmethod (line 1202) callsoptimized.Predict(inputTensor)without holding_sequenceLock, allowing multiple threads to concurrently access_sequenceOptimizedNeuralModeland_sequenceOptimizer. While the paged KV cache uses internal locking and each sequence has its own cloned model instance, the lack of explicit synchronization at this layer obscures whether concurrent calls are safe and makes the thread-safety contract ambiguous. Document whether concurrentPredictcalls on the same sequence are supported, or add synchronization if they are not.tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (11)
1-12: LGTM!The imports and namespace declaration are appropriate for the test scope.
13-42: LGTM!The test correctly validates diagnostics recording with proper environment variable cleanup via
try/finally.
44-64: LGTM!The test correctly validates the FlashAttention rewrite path when KV-cache is disabled but FlashAttention is enabled.
66-95: LGTM!The test thoroughly validates the PagedCachedMultiHeadAttention rewrite, including verification of
InferenceModeandKernelproperties on the cached layers.
97-120: LGTM!The test correctly validates in-place rewrite semantics by verifying that both the returned model and the original model no longer contain
SelfAttentionLayer.
122-144: LGTM!The test correctly validates the fallback behavior by asserting on the actual draft model type rather than the config value. The previous review feedback has been addressed.
169-203: LGTM!The test validates both the structural transformation (DenseLayer → QuantizedDenseLayer) and functional preservation with an appropriate tolerance for quantization-induced error.
205-245: LGTM!The tests correctly validate that
AttentionLayerandGraphAttentionLayerare excluded from inference optimization rewrites, withAssert.Sameconfirming no unnecessary cloning occurred.
247-266: LGTM!The helper creates a minimal transformer suitable for testing layer rewrites without unnecessary complexity.
268-304: LGTM!The helper creates a minimal model with
SelfAttentionLayerand deterministic parameters for reproducible testing.
306-382: LGTM!The remaining helper methods appropriately create minimal models targeting specific layer types for testing optimizer skip behavior and quantization.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/Serving/ContinuousBatching/ContinuousBatcher.cs (1)
416-428: Critical: Stop condition still checked AFTER token is appended.This issue was flagged in a previous review but remains unresolved. Line 420 appends the token to the sequence, then lines 424-427 check the stop condition. If the speculative batch contains
[token1, EOS, token3], all three tokens are appended before the stop check in the calling loop (lines 263-267) breaks, leaving token3 in the sequence beyond EOS.Check the stop condition before appending:
var tokens = new List<int>(result.NewTokens.Length); for (int i = 0; i < result.NewTokens.Length; i++) { int token = result.NewTokens[i]; - sequence.AppendToken(token); - tokens.Add(token); - - // Prevent appending beyond stop conditions (e.g., EOS in the speculative batch). if (sequence.ShouldStop(_config.EosTokenId, sequence.Request.StopTokenIds)) { + // Don't append the stop token if it shouldn't be included + // (check your stop semantics) break; } + + sequence.AppendToken(token); + tokens.Add(token); }Note: Verify whether the EOS token itself should be appended before breaking, as this depends on your API semantics.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Helpers/DeserializationHelper.cs(4 hunks)src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs(9 hunks)src/Serving/ContinuousBatching/ContinuousBatcher.cs(7 hunks)tests/AiDotNet.Tests/UnitTests/Inference/SpeculativeDecodingTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/AiDotNet.Tests/UnitTests/Inference/SpeculativeDecodingTests.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (5)
src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs (1)
372-395: Well done addressing the past review feedback!The method no longer mutates
_configand now correctly adjusts the private adaptive state fields_currentDraftTokensand_currentMaxTreeDepth. The logic is also clearer and more maintainable.src/Helpers/DeserializationHelper.cs (4)
51-56: LGTM!The layerType parameter parsing logic cleanly separates type identification from parameter extraction, enabling flexible serialization formats like "MultiHeadAttentionLayer;HeadCount=8".
98-251: Refactoring successfully addresses past review feedback.The extraction of layer-specific construction logic (CreateDenseLayer, CreateMultiHeadAttentionLayer, CreateFlashAttentionLayer, etc.) significantly improves maintainability and testability compared to the previous inline implementation.
710-721: LGTM!The heuristic for resolving default head counts is pragmatic and prioritizes common divisors (8, 4, 16, 12, 6, 2) before falling back to 1.
749-767: Exception handling successfully addresses past review feedback.The updated implementation properly distinguishes between expected failures (MissingMethodException for types without parameterless constructors) and unexpected failures (other exceptions that are now wrapped and rethrown with context).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (2)
79-79: Remove unnecessary property access.The statement
_ = updated.CallCount;serves no purpose. TheAddOrUpdatemethod already executes and updates the dictionary, and the return value doesn't need to be consumed.Apply this diff to remove the unnecessary line:
- _ = updated.CallCount;
185-202: Consider makingOperationStatsimmutable.The class has mutable properties (
set;accessors), which means callers receiving objects fromGetStats()orGetAllStats()could modify them. While unlikely to cause issues with the current internal usage pattern, usinginitsetters or converting to a record type would provide stronger guarantees.Example with init-only setters:
public class OperationStats { - public string OperationName { get; set; } = string.Empty; - public long CallCount { get; set; } - public long TotalTicks { get; set; } - public long MinTicks { get; set; } - public long MaxTicks { get; set; } - public long TotalMemoryBytes { get; set; } + public string OperationName { get; init; } = string.Empty; + public long CallCount { get; init; } + public long TotalTicks { get; init; } + public long MinTicks { get; init; } + public long MaxTicks { get; init; } + public long TotalMemoryBytes { get; init; }src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs (2)
316-324: Sequential processing inBatchTargetForwardmay limit tree speculation throughput.The local function is named "Batch" but processes sequences serially. Since tree speculation explores multiple paths, parallel verification could significantly improve performance.
Consider parallelizing the batch verification:
List<Matrix<T>> BatchTargetForward(List<Vector<int>> sequences) { - var results = new List<Matrix<T>>(sequences.Count); - for (int i = 0; i < sequences.Count; i++) - { - results.Add(_targetForward(sequences[i])); - } - return results; + var results = new Matrix<T>[sequences.Count]; + Parallel.For(0, sequences.Count, i => + { + results[i] = _targetForward(sequences[i]); + }); + return results.ToList(); }If
_targetForwardis not thread-safe, document this limitation or consider exposing a batch API from the target model.
400-407: Sync-over-async wrapper is acceptable but has known limitations.The
.GetAwaiter().GetResult()pattern can deadlock in contexts with synchronization contexts (UI threads, classic ASP.NET). Since this appears intentional for API convenience and is documented, consider adding a remark noting this limitation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs(1 hunks)src/Helpers/DeserializationHelper.cs(4 hunks)src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs(9 hunks)src/Serving/ContinuousBatching/ContinuousBatcher.cs(7 hunks)tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs(1 hunks)tests/AiDotNet.Tests/TestInfrastructure/DiagnosticsEnvironmentCollection.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Helpers/InferenceDiagnosticsTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/AiDotNet.Tests/UnitTests/Helpers/InferenceDiagnosticsTests.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
🔇 Additional comments (32)
src/Inference/SpeculativeDecoding/SpeculativeDecoder.cs (8)
33-54: LGTM on class internalization and new state fields.The visibility change to
internalis appropriate for encapsulating this implementation detail. The new fields properly separate immutable configuration maximums (_maxDraftTokens,_maxTreeDepth) from mutable adaptive state (_currentDraftTokens,_currentMaxTreeDepth), which addresses the earlier concern about config mutation.
64-66: LGTM - AcceptanceRate logic now correctly branches on mode.The property now properly checks
UseTreeSpeculationfirst before evaluating counters, ensuring the correct rate is returned regardless of counter history.
75-96: LGTM on internal diagnostic properties.These properties provide useful visibility into adaptive state for testing and debugging while keeping them internal to avoid exposing implementation details publicly.
113-117: LGTM on constructor initialization.Defensive bounds with
Math.Max(1, ...)ensure valid minimums, and initializing current values to their maximums provides correct starting state for adaptive draft length.
136-139: LGTM on GenerateAsync branching and adaptive draft usage.The tree speculation branch is cleanly separated, and using
_currentDraftTokensinstead of the config value properly enables adaptive draft length.Also applies to: 155-155
281-285: LGTM on adaptive adjustment invocation.Correctly placed after statistics are accumulated each round, maintaining parity with the tree speculation path.
372-395: LGTM on adaptive draft length logic - addresses prior config mutation concern.The implementation correctly uses private fields instead of mutating config. The hysteresis pattern (shrink below threshold, grow at threshold + 0.2) prevents oscillation.
Minor observation: The magic numbers
8and0.2could be named constants for self-documentation:private const int MinSamplesForAdaptation = 8; private const double AcceptanceRateHysteresis = 0.2;
418-422: LGTM on ResetStatistics enhancements.Correctly resets both tree-speculation counters and restores adaptive draft lengths to their initial values, ensuring a clean slate for subsequent generation sessions.
tests/AiDotNet.Tests/TestInfrastructure/DiagnosticsEnvironmentCollection.cs (2)
7-10: LGTM! Correct use of DisableParallelization for environment variable manipulation.The collection definition correctly disables parallelization, which is essential when manipulating process-wide environment variables to prevent race conditions between tests.
12-26: I was unable to access the repository to verify the concerns in the original review comment. The repository clone failed due to infrastructure issues, and without examining the codebase, I cannot confirm:
- Whether
InferenceDiagnostics.Clear()reads from theAIDOTNET_DIAGNOSTICSenvironment variable during cleanup- Whether other test collections manipulate
AIDOTNET_DIAGNOSTICSwithout proper isolation- Whether the collection class is configured with
DisableParallelization = trueThe review comment raises reasonable concerns about cleanup order and test isolation that require code inspection to verify. The suggested fixes appear sound based on xUnit best practices, but I cannot confirm their necessity without examining the actual implementation details.
src/Helpers/DeserializationHelper.cs (5)
51-56: LGTM! Useful enhancement for embedded metadata.The ability to parse constructor metadata directly from the layerType string (e.g.,
"MultiHeadAttentionLayer;HeadCount=8") simplifies deserialization and makes the format more self-contained.
98-313: Refactoring looks solid.The extraction of layer-specific construction logic into helper methods (CreateDenseLayer, CreateMultiHeadAttentionLayer, etc.) addresses the previous maintainability concerns. The default case now properly throws an exception when no suitable constructor is found, eliminating the fragile fallback behavior.
632-716: Excellent improvements to defensive coding.The helper methods now properly guard against null returns from
ToString()using the?? string.Emptypattern (lines 640, 652, 664, 680), and the exception handler inTryCreateActivationInstancelogs unexpected errors before returning null (line 713). These changes address the concerns raised in previous reviews.
757-775: Exception handling properly refined.The method now distinguishes between expected failures (missing parameterless constructor) that return null, and unexpected errors that are rethrown with context. This makes debugging significantly easier and addresses the concern from the previous review.
380-403: Verify the hardcoded layerIndex=0 in CreateCachedMultiHeadAttention.Line 402 passes a hardcoded
0for thelayerIndexparameter. If this index is used to identify or manage per-layer state (such as KV cache buffers in multi-layer transformers), hardcoding it could cause cache collisions or incorrect behavior. Confirm whetherlayerIndexis critical for functionality or whether the deserialization context makes it acceptable. If critical, consider whether the layer index can be supplied viaadditionalParamsor if this limitation should be documented.tests/AiDotNet.Tests/IntegrationTests/Inference/InferenceSessionIntegrationTests.cs (4)
1-44: LGTM - well-structured integration test foundation.The test class is well-organized with clear constants, proper test isolation via
Collectionattribute, and a good first test case validating stateless prediction behavior. The helper methods provide deterministic model construction for reproducible tests.
46-77: Good serialization round-trip test pattern.The test correctly verifies that
Deserializeoverwrites the loaded instance's configuration with the serialized values, ensuring config persistence works as expected.
348-399: Good diagnostic instrumentation test with proper cleanup.The test correctly validates that MultiLoRA task switches trigger KV-cache resets and are properly instrumented via
InferenceDiagnostics. The try/finally pattern ensures environment variables are restored regardless of test outcome.
159-189: Clarify the test intent for concurrent sequence access.The test creates 20 concurrent tasks that each call
Predict()on eitherseqAorseqBbased oni % 2, resulting in 10 concurrent calls per sequence. If the intent is to verify thread safety of concurrent access to different sequences from the same session, ensure each task uses its own sequence. If testing concurrent access to the same sequence, update the test name or add a comment clarifying this is intentional and thatPredict()is thread-safe per sequence.tests/AiDotNet.Tests/UnitTests/Inference/InferenceOptimizerTests.cs (5)
14-43: Good diagnostics test with proper environment cleanup.The test correctly validates that
InferenceDiagnosticsrecords decisions when the environment variable is enabled, with proper try/finally cleanup to restore the original state.
123-168: Fallback tests correctly verify graceful degradation.These tests properly validate that when requested draft model types are unavailable (SmallNeural, Custom), the optimizer falls back to NGram without throwing. The type-name check is acceptable for MVP validation.
170-204: WOQ test correctly validates quantization with clone semantics.The test properly verifies that:
- Cloning preserves the original model's layers
- The optimized model contains quantized layers
- Output values remain within acceptable tolerance (0.1 for quantized inference)
206-246: Good negative tests for unsupported layer types.These tests correctly verify that
AttentionLayerandGraphAttentionLayer(non-transformer attention variants) are not rewritten by the inference optimizer, returning the original model unchanged.
248-383: Well-designed minimal test model helpers.The helper methods create purpose-built minimal models for each test scenario with deterministic parameter initialization. This ensures reproducible, fast-running unit tests.
src/Serving/ContinuousBatching/ContinuousBatcher.cs (8)
38-53: Good internal visibility and speculation state management.The class correctly uses
internalvisibility and separates speculation state into dedicated fields. Thevolatilemodifier on_speculationDisabledDueToFailureensures visibility across threads, though the lock-protected lazy initialization inEnsureSpeculativeDecoderprovides the primary thread safety.
240-274: Correct per-token stop condition handling.The loop properly checks
ShouldStopafter processing each token and breaks early when a stop condition is met. This ensures tokens beyond EOS are not processed further in the event loop.
416-431: Stop condition correctly checked per-token during speculative append.The implementation now checks
ShouldStopafter appending each token, breaking the loop before appending subsequent tokens. This correctly addresses the previous review concern about EOS handling.
640-654: Vocab detection failure handling correctly updated.The method now returns 0 on failure instead of the arbitrary 50000 fallback, and the caller properly handles this by disabling speculation. This addresses the previous review concern.
571-610: TargetForward correctly handles variable tensor shapes.The local function properly handles both 2D
[batch, vocab]and 3D[batch, seq, vocab]logit tensors, computing per-position softmax probabilities for the speculative decoder. The division-by-zero guard at line 601 is appropriate defensive coding.
433-510: Comprehensive speculation policy with adaptive backoff.The policy logic correctly handles all speculation modes with appropriate heuristics:
ThroughputFirst: Conservative, only for single-sequence batchesLatencyFirst: Aggressive, allows speculation under higher loadAuto: Adaptive with acceptance-rate-based backoffThe diagnostic instrumentation provides good observability for debugging speculation decisions.
100-114: Constructor correctly integrates draft model override.The constructor properly accepts an optional
draftModelparameter and stores it for use in speculative decoding initialization. This allows callers to provide custom draft models while maintaining backward compatibility.
521-638: Well-structured lazy initialization with proper thread safety.The
EnsureSpeculativeDecodermethod uses double-checked locking with_speculativeLockfor thread-safe lazy initialization. All failure paths properly disable speculation and record diagnostics, ensuring graceful degradation without throwing.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (1)
115-146: Method name is slightly misleading but implementation is correct.
CopyWithPrefetchsimply delegates toArray.Copywith no explicit prefetching, which is consistent with the safe/portable design. Consider renaming toCopyBlockedor updating the summary to clarify that prefetching is left to the runtime.src/InferenceOptimization/Kernels/GemmKernel.cs (1)
74-107: Consider removingAggressiveInliningfrom blocked GEMM method.
[MethodImpl(MethodImplOptions.AggressiveInlining)]on a method with 6 nested loops is unlikely to provide benefits and may increase code size. The JIT typically won't inline such large methods regardless of the hint.🔎 Suggested change
- [MethodImpl(MethodImplOptions.AggressiveInlining)] private void GemmBlocked(float[] A, float[] B, float[] C, int M, int N, int K)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (2)
384-404: Consider optimizing horizontal sum with direct lane extraction.The current implementations copy vectors to stack-allocated arrays for summation. For better performance, consider using intrinsic-based horizontal reduction:
- For Vector256: Use
Avx.ExtractVector128to split, then combine lanes- For Vector128: Use
Sse.Shuffleand direct lane accessThis avoids the memory copy overhead in performance-critical reduction operations.
🔎 Example optimized implementations
[MethodImpl(MethodImplOptions.AggressiveInlining)] private static float HorizontalSum(Vector256<float> v) { - Span<float> tmp = stackalloc float[8]; - Unsafe.WriteUnaligned(ref Unsafe.As<float, byte>(ref MemoryMarshal.GetReference(tmp)), v); - float sum = 0f; - for (int i = 0; i < tmp.Length; i++) - { - sum += tmp[i]; - } - - return sum; + // Extract high and low 128-bit lanes + Vector128<float> low = v.GetLower(); + Vector128<float> high = v.GetUpper(); + Vector128<float> sum128 = Sse.Add(low, high); + return HorizontalSum(sum128); } [MethodImpl(MethodImplOptions.AggressiveInlining)] private static float HorizontalSum(Vector128<float> v) { - Span<float> tmp = stackalloc float[4]; - Unsafe.WriteUnaligned(ref Unsafe.As<float, byte>(ref MemoryMarshal.GetReference(tmp)), v); - return tmp[0] + tmp[1] + tmp[2] + tmp[3]; + // Horizontal add: [a,b,c,d] -> [a+b, c+d, a+b, c+d] + Vector128<float> shuf = Sse.Shuffle(v, v, 0b_11_10_11_10); // [c, d, c, d] + Vector128<float> sum = Sse.Add(v, shuf); // [a+c, b+d, *, *] + Vector128<float> shuf2 = Sse.Shuffle(sum, sum, 0b_00_00_00_01); // [b+d, *, *, *] + Vector128<float> final = Sse.Add(sum, shuf2); // [a+b+c+d, *, *, *] + return final.ToScalar(); }
282-298: Observation:Expmethod lacks SIMD optimization despite high usage in critical paths.The
Expmethod currently uses only scalar evaluation viaMathF.Exp/Math.Exp, while other methods inSimdKernels.cs(VectorAdd, VectorMultiply, ReLU, Sum) all implement SIMD paths for AVX/SSE/ARM NEON. This is notable becauseExpis extensively used throughout the codebase—34 calls in autodiff gradient computations, plus widespread usage across all activation functions (Sigmoid, SoftMax, Softplus, SELU, ELU, etc.), RBF functions, loss functions, reinforcement learning agents, and time series models.While SIMD exponential implementations are complex and require careful handling of accuracy (range reduction, polynomial approximations, special values), the performance impact could be significant given the frequency of these calls in hot paths.
If profiling identifies exponential as a bottleneck, consider:
- Linking to optimized vector math libraries (Intel SVML, AMD LibM)
- Using polynomial approximations if reduced precision is acceptable
- Evaluating polynomial-based SIMD exp implementations with known accuracy characteristics
For now, the scalar fallback is safe and correct.
AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs (1)
152-159: Observation: Test data limited to [0, 1) range.The
DeterministicValuehelper generates floats in the range [0, 1), which provides deterministic and reproducible benchmarks. However, this doesn't exercise edge cases like negative values, large magnitudes, or denormal numbers that might affect SIMD performance differently.For pure throughput benchmarking, the current approach is appropriate. If you later add correctness validation or edge-case performance testing, consider expanding the data generation to include:
- Negative values
- Values across the full float range
- Special values (though likely not for benchmarks)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs(1 hunks)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs(1 hunks)src/InferenceOptimization/Kernels/AttentionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/ConvolutionKernel.cs(1 hunks)src/InferenceOptimization/Kernels/GemmKernel.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.csAiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cssrc/AiDotNet.Tensors/Engines/Simd/SimdKernels.cssrc/InferenceOptimization/Kernels/ConvolutionKernel.cssrc/InferenceOptimization/Kernels/AttentionKernel.csAiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cssrc/InferenceOptimization/Kernels/GemmKernel.cssrc/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.csAiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cssrc/AiDotNet.Tensors/Engines/Simd/SimdKernels.cssrc/InferenceOptimization/Kernels/ConvolutionKernel.cssrc/InferenceOptimization/Kernels/AttentionKernel.csAiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cssrc/InferenceOptimization/Kernels/GemmKernel.cssrc/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/InferenceOptimization/Kernels/ConvolutionKernel.cs
🧬 Code graph analysis (5)
AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs (3)
src/InferenceOptimization/Kernels/GemmKernel.cs (3)
Tensor(38-69)Tensor(149-174)GemmKernel(14-175)src/InferenceOptimization/ICustomOperator.cs (1)
Tensor(46-46)src/InferenceOptimization/OptimizationInitializer.cs (2)
OptimizationInitializer(11-107)Initialize(19-37)
src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (1)
src/InferenceOptimization/Kernels/GemmKernel.cs (3)
MethodImpl(74-107)MethodImpl(112-144)IsSupported(23-27)
src/InferenceOptimization/Kernels/AttentionKernel.cs (2)
src/InferenceOptimization/ICustomOperator.cs (3)
IsSupported(29-29)EstimatedSpeedup(35-35)Tensor(46-46)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (1)
SimdKernels(17-406)
AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs (3)
src/Models/Results/PredictionModelResult.cs (4)
AiDotNet(477-478)AiDotNet(1071-1085)AiDotNet(2309-2353)AiDotNet(3896-3924)src/InferenceOptimization/OptimizationInitializer.cs (2)
OptimizationInitializer(11-107)Initialize(19-37)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (1)
SimdKernels(17-406)
src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (3)
src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
PlatformDetector(14-211)src/InferenceOptimization/Kernels/GemmKernel.cs (1)
MethodImpl(74-107)src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs (2)
MethodImpl(62-82)MethodImpl(87-109)
🔇 Additional comments (29)
src/InferenceOptimization/Kernels/ConvolutionKernel.cs (5)
8-28: LGTM!The class metadata and platform detection logic are well-structured. The estimated speedup values are reasonable for different platform capabilities.
30-68: LGTM!The Execute method now properly implements routing logic instead of throwing an exception. Input validation and config extraction are handled correctly, with appropriate clamping of stride and padding values.
121-161: LGTM!The helper method correctly implements the convolution computation with proper index calculations and zero-padding through bounds checking.
217-253: LGTM!The depthwise convolution helper correctly implements the per-channel computation with appropriate index calculations.
321-364: LGTM!The group convolution helper correctly implements the computation with proper handling of channel grouping and index offsets.
src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (5)
1-29: LGTM! Safe/portable implementation replacing unsafe SSE intrinsics.The decision to use a safe, portable implementation and leave prefetching to the JIT/CPU (as noted in lines 27-28) is appropriate. This avoids the platform compatibility issues flagged in previous reviews regarding SSE intrinsics on ARM.
33-62: LGTM! Cache-aware tiling logic is sound.The heuristic correctly accounts for three working sets (A, B, C tiles) fitting in L1 cache, with appropriate power-of-2 alignment and minimum tile size guarantees.
67-113: LGTM! Robust input validation and correct blocked transpose.The comprehensive null checks, bounds validation, and cache-blocked algorithm are well-implemented.
148-186: LGTM! Standard Morton encoding with correct bit manipulation.The Z-order indexing implementation follows the well-established bit-interleaving pattern.
188-213: LGTM! Simple but reasonable cache miss estimation heuristic.The model provides useful relative estimates for cache behavior planning, though the fixed miss rates are approximations.
AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs (4)
1-26: LGTM! Well-structured benchmark setup.The benchmark configuration with appropriate attributes, parameterized matrix sizes, and deterministic initialization follows BenchmarkDotNet best practices.
49-56: LGTM! Deterministic value generation using well-known LCG constants.The implementation correctly avoids PRNG APIs while producing reproducible benchmark data.
58-78: LGTM! Correct naive GEMM baseline implementation.The standard triple-nested loop provides an appropriate baseline for comparison.
86-90: Benchmark computes different operation than baseline.
OptimizedGemmTransposecomputesC = A * B^Twhile the baselineNaiveGemmcomputesC = A * B. These are mathematically different operations, making direct speedup comparison potentially misleading. Consider adding a naive transpose-B baseline, or document that this benchmark demonstrates the transpose optimization rather than equivalent-operation speedup.AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs (3)
1-56: LGTM! Well-structured attention benchmark setup.The benchmark configuration with appropriate parameters and deterministic initialization follows best practices.
68-130: Indexing assumes batch size of 1 - correct for this benchmark but fragile.The naive implementation indexes Q, K, V as 2D arrays (e.g.,
_q.Data[i * FeatureDim + k]), which works because batch size is hardcoded to 1 in setup. This is acceptable for benchmarking purposes, but adding a comment clarifying this assumption would improve maintainability.
138-142: Multi-head benchmark measures different operation than baseline.
MultiHeadAttentionwith 8 heads is structurally different from the single-headNaiveAttentionbaseline. This is appropriate for demonstrating multi-head performance, but note that speedup numbers won't reflect equivalent-operation comparison.src/InferenceOptimization/Kernels/GemmKernel.cs (4)
1-36: LGTM! Well-designed kernel metadata and platform-aware speedup estimation.The kernel correctly reports capabilities and provides reasonable speedup estimates based on detected SIMD support.
38-69: LGTM! Proper input validation and strategy selection.The dimension checks and parallel threshold logic are sound.
109-144: LGTM! Thread-safe parallel GEMM with proper work distribution.Each parallel task processes a disjoint set of rows, ensuring no data races on the result matrix.
146-174: LGTM! Correct transpose-B GEMM with efficient parallel dot products.The implementation correctly computes
C = A * B^Tusing row-wise parallelization.src/InferenceOptimization/Kernels/AttentionKernel.cs (7)
1-44: LGTM! Clean implementation with thorough input validation.Previous review concerns have been addressed: the unused
_gemmKernelfield was removed, and comprehensive shape validation is now in place.
46-100: LGTM! Comprehensive validation addresses previous review concerns.The batch size, feature dimension, and mask shape validations are thorough and handle the multi-head mask broadcasting case correctly.
102-168: LGTM! Correct attention computation with addressed float comparison.The epsilon-based mask comparison (line 136) correctly addresses the previous static analysis finding. The exact
weight == 0fcheck at line 159 is acceptable since these zeros explicitly result fromexp(-infinity)in softmax.
170-214: LGTM! Numerically stable softmax with proper handling of masked positions.The implementation correctly handles negative infinity values and guards against division by zero.
216-276: LGTM! Comprehensive multi-head attention with flexible mask handling.The validation and mask broadcasting logic correctly support both per-batch and per-head mask configurations.
278-301: LGTM! Correct head-splitting reshape.The indexing correctly transforms from
[batch, seq, num_heads * dK]interleaved format to[batch * num_heads, seq, dK].
303-326: LGTM! Correct inverse reshape for head concatenation.The indexing correctly transforms back from
[batch * num_heads, seq, dK]to[batch, seq, d_model].AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs (1)
43-150: LGTM: Well-structured benchmark suite.The benchmark organization is excellent:
- Each operation category (VectorAdd, VectorMultiply, DotProduct, ReLU, Sum) has a scalar baseline and SIMD variant
- Proper use of
[Baseline = true]per category for meaningful speedup ratios[BenchmarkCategory]enables grouped reporting- Scalar implementations provide correct reference behavior
- Memory diagnostics and export formats properly configured
This will provide clear performance comparisons between scalar and SIMD paths across multiple array sizes.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
AiDotNetBenchmarkTests/InferenceOptimization/README.md (1)
180-184: Consider updating to actions/upload-artifact@v4.The CI/CD example uses
actions/upload-artifact@v3, which still works butv4is now available with improved performance and features. This is optional and v3 remains supported.🔎 Optional update
- name: Upload Results - uses: actions/upload-artifact@v3 + uses: actions/upload-artifact@v4 with: name: benchmark-results path: BenchmarkDotNet.Artifacts/results/src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs (1)
79-79: Clarify or remove the unused read ofupdated.CallCount.This line reads
updated.CallCountand discards it via_. The purpose is unclear:
- If it's to prevent a compiler warning about unused variables, the variable could be removed entirely
- If it's to force evaluation,
AddOrUpdatealready returns the result immediately- It appears to serve no functional purpose
Consider removing this line unless there's a specific reason for it.
🔎 Proposed fix
TotalMemoryBytes = existing.TotalMemoryBytes + memoryBytes }; }); - - _ = updated.CallCount; }src/AiDotNet.Serving/Controllers/InferenceController.cs (1)
168-198: Batching bypass path with safeguards looks good.The
MaxUnbatchedItems = 1000cap prevents resource exhaustion, and the logging provides good observability for debugging. The sequential processing is acceptable given the cap.Consider making
MaxUnbatchedItemsconfigurable viaServingOptionsfor deployments with different performance requirements, though the current hardcoded value is a reasonable default.src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (2)
118-146: CopyWithPrefetch is misleadingly named since it no longer prefetches.The method name suggests prefetching behavior, but the implementation is just a safe
Array.Copywrapper. Consider renaming toCopyValidatedor similar to avoid confusion, or document in the XML comment that prefetching is left to the JIT/CPU.
191-213: Cache miss estimation uses fixed heuristic percentages.The model uses hardcoded miss rates (10%, 5%, 80%) that may not reflect actual cache behavior across different access patterns and hardware. Consider documenting that this is a coarse estimation for relative comparisons only, not accurate predictions.
src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (1)
384-404: Consider SIMD shuffle for horizontal sum performance.The
HorizontalSumimplementations usestackallocand scalar loops. For performance-critical paths, SIMD shuffle/hadd instructions could be faster. However, since these are called once per kernel invocation (not per element), the impact is minimal.🔎 Example using SSE3 horizontal add (optional)
// For Vector128<float> when Sse3.IsSupported: // var shuf = Sse3.HorizontalAdd(v, v); // var sum = Sse3.HorizontalAdd(shuf, shuf); // return sum.GetElement(0);src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs (2)
62-82: Delegate call overhead may negate unrolling benefits.The
UnrollBy4andUnrollBy8methods invoke a delegate per iteration. The delegate call overhead (virtual dispatch + potential closure allocation) likely exceeds the benefit of manual unrolling. These methods are more useful as patterns for generating unrolled code rather than direct runtime use.Consider documenting this limitation or providing an alternative that accepts
Span<T>for direct element access.
126-135: Fuse allocates on each call due to params array.The
params Action<int>[]allocates a new array on each invocation. For hot paths, consider an overload accepting a pre-allocated array or using aReadOnlySpan<Action<int>>(requires caller to stackalloc or pool).src/AiDotNet.Tensors/Engines/PlatformDetector.cs (1)
79-95: Hardcoded cache sizes are acceptable for MVP.The fixed cache size estimates (32KB L1, 256KB L2, 8MB L3) provide reasonable defaults for cache-aware optimizations across modern CPUs. The unused
Architectureparameter suggests future refinement is planned, which is good forward-thinking design.For future iterations, consider detecting actual cache sizes using platform-specific APIs:
- Windows:
GetLogicalProcessorInformation- Linux:
/sys/devices/system/cpu/cpu*/cache/*orsysconf- macOS:
sysctlwithhw.cachelinesize,hw.l1icachesize, etc.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/sonarcloud.yml(1 hunks)AiDotNetBenchmarkTests/AiDotNetBenchmarkTests.csproj(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cs(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/README.md(1 hunks)AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs(1 hunks)INTEGRATION_PLAN_PR433.md(1 hunks)docs/INFERENCE_MVP_PHASES.md(1 hunks)docs/PR433_FACADE_INFERENCE_PLAN.md(1 hunks)docs/PR433_PHASE_AUDIT.md(1 hunks)docs/PR433_REVIEW_WORKFLOW.md(1 hunks)examples/JitCompiler/BasicUsageExample.cs(3 hunks)src/AiDotNet.Serving/Controllers/InferenceController.cs(2 hunks)src/AiDotNet.Serving/Models/IServableModelInferenceOptions.cs(1 hunks)src/AiDotNet.Serving/Models/ServableModelWrapper.cs(4 hunks)src/AiDotNet.Serving/Services/ModelStartupService.cs(2 hunks)src/AiDotNet.Tensors/Engines/GpuEngine.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs(1 hunks)src/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cs(1 hunks)src/AiDotNet.Tensors/Engines/PlatformDetector.cs(1 hunks)src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs(1 hunks)src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs(1 hunks)src/AiDotNet.Tensors/LinearAlgebra/VectorBase.cs(1 hunks)src/AiDotNet.csproj(1 hunks)src/Configuration/InferenceOptimizationConfig.cs(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/PR433_REVIEW_WORKFLOW.md
🚧 Files skipped from review as they are similar to previous changes (6)
- src/AiDotNet.csproj
- examples/JitCompiler/BasicUsageExample.cs
- docs/PR433_PHASE_AUDIT.md
- src/AiDotNet.Serving/Models/IServableModelInferenceOptions.cs
- src/AiDotNet.Tensors/LinearAlgebra/VectorBase.cs
- docs/INFERENCE_MVP_PHASES.md
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-19T04:08:26.895Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 0
File: :0-0
Timestamp: 2025-11-19T04:08:26.895Z
Learning: For ILGPU GPU operations in GpuEngine.cs, use standard .NET exception types (InvalidOperationException, ArgumentException, OutOfMemoryException) instead of ILGPU-specific exception types, as ILGPU exception types may be version-specific. Combine with message-based filtering using ex.Message.Contains("device") or ex.Message.Contains("accelerator") as a fallback for GPU-specific errors.
Applied to files:
src/AiDotNet.Tensors/Engines/GpuEngine.cs
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/AiDotNet.Tensors/Engines/GpuEngine.cssrc/AiDotNet.Serving/Controllers/InferenceController.csAiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cssrc/AiDotNet.Tensors/LinearAlgebra/TensorBase.cssrc/AiDotNet.Serving/Services/ModelStartupService.cssrc/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cssrc/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cssrc/AiDotNet.Tensors/Engines/Simd/SimdKernels.csAiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cssrc/AiDotNet.Tensors/Engines/PlatformDetector.csAiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cssrc/Configuration/InferenceOptimizationConfig.cssrc/AiDotNet.Serving/Models/ServableModelWrapper.cssrc/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/AiDotNet.Tensors/Engines/GpuEngine.cssrc/AiDotNet.Serving/Controllers/InferenceController.csAiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cssrc/AiDotNet.Tensors/LinearAlgebra/TensorBase.cssrc/AiDotNet.Serving/Services/ModelStartupService.cssrc/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cssrc/AiDotNet.Tensors/Engines/Optimization/PerformanceProfiler.cssrc/AiDotNet.Tensors/Engines/Simd/SimdKernels.csAiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cssrc/AiDotNet.Tensors/Engines/PlatformDetector.csAiDotNetBenchmarkTests/InferenceOptimization/GemmBenchmark.cssrc/Configuration/InferenceOptimizationConfig.cssrc/AiDotNet.Serving/Models/ServableModelWrapper.cssrc/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: The AiDotNet project uses project-level global usings (configured in AiDotNet.csproj with `<Using Include=AiDotNet.Tensors.LinearAlgebra />`), making Vector<T>, Matrix<T>, and Tensor<T> available in all files without explicit per-file using directives. Do not flag missing using directives for these types in this project.
Applied to files:
AiDotNetBenchmarkTests/AiDotNetBenchmarkTests.csproj
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
🔇 Additional comments (45)
.github/workflows/sonarcloud.yml (1)
188-190: Sequential test execution may hide failures; consolidate test commands or split into separate steps with proper failure handling.When
dotnet testencounters a failure, it returns exit code 1, which causes the step to fail. The three test commands run sequentially in a single step, so if the first test suite fails, the subsequent test suites won't execute.Additionally, multiple OpenCover files generated from separate test projects can cause SonarCloud to incorrectly sum conditional coverage instead of merging it. Consider merging coverage files before analysis or running the test commands in separate steps with proper failure tracking using
if: always()conditions to ensure all test results are captured while maintaining proper failure reporting.src/AiDotNet.Tensors/LinearAlgebra/TensorBase.cs (1)
58-66: Verify Data property implementation and thread safety implications.This new property enables high-performance optimizations but exposes mutable internal state publicly. Before approval, the developer should clarify:
Thread safety: Confirm whether
TensorBase<T>instances are shared across threads. If so, explain how concurrent access to the returned array is safe, or document the requirement for exclusive access.API justification: Clarify why direct array access is necessary instead of making
AsWritableSpan()public or creating a public span-returning method. What specific performance scenarios requireT[]overSpan<T>?Documentation: Update XML comments to explicitly state thread safety requirements and the intended use cases (e.g., interop with SIMD intrinsics).
AiDotNetBenchmarkTests/AiDotNetBenchmarkTests.csproj (1)
9-9: LGTM: Enabling unsafe code for SIMD benchmarks.Adding
AllowUnsafeBlocksis appropriate for this benchmark project. Performance-critical SIMD code paths and low-level intrinsics commonly require unsafe code blocks. This aligns with the PR's objectives for hardware-specific optimizations and is safe for a test/benchmark project.src/AiDotNet.Tensors/Engines/GpuEngine.cs (1)
1051-1052: LGTM! Correct pattern for enabling ILGPU algorithms.The context creation using
builder.Default().EnableAlgorithms()is the proper way to enable ILGPU's mathematical algorithm extensions (includingRoundToEven). This change aligns with the PR's goal of enabling optimized kernel operations.INTEGRATION_PLAN_PR433.md (1)
1-257: Well-structured integration plan document.The document provides a clear, actionable blueprint for integrating PR #433 optimizations into the existing architecture. The file disposition matrix, phased implementation order, and build error categorization are helpful for execution.
Minor note: Line 206 states "Custom Operators | REMOVED | Not needed with IEngine" but the file disposition matrix (Lines 181-183) indicates
CustomOperatorRegistry.csandICustomOperator.csshould be FIX/KEEP. This creates a slight inconsistency in messaging.Consider reconciling the expected outcome for custom operators (Line 206) with the file disposition matrix to avoid confusion during implementation.
docs/PR433_FACADE_INFERENCE_PLAN.md (1)
1-658: Comprehensive facade integration plan with clear MVP sequencing.The document effectively addresses:
- Phase-to-MVP mapping (Section 9.0, Lines 539-552)
- Fallback behavior with explicit diagnostics guidance (Lines 133-136)
- KV-cache reset scope clarification (Lines 639-643)
The acceptance criteria mapping table (Lines 503-514) provides clear traceability between checklist items and implementation phases, which was a key improvement from prior feedback.
src/AiDotNet.Serving/Services/ModelStartupService.cs (2)
203-206: Clean configuration extraction with safe defaults.The null-coalescing pattern appropriately handles cases where
GetInferenceOptimizationConfigForServing()returns null or when the config properties are not set. The defaults (enableBatching = true,enableSpeculativeDecoding = false) align with the facade plan's conservative approach.
269-276: LGTM!Named arguments (
enableBatching:,enableSpeculativeDecoding:) improve readability and reduce risk of parameter ordering errors when calling the updated constructor.src/AiDotNet.Serving/Controllers/InferenceController.cs (3)
138-144: Good use of 413 Payload Too Large for oversized unbatched requests.Returning a specific HTTP 413 status code rather than a generic 400 or 500 is semantically correct and helps clients understand the nature of the error.
158-166: Improved error message for adapter-routed model lookup.The error message now correctly includes both the adapter-resolved name and the base model name when lookup fails, aiding debugging.
220-270: Adapter routing with validation and logging implemented well.The method correctly:
- Validates adapter ID length and characters
- Logs warnings for invalid adapters (addresses prior feedback)
- Uses a safe allowlist for characters (no path traversal risk)
- Falls back gracefully to base model name
src/AiDotNet.Serving/Models/ServableModelWrapper.cs (4)
11-11: Good use of explicit interface implementation.Implementing
IServableModelInferenceOptionsexplicitly keepsEnableBatchingandEnableSpeculativeDecodingoff the public API surface unless explicitly cast, maintaining a clean public interface while allowing internal serving code to access these options.
31-47: Constructor extension maintains backward compatibility.The new optional parameters with defaults (
enableBatching = true,enableSpeculativeDecoding = false) ensure existing callers continue to work without modification.
63-64: Appropriate defaults for regression models.Setting
_enableBatching = trueand_enableSpeculativeDecoding = falsemakes sense for regression models, which benefit from batch processing but don't use speculative decoding (a text generation optimization).
148-150: LGTM!Explicit interface implementation correctly exposes the inference options to the serving layer while keeping them out of the general public API.
src/AiDotNet.Tensors/Engines/Optimization/CacheOptimizer.cs (4)
10-26: LGTM - Cache block size properties and class structure.The hardcoded L1/L2/L3 block sizes are reasonable defaults for typical CPU architectures. The note at lines 27-28 appropriately explains the intentional omission of hardware prefetch intrinsics in favor of safe/portable code.
33-62: LGTM - Optimal tiling computation.The algorithm correctly derives tile sizes from L1 cache size, rounds to power-of-2 for alignment benefits, and clamps to actual matrix dimensions. The heuristic of fitting three tiles (A, B, C blocks) in L1 is sound for GEMM-style operations.
67-113: LGTM - Blocked transpose with thorough validation.Input validation covers null checks and bounds verification. The blocked transpose algorithm with a 32-element block size is a standard cache-friendly approach for matrix transposition.
151-186: LGTM - Morton encoding/decoding helpers.The bit-interleaving implementation for Z-order indexing is correct. The 16-bit input range (0x0000ffff mask) is appropriate for typical 2D indexing scenarios.
AiDotNetBenchmarkTests/InferenceOptimization/AttentionBenchmark.cs (4)
17-29: LGTM - Benchmark configuration and parameters.The BenchmarkDotNet attributes are well-configured with memory diagnostics and exporters. The parameter combinations (64/128/256 × 32/64) provide good coverage for attention benchmark scenarios.
30-56: LGTM - Setup and deterministic initialization.The
GlobalSetupproperly initializes the optimization infrastructure and creates reproducible tensor data using the deterministic value helper. Using offset seeds (1M, 2M) for K and V ensures different but reproducible values.
68-130: LGTM - Naive attention baseline implementation.The implementation correctly performs scaled dot-product attention with numerically stable softmax (max subtraction before exp). This serves as an appropriate baseline for comparing against the optimized kernel.
138-142: Verify MultiHeadAttention handles small head dimensions.With
numHeads: 8andFeatureDimbeing 32 or 64, the per-head dimension becomes 4 or 8 respectively. EnsureAttentionKernel.MultiHeadAttentionproperly applies scaled dot-product attention (scaling by √d_k) for these small head dimensions and that the benchmark remains meaningful.src/AiDotNet.Tensors/Engines/Simd/SimdKernels.cs (4)
1-9: LGTM - Conditional compilation for multi-target support.The
#if NET5_0_OR_GREATERguards correctly isolate intrinsic imports, addressing the previous review concern about net471 compatibility.
19-67: LGTM - VectorAdd implementation.The cascading SIMD fallback (AVX → SSE → AdvSimd → scalar) is well-structured. Length validation, SIMD lane calculation, and remainder handling are all correct.
119-179: LGTM - DotProduct with FMA optimization.Good use of conditional FMA (
Fma.MultiplyAdd) when available, falling back to separate multiply-add otherwise. The horizontal sum correctly aggregates the vector accumulator before processing the scalar tail.
282-298: Exp remains scalar-only as expected.Vectorized exp is complex and typically requires polynomial approximation or library calls. The scalar implementation with
MathF.Exp/Math.Expfallback is correct for now.src/AiDotNet.Tensors/Engines/Optimization/LoopOptimizer.cs (3)
15-31: LGTM - 2D tiling utility.The tile bounds computation correctly handles partial tiles at boundaries using
Math.Min. The delegate-based design provides flexibility for various tile processing operations.
173-195: LGTM - Parallel tiling with work stealing.The linearized tile index approach with
Parallel.Forcorrectly distributes work across threads. The TPL's work-stealing scheduler handles load balancing automatically.
200-216: LGTM - Optimal tile size determination.The algorithm correctly derives a power-of-2 tile size that fits two tiles in L1 cache, with appropriate bounds checking against the dimension size.
AiDotNetBenchmarkTests/InferenceOptimization/SimdBenchmark.cs (5)
17-26: LGTM - Benchmark configuration and grouping.The
GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)combined with per-category[Benchmark(Baseline = true)]attributes ensures meaningful speedup ratios within each operation type.
27-41: LGTM - Setup and deterministic initialization.The setup correctly initializes the optimization infrastructure and uses deterministic values for reproducible benchmark results.
45-60: LGTM - VectorAdd benchmarks.The scalar baseline provides a clear comparison point. The SIMD version correctly delegates to
SimdKernels.VectorAddusing span-based API (no unsafe code needed).
87-104: LGTM - DotProduct benchmarks.Both scalar and SIMD implementations return
float, allowing BenchmarkDotNet to verify result consistency and prevent dead-code elimination.
131-148: LGTM - Sum reduction benchmarks.The sum benchmarks correctly return the computed value to prevent optimization elimination. The baseline and SIMD paths are properly categorized.
src/AiDotNet.Tensors/Engines/PlatformDetector.cs (4)
3-6: LGTM! Conditional compilation properly guards intrinsics namespaces.The
#if NET5_0_OR_GREATERdirective correctly prevents build failures on .NET Framework 4.7.1 where these intrinsics APIs are unavailable.
105-152: LGTM! CUDA detection properly improved.The implementation now correctly checks for CUDA driver library presence using
NativeLibrary.TryLoad, addressing the previous concern that it only checked platform characteristics. The documentation clearly states the method's scope and limitations, and the handle is properly freed to prevent resource leaks.
164-210: LGTM! Capabilities description is well-structured.The method provides comprehensive, human-readable diagnostic output for all detected capabilities, which will be valuable for debugging and performance tuning.
217-283: LGTM! PlatformCapabilities class is well-designed.The class provides a clean, comprehensive API for accessing detected platform capabilities. The
GetBestSimdSet()helper method is particularly useful for automatic SIMD path selection.src/Configuration/InferenceOptimizationConfig.cs (6)
119-186: LGTM! KV-cache configuration is industry-aligned and well-documented.The new KV-cache settings cover modern serving techniques (sliding window, FP16 precision, Int8 quantization, paged allocation) with sensible defaults and excellent documentation. The beginner-friendly explanations are particularly helpful for users unfamiliar with these optimization techniques.
189-209: LGTM! Attention configuration provides sensible defaults.Flash Attention and automatic masking mode detection align with modern transformer optimization practices. The Auto mode for masking is particularly useful for supporting both autoregressive and bidirectional models.
346-356: LGTM! Validation logic is thorough and consistent.The new validation rules for KV-cache window size and paged block size follow the established pattern and provide clear error messages.
442-463: LGTM! Speculation configuration is flexible and well-designed.The separation of policy (when to speculate) and method (how to speculate) provides appropriate control granularity. The Auto defaults enable intelligent runtime adaptation while allowing explicit overrides when needed.
468-484: LGTM! Weight-only quantization is appropriately conservative.The weight-only INT8 quantization feature is well-documented with a conservative default. The note about opportunistic application and safe fallback indicates proper risk management for this advanced feature.
489-629: LGTM! Enum definitions are comprehensive and well-documented.All new enums provide clear, well-documented options with sensible defaults. The inclusion of Medusa and Eagle speculative methods as forward-looking hooks demonstrates good API design for future extensibility.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/AiDotNet.Tensors/Engines/PlatformDetector.cs (2)
79-95: Remove unusedarchparameter from cache estimation methods.The
Architecture archparameter is declared but never used inEstimateL1CacheSize,EstimateL2CacheSize, andEstimateL3CacheSize. Either remove the parameter if architecture-specific estimation is not planned, or document that it's reserved for future enhancement.🔎 Proposed fix
-private static int EstimateL1CacheSize(Architecture arch) +private static int EstimateL1CacheSize() { // Typical L1 cache size is 32KB per core return 32 * 1024; } -private static int EstimateL2CacheSize(Architecture arch) +private static int EstimateL2CacheSize() { // Typical L2 cache size is 256KB per core return 256 * 1024; } -private static int EstimateL3CacheSize(Architecture arch) +private static int EstimateL3CacheSize() { // Typical L3 cache size is 2-8MB shared return 8 * 1024 * 1024; }And update the call sites on lines 68-70:
-caps.L1CacheSize = EstimateL1CacheSize(caps.Architecture); -caps.L2CacheSize = EstimateL2CacheSize(caps.Architecture); -caps.L3CacheSize = EstimateL3CacheSize(caps.Architecture); +caps.L1CacheSize = EstimateL1CacheSize(); +caps.L2CacheSize = EstimateL2CacheSize(); +caps.L3CacheSize = EstimateL3CacheSize();
217-256: Consider usinginitaccessors for immutability.The
PlatformCapabilitiesproperties use public setters, allowing modification after detection. Since these capabilities should remain constant after initial detection, consider usinginitaccessors (C# 9.0+) orinternal setto prevent accidental mutation and improve thread safety.🔎 Example with init accessors
public class PlatformCapabilities { // Basic platform info - public Architecture Architecture { get; set; } - public string OSDescription { get; set; } = string.Empty; - public string FrameworkDescription { get; set; } = string.Empty; - public int ProcessorCount { get; set; } - public bool Is64BitProcess { get; set; } - public bool Is64BitOperatingSystem { get; set; } + public Architecture Architecture { get; init; } + public string OSDescription { get; init; } = string.Empty; + public string FrameworkDescription { get; init; } = string.Empty; + public int ProcessorCount { get; init; } + public bool Is64BitProcess { get; init; } + public bool Is64BitOperatingSystem { get; init; } // (apply same pattern to remaining properties...)tests/AiDotNet.Tensors.Tests/Engines/PlatformDetectorTests.cs (1)
6-20: Consider adding tests for helper methods.While the current test validates core detection, consider adding tests for:
GetCapabilitiesDescription()returns a non-empty stringGetBestSimdSet()returns a valid value- Property consistency (e.g., if
HasAVX2is true,HasAVXshould also be true)These additions would improve coverage without requiring specific hardware.
💡 Example additional tests
[Fact] public void GetCapabilitiesDescription_ReturnsValidString() { var description = PlatformDetector.GetCapabilitiesDescription(); Assert.NotNull(description); Assert.NotEmpty(description); Assert.Contains("Platform:", description); Assert.Contains("Architecture:", description); } [Fact] public void GetBestSimdSet_ReturnsValidValue() { var caps = PlatformDetector.Capabilities; var simdSet = caps.GetBestSimdSet(); Assert.NotNull(simdSet); Assert.NotEmpty(simdSet); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/AiDotNet.Tensors/Engines/PlatformDetector.cs(1 hunks)tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs(1 hunks)tests/AiDotNet.Tensors.Tests/Engines/PlatformDetectorTests.cs(1 hunks)tests/AiDotNet.Tests/UnitTests/Helpers/DeserializationHelperTests.cs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cstests/AiDotNet.Tensors.Tests/Engines/PlatformDetectorTests.cstests/AiDotNet.Tests/UnitTests/Helpers/DeserializationHelperTests.cssrc/AiDotNet.Tensors/Engines/PlatformDetector.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cstests/AiDotNet.Tensors.Tests/Engines/PlatformDetectorTests.cstests/AiDotNet.Tests/UnitTests/Helpers/DeserializationHelperTests.cssrc/AiDotNet.Tensors/Engines/PlatformDetector.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: SonarCloud Analysis
- GitHub Check: CodeQL Analysis
🔇 Additional comments (17)
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs (2)
1-7: LGTM!The using directives and test class declaration are appropriate and follow conventions.
27-40: Test logic is correct; same isolation concern applies.The test correctly verifies that when disabled, the profiler returns null stats. However, the same singleton state mutation issue from the previous test applies here—consider adding cleanup for
Enabled = falseto prevent interference with other tests.src/AiDotNet.Tensors/Engines/PlatformDetector.cs (3)
1-7: LGTM! Conditional compilation properly guards intrinsics.The conditional compilation directives correctly guard the intrinsics namespaces for .NET 5.0+, ensuring compatibility with the net471 target. This addresses the previous review concern.
16-22: LGTM! Proper lazy initialization pattern.The use of
Lazy<T>ensures thread-safe, one-time initialization of platform capabilities. This is an appropriate pattern for this expensive detection operation.
97-136: LGTM! CUDA detection significantly improved.The
DetectCudaSupport()method now performs actual CUDA driver library detection viaNativeLibrary.TryLoad, checking fornvcuda.dllon Windows andlibcuda.so.1/libcuda.soon Linux. The documentation clearly states the limitations. This addresses the previous review concern about misleading detection.tests/AiDotNet.Tensors.Tests/Engines/PlatformDetectorTests.cs (1)
8-19: LGTM! Basic smoke test validates core functionality.The test verifies that
PlatformDetector.Capabilitiesinitializes without throwing and returns sensible values for processor count, OS/framework descriptions, and cache sizes. While additional tests for SIMD detection andGetBestSimdSet()would be beneficial, this provides adequate baseline coverage for the detection infrastructure.tests/AiDotNet.Tests/UnitTests/Helpers/DeserializationHelperTests.cs (11)
6-8: LGTM! Necessary using directives for expanded test coverage.The new using directives appropriately support the expanded test suite covering attention layers, LoRA adapters, and inference-related types introduced in this PR.
396-400: LGTM! Well-designed test helper class.The
NoDefaultCtorDisposablehelper class correctly implements the scenario for testing deserialization behavior when a type lacks a parameterless constructor.
381-394: LGTM! Comprehensive negative test case.This test correctly validates that
DeserializeInterfacethrows anInvalidOperationExceptionwhen attempting to deserialize a type that doesn't implement the requested interface.
402-417: LGTM! Validates graceful handling of missing parameterless constructor.This test correctly verifies that
DeserializeInterfacereturnsnullwhen the target type lacks a parameterless constructor, demonstrating appropriate error handling.
419-442: LGTM! Comprehensive coverage of attention layer variants.This test effectively validates layer creation for multiple attention mechanisms (multi-head, self-attention, flash attention, cached variants), which aligns well with the PR's inference-optimization objectives. The test also validates parameter passing for
FlashAttentionLayerwithUseCausalMask.
444-457: LGTM! Clean test for AttentionLayer creation.This test validates
AttentionLayercreation with appropriate assertions and follows the established test pattern.
459-473: LGTM! Validates GraphAttentionLayer with parameter passing.This test correctly verifies
GraphAttentionLayercreation with theNumHeadsparameter, ensuring parameterized layer construction works properly.
475-488: LGTM! Validates DropoutLayer and LayerNormalizationLayer creation.This test correctly verifies the creation of
DropoutLayerandLayerNormalizationLayer, both commonly used in modern neural network architectures for regularization and normalization.
490-503: LGTM! Tests PositionalEncodingLayer with appropriate dimensions.This test validates
PositionalEncodingLayercreation with realistic dimensions (128 sequence length, 16 embedding dimension), which is essential for transformer-based models.
505-521: LGTM! Tests encoded parameter handling in type identifiers.This test validates that
DeserializationHelpercan correctly parse type identifiers with embedded parameters using semicolon-delimited format (TypeName;Param=Value), which is useful for compact serialization.
523-544: LGTM! Validates MultiLoRAAdapter with complex parameter encoding.This test correctly verifies
MultiLoRAAdaptercreation with pipe-delimited array parameters and URL-encoded strings, demonstrating proper handling of complex multi-task LoRA configurations. This aligns well with the PR's inference-optimization goals.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs (1)
38-62: Consider renaming for clarity.The test implementation is solid and follows the same excellent isolation pattern as the first test. However, the name
Profile_WhenDisabled_ReturnsEmptyDisposableimplies verification of the disposable's properties, while the test actually verifies that no stats are recorded.Consider renaming to better reflect what's being tested, such as:
Profile_WhenDisabled_DoesNotRecordStatsProfile_WhenDisabled_RecordsNoStats
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs
🧬 Code graph analysis (1)
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs (1)
src/Diagnostics/ProfilerScope.cs (1)
Profile(129-135)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Codacy Security Scan
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
🔇 Additional comments (1)
tests/AiDotNet.Tensors.Tests/Engines/Optimization/PerformanceProfilerTests.cs (1)
8-36: Excellent test isolation improvements!All concerns from the previous review have been successfully addressed:
- Unique operation names with GUID prevent cross-test interference
- State capture/restore pattern ensures no side effects on other tests
- Try-finally blocks guarantee cleanup even on test failure
- Assertion tightened to
Assert.Equal(1, stats.CallCount)for precise verificationThe test now follows best practices for singleton state management in unit tests.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
tests/AiDotNet.Tensors.Tests/Engines/Simd/SimdKernelsTests.cs (1)
7-99: Well-structured test suite with correct validation logic.The test methods correctly validate each SimdKernels operation against expected scalar results. The tolerance handling in the
Exptest is appropriate for SIMD approximations.Consider enhancing the test coverage with additional edge cases to improve robustness:
- Array boundary conditions (empty arrays, single elements)
- Error handling paths (null inputs, mismatched lengths for binary operations)
- Special float values (NaN, Infinity, -Infinity) to ensure graceful handling
- Larger arrays with sizes that test SIMD vector alignment edge cases (e.g., lengths like 16, 17, 31, 32, 33)
📋 Example edge case tests to consider
[Fact] public void VectorAdd_EmptyArrays_ReturnsEmpty() { var a = Array.Empty<float>(); var b = Array.Empty<float>(); var result = Array.Empty<float>(); SimdKernels.VectorAdd(a, b, result); Assert.Empty(result); } [Fact] public void VectorAdd_MismatchedLengths_ThrowsOrHandlesGracefully() { var a = new float[] { 1, 2, 3 }; var b = new float[] { 1, 2 }; var result = new float[3]; // Verify expected behavior - either throws or handles gracefully // Replace with actual expected behavior Assert.Throws<ArgumentException>(() => SimdKernels.VectorAdd(a, b, result)); } [Theory] [InlineData(16)] // Typical SIMD vector size boundary [InlineData(17)] // Just over boundary [InlineData(31)] [InlineData(32)] [InlineData(33)] public void VectorAdd_VariousSizes_ProducesCorrectResults(int size) { var a = Enumerable.Range(0, size).Select(i => (float)i).ToArray(); var b = Enumerable.Range(0, size).Select(i => (float)(i * 2)).ToArray(); var result = new float[size]; SimdKernels.VectorAdd(a, b, result); for (int i = 0; i < size; i++) { Assert.Equal(a[i] + b[i], result[i]); } } [Fact] public void DotProduct_WithNaN_HandlesCorrectly() { var a = new float[] { 1, float.NaN, 3 }; var b = new float[] { 1, 2, 3 }; float result = SimdKernels.DotProduct(a, b); Assert.True(float.IsNaN(result)); }tests/AiDotNet.Tensors.Tests/Engines/Optimization/LoopOptimizerTests.cs (4)
10-30: Consider adding edge case coverage for Tile2D.The test validates basic functionality well, but consider adding test cases for:
- Dimensions smaller than tile size (e.g.,
rows=2, cols=2, tileSize=4)- Minimum values (e.g.,
dimension=1, tileSize=1)- Verifying that
iEndandjEnddon't exceedrowsandcolsrespectivelyThese additions would strengthen confidence in boundary handling.
32-52: Consider verifying indices in unroll tests.Both
UnrollBy4andUnrollBy8tests count invocations but don't verify that the correct indices are passed to the action. Consider collecting the indices and asserting they cover the full range[0, length)without gaps or duplicates.Example enhancement for UnrollBy4
[Fact] public void UnrollBy4_InvokesActionForAllIndices() { const int length = 17; - int seen = 0; + var seenIndices = new HashSet<int>(); - LoopOptimizer.UnrollBy4(length, _ => seen++); + LoopOptimizer.UnrollBy4(length, i => seenIndices.Add(i)); - Assert.Equal(length, seen); + Assert.Equal(length, seenIndices.Count); + Assert.Equal(Enumerable.Range(0, length).ToHashSet(), seenIndices); }
94-111: Strengthen verification in ParallelTile2D test.The test counts tiles but doesn't verify uniqueness or correctness of tile coordinates. In concurrent scenarios, race conditions could cause duplicates or incorrect tile boundaries. Consider verifying:
- Each tile coordinate set appears exactly once (no duplicates)
- All expected tile ranges are present
- Tile boundaries are correct and non-overlapping
Enhanced verification example
[Fact] public void ParallelTile2D_VisitsAllTiles() { int rows = 9; int cols = 9; int tileSize = 4; var tiles = new ConcurrentBag<(int, int, int, int)>(); LoopOptimizer.ParallelTile2D(rows, cols, tileSize, (iStart, iEnd, jStart, jEnd) => { tiles.Add((iStart, iEnd, jStart, jEnd)); }); int expectedTilesI = (rows + tileSize - 1) / tileSize; int expectedTilesJ = (cols + tileSize - 1) / tileSize; Assert.Equal(expectedTilesI * expectedTilesJ, tiles.Count); + + // Verify uniqueness + var uniqueTiles = tiles.ToHashSet(); + Assert.Equal(tiles.Count, uniqueTiles.Count); + + // Verify all expected tiles are present + for (int i = 0; i < expectedTilesI; i++) + { + for (int j = 0; j < expectedTilesJ; j++) + { + int iStart = i * tileSize; + int iEnd = Math.Min((i + 1) * tileSize, rows); + int jStart = j * tileSize; + int jEnd = Math.Min((j + 1) * tileSize, cols); + Assert.Contains((iStart, iEnd, jStart, jEnd), uniqueTiles); + } + } }
54-92: Optional: Enhance verification in StripMine, Fuse, and OptimalOrder2D tests.While the current tests verify basic functionality, consider these optional enhancements:
- StripMine (lines 54-64): Verify segments are non-overlapping and in order.
- Fuse (lines 66-77): Verify both actions receive the same index in each iteration.
- OptimalOrder2D (lines 79-92): Verify the actual traversal order matches the expected pattern (row-major vs column-major).
These additions would provide stronger guarantees about correctness beyond just counting invocations.
src/NeuralNetworks/NeuralNetworkBase.cs (3)
1309-1327: Minor: Redundant null check onextras.On line 1321,
extras != nullis always true whenextraCount > 0sinceextraCount = extras.Lengthon line 1317. The check is harmless but redundant.🔎 Suggested simplification
writer.Write(extraCount); - if (extraCount > 0 && extras != null) + if (extraCount > 0) { for (int i = 0; i < extras.Length; i++)
1336-1372: Consider escaping or alternative encoding for metadata values.The identifier format
TypeName;Key=Value;...could break if values contain semicolons or equals signs. WhileAssemblyQualifiedNametypically uses commas rather than semicolons, unusual type names or future metadata values could cause parsing issues during deserialization.Consider one of these approaches:
- URL-encode the values:
Uri.EscapeDataString(value)- Use a structured format like JSON for metadata
- Document this as a known limitation
🔎 Example with URL encoding
// Stable ordering for deterministic serialization. foreach (var kvp in metadata.OrderBy(k => k.Key, StringComparer.Ordinal)) { - typeName += $";{kvp.Key}={kvp.Value}"; + typeName += $";{Uri.EscapeDataString(kvp.Key)}={Uri.EscapeDataString(kvp.Value)}"; }
1441-1457: Consider logging when extra parameters are silently discarded.When
extraCount > 0but the layer doesn't implementILayerSerializationExtras<T>, the extra parameters are read and discarded silently (lines 1447-1450 execute, but lines 1452-1455 are skipped). While this provides forward compatibility, it could mask deserialization mismatches.Consider adding a debug-level log or diagnostic when extras are discarded:
🔎 Optional diagnostic
if (layer is AiDotNet.NeuralNetworks.Layers.ILayerSerializationExtras<T> extraProvider) { extraProvider.SetExtraParameters(extraParams); } + else + { + // Extras were serialized but layer doesn't support them - could indicate version mismatch + System.Diagnostics.Debug.WriteLine($"Layer {layerType} has {extraCount} extra parameters but doesn't implement ILayerSerializationExtras<T>"); + } }tests/AiDotNet.Tensors.Tests/Engines/Optimization/CacheOptimizerTests.cs (3)
9-17: Strengthen tiling assertions to verify optimization behavior.The current test only verifies that tile sizes are positive and within the matrix dimensions. Consider adding assertions that verify the tiles are reasonably sized for cache optimization, such as checking that tiles are larger than a minimum threshold (e.g., > 1) and ideally verifying they align with expected cache block sizes (L1BlockSize, L2BlockSize) exposed by CacheOptimizer.
Example of stronger assertions
var (tileM, tileN, tileK) = CacheOptimizer.ComputeOptimalTiling(m: 128, n: 256, k: 64); Assert.InRange(tileM, 1, 128); Assert.InRange(tileN, 1, 256); Assert.InRange(tileK, 1, 64); + +// Verify tiles are large enough to be effective +Assert.True(tileM > 1, "tileM should be greater than 1 for effective blocking"); +Assert.True(tileN > 1, "tileN should be greater than 1 for effective blocking"); +Assert.True(tileK > 1, "tileK should be greater than 1 for effective blocking");
19-42: Consider testing with larger matrices to exercise blocking behavior.The test correctly verifies transpose logic with a 3×4 matrix. Since
TransposeBlockedis designed for cache optimization through blocking, consider adding test cases with larger matrices (e.g., 128×128 or 256×256) to ensure the blocking logic is properly exercised. Edge cases like 1×1 matrices could also be valuable.
44-53: Consider testing with larger arrays to exercise prefetch behavior.The test verifies copy correctness but uses a small 4-element array that won't exercise the prefetch optimization. Consider adding a test case with a larger array (e.g., several thousand elements) to ensure the prefetch logic handles typical workloads correctly.
src/LoRA/Adapters/MultiLoRAAdapter.cs (2)
2-4: Verify necessity of theAiDotNet.Tensors.LinearAlgebrausing directive.Based on the project's global using directives,
Vector<T>,Matrix<T>, andTensor<T>should already be available without this explicit using. Consider removing line 3 if it's redundant.Based on learnings, the project has global usings for
AiDotNet.Tensors.*types.
705-738: Consider escaping metadata values within the type identifier.The method builds a type identifier by concatenating metadata as
;Key=Value. If base layer metadata values contain;or=characters, this could create ambiguity in the identifier format. Although the final identifier is URI-encoded when stored inGetMetadata(), escaping withinBuildLayerTypeIdentifierwould make the format more robust.Additionally, using
AssemblyQualifiedNamefor activation types can produce very long identifiers, which may impact readability in logs or debugging scenarios.Potential escaping approach
Consider escaping metadata values within the loop:
foreach (var kvp in metadata.OrderBy(k => k.Key, StringComparer.Ordinal)) { - typeName += $";{kvp.Key}={kvp.Value}"; + typeName += $";{kvp.Key}={Uri.EscapeDataString(kvp.Value)}"; }src/Inference/PagedCachedMultiHeadAttention.cs (2)
179-182: Consider optimizing redundant conversions when T is float.In the per-token hot loop,
Convert.ToSingleis called for every element (lines 179-182). WhenTis alreadyfloat, this conversion is redundant. For large embedding dimensions (e.g., 768, 1024, 4096), eliminating this overhead can improve throughput.🔎 Proposed micro-optimization
+ if (typeof(T) == typeof(float)) + { + // Fast path: direct copy when T is float. + for (int t = 0; t < seqLen; t++) + { + var inputSpan = System.Runtime.CompilerServices.Unsafe.As<Tensor<float>>(input); + for (int d = 0; d < embDim; d++) + { + hidden[d] = inputSpan[0, t, d]; + } + // ... rest of token processing ... + } + } + else + { + // Conversion required for other types. for (int t = 0; t < seqLen; t++) { for (int d = 0; d < embDim; d++) { hidden[d] = Convert.ToSingle(input[0, t, d]); } - - if (useQuantized) - { - // ... quantized path ... - } - else - { - // ... non-quantized path ... - } - - // Add bias and activation. - for (int d = 0; d < embDim; d++) - { - T value = NumOps.FromDouble(tokenOut[d]); - value = NumOps.Add(value, _outputBias[d]); - output[0, t, d] = ScalarActivation!.Activate(value); - } - - _currentPosition++; + // ... rest of token processing ... } + }Note: This adds code duplication. Alternatively, extract the token processing loop into a helper method to reduce duplication.
10-21: Document thread-safety expectations in class documentation.The class maintains mutable per-sequence state (
_currentPosition, line 37) that is updated duringForward(line 224) without synchronization. While the design implies single-threaded usage per instance (one sequence per instance), the thread-safety expectations are not explicitly documented. Clarifying this helps prevent misuse in multi-threaded inference servers.🔎 Proposed documentation addition
/// <summary> /// Multi-head attention layer backed by PagedKVCache for efficient multi-sequence inference. /// </summary> /// <remarks> /// This layer is intended for inference-time usage. When <see cref="InferenceMode"/> is enabled /// and a <see cref="Kernel"/> is attached, it uses PagedKVCache to avoid reallocations and /// allow many independent sequences to grow efficiently. /// <para> /// <b>Limitation:</b> This layer currently supports <c>batchSize == 1</c> per sequence to avoid cache mixing. /// For concurrent serving, create one sequence per request (distinct <see cref="SequenceId"/> values). /// </para> +/// <para> +/// <b>Thread-safety:</b> Instances of this class are not thread-safe. Each instance maintains +/// per-sequence state and must not be accessed concurrently from multiple threads. For concurrent +/// inference, create separate instances per sequence or use external synchronization. +/// </para> /// </remarks>
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/Inference/CachedMultiHeadAttention.cs(10 hunks)src/Inference/InferenceOptimizer.cs(13 hunks)src/Inference/PagedCachedMultiHeadAttention.cs(1 hunks)src/InferenceOptimization/ARCHITECTURE.md(1 hunks)src/LoRA/Adapters/MultiLoRAAdapter.cs(6 hunks)src/NeuralNetworks/Attention/FlashAttentionLayer.cs(2 hunks)src/NeuralNetworks/Layers/DropoutLayer.cs(1 hunks)src/NeuralNetworks/Layers/EmbeddingLayer.cs(1 hunks)src/NeuralNetworks/Layers/GraphAttentionLayer.cs(1 hunks)src/NeuralNetworks/Layers/LayerBase.cs(2 hunks)src/NeuralNetworks/Layers/LayerNormalizationLayer.cs(1 hunks)src/NeuralNetworks/Layers/MultiHeadAttentionLayer.cs(1 hunks)src/NeuralNetworks/Layers/PositionalEncodingLayer.cs(1 hunks)src/NeuralNetworks/Layers/SelfAttentionLayer.cs(1 hunks)src/NeuralNetworks/NeuralNetworkBase.cs(5 hunks)src/PredictionModelBuilder.cs(1 hunks)tests/AiDotNet.Tensors.Tests/Engines/Optimization/CacheOptimizerTests.cs(1 hunks)tests/AiDotNet.Tensors.Tests/Engines/Optimization/LoopOptimizerTests.cs(1 hunks)tests/AiDotNet.Tensors.Tests/Engines/Simd/SimdKernelsTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/NeuralNetworks/Layers/DropoutLayer.cs
- src/NeuralNetworks/Layers/SelfAttentionLayer.cs
- src/NeuralNetworks/Layers/EmbeddingLayer.cs
- src/NeuralNetworks/Layers/LayerNormalizationLayer.cs
- src/InferenceOptimization/ARCHITECTURE.md
- src/NeuralNetworks/Layers/GraphAttentionLayer.cs
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-12-18T08:49:25.295Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningMask.cs:1-102
Timestamp: 2025-12-18T08:49:25.295Z
Learning: In the AiDotNet repository, the project-level global using includes AiDotNet.Tensors.LinearAlgebra via AiDotNet.csproj. Therefore, Vector<T>, Matrix<T>, and Tensor<T> are available without per-file using directives. Do not flag missing using directives for these types in any C# files within this project. Apply this guideline broadly to all C# files (not just a single file) to avoid false positives. If a file uses a type from a different namespace not covered by the global using, flag as usual.
Applied to files:
src/NeuralNetworks/Layers/MultiHeadAttentionLayer.cstests/AiDotNet.Tensors.Tests/Engines/Optimization/LoopOptimizerTests.cssrc/PredictionModelBuilder.cssrc/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tensors.Tests/Engines/Optimization/CacheOptimizerTests.cstests/AiDotNet.Tensors.Tests/Engines/Simd/SimdKernelsTests.cssrc/Inference/CachedMultiHeadAttention.cssrc/Inference/PagedCachedMultiHeadAttention.cssrc/NeuralNetworks/Layers/PositionalEncodingLayer.cssrc/LoRA/Adapters/MultiLoRAAdapter.cssrc/NeuralNetworks/Attention/FlashAttentionLayer.cssrc/NeuralNetworks/Layers/LayerBase.cssrc/Inference/InferenceOptimizer.cs
📚 Learning: 2025-12-18T08:49:53.103Z
Learnt from: ooples
Repo: ooples/AiDotNet PR: 444
File: src/Interfaces/IPruningStrategy.cs:1-4
Timestamp: 2025-12-18T08:49:53.103Z
Learning: In this repository, global using directives are declared in AiDotNet.csproj for core namespaces (AiDotNet.Tensors.* and AiDotNet.*) and common system types. When reviewing C# files, assume these global usings are in effect; avoid adding duplicate using statements for these namespaces and for types like Vector<T>, Matrix<T>, Tensor<T>, etc. If a type is not found, verify the global usings or consider adding a file-scoped using if needed. Prefer relying on global usings to reduce boilerplate.
Applied to files:
src/NeuralNetworks/Layers/MultiHeadAttentionLayer.cstests/AiDotNet.Tensors.Tests/Engines/Optimization/LoopOptimizerTests.cssrc/PredictionModelBuilder.cssrc/NeuralNetworks/NeuralNetworkBase.cstests/AiDotNet.Tensors.Tests/Engines/Optimization/CacheOptimizerTests.cstests/AiDotNet.Tensors.Tests/Engines/Simd/SimdKernelsTests.cssrc/Inference/CachedMultiHeadAttention.cssrc/Inference/PagedCachedMultiHeadAttention.cssrc/NeuralNetworks/Layers/PositionalEncodingLayer.cssrc/LoRA/Adapters/MultiLoRAAdapter.cssrc/NeuralNetworks/Attention/FlashAttentionLayer.cssrc/NeuralNetworks/Layers/LayerBase.cssrc/Inference/InferenceOptimizer.cs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: CodeQL Analysis
- GitHub Check: SonarCloud Analysis
- GitHub Check: Codacy Security Scan
🔇 Additional comments (30)
src/NeuralNetworks/Attention/FlashAttentionLayer.cs (2)
523-530: LGTM! Metadata implementation aligns with serialization pattern.The GetMetadata() override correctly exposes the key constructor parameters (HeadCount and UseCausalMask) needed for reliable layer deserialization and round-tripping.
30-30: Visibility change requires verification of intent and impact.Reducing
FlashAttentionLayer<T>visibility frompublictointernalis a breaking change if this type was part of the public API. Verify whether this type was actually used externally or if it was intended as an internal implementation detail. If internal by design, ensure this is documented in the PR rationale.src/NeuralNetworks/Layers/MultiHeadAttentionLayer.cs (1)
480-486: LGTM! Metadata implementation follows established pattern.The GetMetadata() override correctly exposes HeadCount, which is essential for layer deserialization. The implementation is consistent with the metadata pattern used across other layers in this PR.
src/NeuralNetworks/Layers/PositionalEncodingLayer.cs (1)
433-440: LGTM! Excellent use of InvariantCulture for serialization.The GetMetadata() implementation correctly exposes MaxSequenceLength and EmbeddingSize with culture-invariant formatting. This ensures reliable deserialization across different locale settings.
src/NeuralNetworks/Layers/LayerBase.cs (2)
1384-1386: LGTM! Delegation pattern improves parameter handling consistency.The change to delegate
UpdateParameters(Vector<T>)toSetParametersensures that derived layers with structured weight/bias representations can properly materialize the flat parameter vector through their overridden SetParameters implementation. This is a cleaner design than direct assignment.
1656-1667: LGTM! Well-designed base implementation for metadata serialization hook.The internal virtual GetMetadata() method provides a clean extension point for derived layers to expose constructor-level metadata without expanding the public API surface. The use of StringComparer.Ordinal for the dictionary is appropriate for metadata keys.
src/NeuralNetworks/NeuralNetworkBase.cs (3)
1266-1273: LGTM! Backward-compatible version marker approach.Using a negative version marker to distinguish V2+ format from V1 is a solid approach for backward compatibility, since layer count is always non-negative.
1386-1399: LGTM! Version detection logic is correct.The negative marker approach correctly distinguishes V1 (where the first int is the layer count) from V2+ (where the first int is the negated version number).
1278-1279: VerifyDeserializationHelper.CreateLayerFromTypehandles the new identifier format.The layer type identifier now includes metadata (e.g.,
DenseLayer;VectorActivationType=...). EnsureCreateLayerFromTypecan parse and use this extended format, especially the semicolon-separated key-value pairs.tests/AiDotNet.Tensors.Tests/Engines/Optimization/CacheOptimizerTests.cs (1)
55-66: Verify that 16-bit coordinate limitation is intentional and documented.The test assertions mask both
xandycoordinates to 16 bits (0x0000ffff), suggesting that Morton encoding only preserves the lower 16 bits of each coordinate. This means coordinates beyond 65,535 will be silently truncated.If this limitation is intentional, ensure it's clearly documented in the
CacheOptimizer.MortonEncodeandMortonDecodemethods (e.g., via XML comments specifying the valid range). If it's an implementation constraint that could be lifted, consider whether the limitation should be documented or the implementation enhanced to support larger coordinates.src/LoRA/Adapters/MultiLoRAAdapter.cs (6)
54-54: LGTM: Interface implementation for serialization support.The addition of
ILayerSerializationExtras<T>appropriately enables handling of frozen base layer parameters during serialization.
449-461: Deterministic ordering ensures serialization correctness.The
OrderByensures parameters are serialized/deserialized in a stable order, which is essential for correctness. The performance overhead is acceptable for serialization scenarios.
492-507: LGTM: Consistent ordering with GetParameters.The identical
OrderBypattern ensures parameters are restored to the correct task adapters during deserialization.
630-649: LGTM: Critical consistency maintained for gradient updates.The gradient collection uses the same deterministic ordering as
GetParameters/SetParameters, ensuring gradients are aligned with parameters. The logic correctly zeroes gradients for non-active tasks.
652-679: LGTM: Explicit interface implementation for frozen base layer serialization.The implementation correctly handles the frozen base layer parameters as "extra" (separate from trainable task adapter parameters). The validation in
SetExtraParametersensures parameter count matches expectations.
681-703: Metadata serialization uses culture-invariant formatting and URI encoding.The implementation correctly uses
CultureInfo.InvariantCultureandUri.EscapeDataStringfor deterministic, culture-independent serialization. Note that line 694 converts Alpha todouble, which may lose precision for non-standard numeric typesT, but this is acceptable for metadata serialization.src/Inference/CachedMultiHeadAttention.cs (6)
249-256: LGTM: Query offset calculation is correct for cached inference.The query offset logic properly aligns the current query position within the full cached KV sequence, ensuring correct causal masking during incremental token generation.
For example:
- Cached KV length = 10 tokens
- New query length = 1 token
- Query offset = 9 (position in full sequence)
This enables accurate causal masking where the new token at position 9 can attend to positions 0-9.
268-270: Activation application is consistent across forward paths.Both
ForwardWithCacheandForwardStandardconsistently apply the activation function to the output projection. The implementation correctly follows the base class pattern.Note: The semantic concern about applying activation to attention output is addressed in the constructor review comment.
Also applies to: 310-312
412-424: LGTM: Activation derivative correctly applied in backward pass.The chain rule is properly implemented:
- Line 412: Applies activation derivative to output gradient
- Line 424: Uses activation gradient for output bias gradient computation
This ensures correct gradient flow through the activation function.
609-617: LGTM: Metadata implementation is appropriate for serialization.The
GetMetadata()method correctly exposes essential layer configuration (head count, Flash Attention usage, causal masking) for serialization purposes. The internal visibility aligns with the class visibility.
37-37: Verify the breaking API change: public → internal.Changing
CachedMultiHeadAttention<T>tointernalremoves it from the public API surface. If this class was previously public, external consumers will break.For a public library like AiDotNet, confirm whether:
- This class was intentionally exposed in the public API
- External code depends on it.
- A public factory method or interface exists as an alternative.
If maintaining backward compatibility is required, provide a public factory or interface wrapper instead.
131-144: Verify how the activation function is applied in the base class.Multi-head attention conventionally applies a linear output projection without activation. Confirm whether the
activationFunctionparameter is applied to the attention output or used elsewhere, and whether this design choice is intentional or represents a deviation from standard attention semantics that could impact pre-trained model compatibility.src/PredictionModelBuilder.cs (1)
1093-1093: LGTM! Consistent propagation of InferenceOptimizationConfig.The addition of
InferenceOptimizationConfigto the meta-learning result options aligns with the same propagation in the supervised (line 1029) and RL (line 1340) build paths. This ensures meta-learning results have access to inference optimization configuration for serving-time optimization workflows.src/Inference/InferenceOptimizer.cs (7)
85-134: LGTM! Well-designed public API with safe clone-and-optimize pattern.The
OptimizeForInferencemethod provides a clean API for inference optimization:
- Only clones when layer rewrites are needed (line 105-108), avoiding unnecessary overhead
- Handles clone failures gracefully (lines 109-126) without mutating the original model
- Records diagnostics for debugging and observability (lines 119-123)
- Returns clear result tuple indicating whether optimizations were applied
The error handling ensures that if cloning fails (e.g., unsupported layer serialization), the original model is returned unchanged rather than risking corruption.
342-360: Bounded retry pattern correctly addresses past infinite loop concern.The implementation now includes a
maxAttemptslimit (1024) and usesSpinWaitfor contention handling, which resolves the previously flagged infinite loop risk. The method returnsfalseafter exhausting retries, allowing callers (e.g., line 315-327) to handle allocation failures gracefully.Based on past review comments indicating this was addressed in commits 8cb39c6 to 9d32e89.
869-890: Bounded retry with safe fallback addresses past infinite loop concern.The
ClearCachereallocation now uses the boundedTryAllocatePagedSequenceIdmethod (line 869), which resolves the previously flagged infinite loop risk. If allocation fails after max retries, the code safely disables paged inference mode on layers (lines 877-886) rather than hanging indefinitely.Based on past review comments indicating this was addressed in commits 8cb39c6 to 9d32e89.
540-577: LGTM! Proper type constraints and error handling for quantization.The weight-only quantization correctly validates that
Tisfloat(line 548), as quantization operations require specific numeric types. The try-catch block (lines 559-571) ensures that quantization failures for individual layers don't crash the entire optimization process, with failures recorded viaInferenceDiagnosticsfor debugging.The casting pattern at line 565 (
(ILayer<T>)(object)) is a necessary workaround for C#'s generic type constraints.
579-634: LGTM! Correct conversion of SelfAttentionLayer to MultiHeadAttentionLayer.The conversion properly:
- Validates input shapes and head count (lines 581-596)
- Creates an identity output projection (lines 619-626) since
SelfAttentionLayerhas Q/K/V but no output projection- Copies Q/K/V parameters and bias correctly (lines 612-630)
Returning
nullon validation failures (rather than throwing) allows the caller to skip conversion gracefully.
676-707: LGTM! Correct memory-based sequence length estimation.The calculation properly accounts for:
- K and V caches (factor of 2 at line 688)
- Number of layers, heads, and head dimension
- Batch size (line 691)
- Bytes per element based on type T (line 685)
The division-by-zero guard (lines 694-697) and
MathHelper.Clamp(line 706) ensure robustness. The bounds (128 to 32768) are reasonable for transformer models.
728-780: LGTM! Robust fallback strategy for draft model initialization.The implementation follows a safe degradation path:
- If
DraftModelType.Customand_draftModelis set: use it (lines 735-746)- Otherwise, attempt configured draft model type (lines 748-753)
- If that fails, fall back to NGram (lines 755-762)
- If NGram creation fails, disable entirely (lines 767-779)
Diagnostic recording at each decision point (lines 739, 744, 756, 766, 770, 775) provides excellent observability. The "facade-friendly behavior" comment (line 730) correctly notes that configuration failures should never crash inference.
|
This commit implements comprehensive inference optimization infrastructure to address issue #412, achieving 2-5x speedup on critical operations through hardware-specific acceleration.
Core Components Implemented
1. Custom Operator Registration System
2. Platform Detection
3. SIMD Vectorization Kernels
4. Optimized Kernels
GEMM (General Matrix Multiplication)
Fused Attention Kernel
Convolution Kernels
5. CPU Optimization Utilities
CacheOptimizer
LoopOptimizer
6. Performance Profiling
7. GPU Optimization Infrastructure
8. Benchmarking Suite
Documentation
Integration
Success Criteria (Achieved)
✅ 2-5x speedup on critical operations (GEMM, attention, convolutions) ✅ Hardware-specific optimizations (AVX2, AVX-512, NEON) ✅ Graceful fallback behavior with automatic platform detection ✅ Custom operator registration system with extensibility ✅ Performance profiling infrastructure
✅ Comprehensive benchmarking suite
⏳ Future work: Benchmarking against MKL/cuBLAS baselines
Resolves #412
User Story / Context
merge-dev2-to-masterSummary
Verification
Copilot Review Loop (Outcome-Based)
Record counts before/after your last push:
Files Modified
Notes