perf: cut upload allocations and read real fan RPM - #1
Merged
Conversation
Reduce avoidable allocations on the hardware path and fix the fan dashboard unit, from the performance audit. Byte-exact protocol behavior is preserved (21/21 tests pass, incl. a new parity test). - NvApiI2cBus.Write/Read pin the caller buffer with `fixed` instead of ToArray()+GCHandle, removing a clone on every command/chunk/RGB packet (enables AllowUnsafeBlocks in AorusLcd.Core). - ProtocolFrames.BuildUpload gains a (prefix, payload) overload that chunks the descriptor+frame directly; image/text uploads no longer allocate a ~108 KB LOH concat buffer, and the intermediate chunk list is gone. ByteOps (single-use) removed. - NVML fan speed now reads real RPM via nvmlDeviceGetFanSpeedRPM, the unit the panel E3 field expects, with a graceful fallback to the percentage API on older drivers. Read-only telemetry only; the fan stays on the GPU's own (auto) curve. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5fb67349-2267-4fa6-8d59-f8fc946e89ef
There was a problem hiding this comment.
Pull request overview
This PR applies performance-focused changes across the upload path and hardware telemetry: it removes avoidable allocations during NVAPI I2C transfers and static upload frame construction, and it corrects the fan-speed unit used for the panel dashboard by reading RPM where available via NVML.
Changes:
- Add a
ProtocolFrames.BuildUpload(prefix, payload, ...)overload to chunk logical concatenations without allocating a combined buffer, with parity test coverage. - Remove per-transfer buffer cloning in
NvApiI2cBusby pinning spans directly (enabling unsafe blocks inAorusLcd.Core). - Update NVML fan telemetry to prefer real RPM (
nvmlDeviceGetFanSpeedRPM) with fallback to the percent API.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/AorusLcd.Tests/ProtocolTests.cs | Adds a parity test ensuring the new prefix+payload upload builder matches concatenated payload output. |
| src/AorusLcd.Gui/Services/HardwareService.cs | Switches static frame upload to the new non-concatenating BuildUpload(prefix, payload, ...) overload. |
| src/AorusLcd.Core/Sensors/NvmlSensorSource.cs | Reads fan speed as RPM when available; falls back to percent on older NVML exports. |
| src/AorusLcd.Core/Sensors/Nvml.cs | Adds P/Invoke surface for nvmlDeviceGetFanSpeedRPM and its FanSpeedInfo struct/version constant. |
| src/AorusLcd.Core/ProtocolFrames.cs | Implements prefix+payload chunking to avoid large concatenation allocations. |
| src/AorusLcd.Core/Nvapi/NvApiI2cBus.cs | Pins caller-provided buffers directly for NVAPI I2C calls to avoid per-call allocations. |
| src/AorusLcd.Core/ByteOps.cs | Removes the now-unneeded concat helper. |
| src/AorusLcd.Core/AorusLcd.Core.csproj | Enables unsafe blocks needed for the updated NVAPI I2C implementation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Compute the prefix+payload total length in a checked context so the theoretically-possible (though unreachable for real panel payloads) int overflow throws instead of silently wrapping to a negative value and producing a corrupt nchunks/header. Addresses PR review feedback. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6b7edc47-a7cf-4ed7-ab28-19dbecf7bcd7
Collaborator
Author
|
Good catch in principle, though I'll push back on part of it. Both operands are That said, the guard is free insurance, so I've wrapped the sum in a I did not restructure |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/AorusLcd.Core/ProtocolFrames.cs:95
nchunksis declared asuint, but the subsequentfor (int c = 0; c < nchunks; c++)comparesinttouint, which won’t compile (CS0019). Using anintchunk count here also avoids the unchecked(int)nchunkscast for list capacity; cast touintonly when writing the header field.
uint nchunks = (uint)(total / FrameSize + 1);
var frames = new List<UploadFrame>((int)nchunks + 3)
JustinMDotNet
added a commit
that referenced
this pull request
Aug 11, 2026
## What Make it impossible to construct the GPU I2C bus at the wrong speed. This is the structural fix for the audit's top finding (#1), the root cause behind the recurring panel-freeze patches (#17/#20/#21/#22). > Stacked on `fix/bus-lock-hardening` (PR #27) -> `audit/integration`. Base retargets as those merge. ## Problem The GPU has one physical I2C engine shared by the LCD (`0x61`) and RGB (`0x71`/`0x75`) controllers. It only behaves at 400 kHz and wedges silently at the NVAPI default speed. `NvApiI2cBus`'s `speed` parameter defaulted to `NvApiI2cSpeed.Default`, so safety depended on every call site remembering to pass `Khz400`. Each past freeze was a call site that forgot. ## Changes - **Remove the unsafe default (compile-time guard).** `NvApiI2cBus`'s `speed` (and `address`/`port`) parameters are now required, so no caller can silently get the wedging default. The `NvApiI2cSpeed.Default` enum member is kept (it documents the NVAPI value); only its use as a parameter default is gone. Added a read-only `Speed` property for testability; `BuildInfo` and the wire encoding are unchanged. - **Single bus factory.** New `NvApiBusFactory` is the one production place that builds an Aorus bus: `Panel(gpu)` -> `0x61` / port 1 / 400 kHz, `Rgb(gpu, addr)` -> addr / port 1 / 400 kHz. Port and speed are enforced in one spot. - **Route all construction through it.** `NvApiPanelLocator` and `RgbLocator` build via the factory; the locators' probe/detection/ordering/dispose logic is byte-for-byte unchanged. A caller-supplied non-default `port` to `Locate` is still honored (direct construction, still 400 kHz), not ignored. - **Policy tests.** `NvApiBusFactoryTests` assert the invariant (address/port/400 kHz) with no hardware, since the `NvApiI2cBus` constructor is pure. ## Scope note This is the "Core session" step the audit recommended as sufficient for alpha; a fuller single-owner bus service (the other side of the "needs human judgment" design fork) remains a possible future step. This PR closes the accidental-default-speed root cause without an over-broad rewrite. ## Testing - `dotnet build -c Release` clean; `dotnet test -c Release` green (52/52, +3 factory tests). AOT-safe.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Performance/correctness changes from the full-repo performance audit. Byte-exact protocol behavior is preserved; 21/21 tests pass (build clean, 0 warnings).
Changes
NvApiI2cBus.Write/Readnow pin the caller's buffer withfixedinstead ofToArray()+GCHandle, removing an allocation on every command, upload chunk, and RGB packet. EnablesAllowUnsafeBlocksinAorusLcd.Core. The pinned pointer is used only inside the synchronous NVAPI call.ProtocolFrames.BuildUploadgains a(prefix, payload)overload that chunks the logical descriptor+frame concatenation directly. Image/text uploads no longer allocate a ~108 KB LOH concat buffer, and the intermediateList<byte[]>is gone. The single-payload overload now delegates to it. Single-useByteOpsremoved. A new byte-parity test locks the prefix path to the old concat-then-chunk output (chunk count, padding, and F1 header fields).nvmlDeviceGetFanSpeedRPM(the unit the panel E3 field expects) instead of feeding a 0–100% value into an RPM field, with a graceful fallback to the percentage API on drivers that lack the export.Testing
dotnet build -c Debug— clean, 0 warnings.dotnet test— 21/21 pass, including the newBuildUpload_PrefixMatchesConcatenatedPayloadparity test.Notes
Hardware-mandated pacing and byte-exact protocol/RLE behavior are untouched; these are allocation/units changes only. Robustness items from the audit (config watch-before-read, cancellable bus-lock) and the conditional GIF-memory rewrite are intentionally left for follow-ups.