Record multiplication inlining benchmark evidence - #231
Conversation
This reverts commit 48ff2f0.
|
@copilot review |
There was a problem hiding this comment.
🟡 Changes recommended
The checksum verification/printing path can emit unvalidated values, and MEMORY.md includes a machine-local artifact path that won’t be usable for other contributors.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds durable, reproducible evidence for the previously deferred operator * inlining proposal by introducing an external-consumer BenchmarkDotNet fixture, recording the ABBA experiment outputs, and updating the documentation to reflect the evidence-backed decision to not adopt the attribute.
Changes:
- Add a new BenchmarkDotNet benchmark fixture and a
--verify-inlining-corpusmode to capture deterministic corpus checksums. - Check in the full experiment record (report + machine-readable summary + paired ratios + commands) under
docs/performance/results/.... - Update top-level performance docs to reference the completed experiment and its conclusion.
File summaries
| File | Description |
|---|---|
| README.md | Links to the completed multiplication inlining experiment report and states the hint was not adopted. |
| MEMORY.md | Updates the delivery-state record with the completed experiment outcome and disposition. |
| docs/performance/multiplication-inlining-test-plan.md | Adds an “Outcome” section pointing to the final report and summary. |
| docs/performance/results/2026-09-05-multiplication-inlining/summary.json | Machine-readable experiment inputs/results and final decision. |
| docs/performance/results/2026-09-05-multiplication-inlining/REPORT.md | Human-readable report summarizing protocol, results, and recommendation. |
| docs/performance/results/2026-09-05-multiplication-inlining/paired-ratios.csv | Raw paired ratios for the ABBA timing campaign. |
| docs/performance/results/2026-09-05-multiplication-inlining/commands.txt | Reproduction commands for validation/disasm/timing runs. |
| benchmarks/FixedPointNano.Benchmarks/Program.cs | Adds a command switch to emit deterministic corpus checksums. |
| benchmarks/FixedPointNano.Benchmarks/FixedPointNanoMultiplyInliningBenchmarks.cs | New benchmark suite targeting multiply-heavy consumer patterns and checksum-based determinism guardrails. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| public IReadOnlyList<KeyValuePair<string, long>> CaptureChecksums() | ||
| { | ||
| return | ||
| [ | ||
| new(nameof(IndependentMultiplyBatch), IndependentMultiplyBatch()), | ||
| new(nameof(DependentMultiplyChain), DependentMultiplyChain()), | ||
| new(nameof(NotionalBatch), NotionalBatch()), | ||
| new(nameof(LerpBatch), LerpBatch()), | ||
| new(nameof(SquareBatch), SquareBatch()), | ||
| new(nameof(PowBatch), PowBatch()), | ||
| ]; | ||
| } |
| - The attribute was rejected because it demonstrated no repeatable consumer | ||
| benefit. It is absent from the delivery branch. The fixture and concise evidence | ||
| are retained under `docs/performance/results/2026-09-05-multiplication-inlining`; | ||
| raw artifacts remain in `C:\dev\FixedPointNano-inlining-evidence\2026-09-05`. |
Co-authored-by: NichUK <346792+NichUK@users.noreply.github.com>
|
@copilot review |
Co-authored-by: NichUK <346792+NichUK@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The benchmark GlobalSetup currently executes the benchmark methods once via CaptureChecksums(), which can perturb tiered compilation/warmup behavior and reduce the control/validity of the recorded timing/disassembly evidence.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
| _singleLeft = _left[17]; | ||
| _singleRight = _right[17]; | ||
|
|
||
| _ = CaptureChecksums(); |
Co-authored-by: NichUK <346792+NichUK@users.noreply.github.com>
…idence' into chore/multiplication-inlining-evidence
…idence' into chore/multiplication-inlining-evidence
Addressed in
Addressed in |
|
@copilot review |
The deferred multiplication inlining proposal needed controlled evidence before adoption. This adds the external-consumer BenchmarkDotNet fixture, records the exact baseline/candidate experiment, and documents the evidence-backed decision to leave
operator *unchanged.Five ABBA blocks completed 20 isolated timing invocations on .NET 10.0.9. Both exact commits passed 1,560 tests and produced byte-identical corpus checksums. Every workload's paired 95% confidence interval crossed parity, no workload reached the required 8/10 direction count, and the aggregate candidate/baseline ratio was 1.0224 (95% CI 0.9803-1.0640), with the candidate faster in 4/10 pairs. The final diff does not contain the rejected attribute.
PR #223 was closed intentionally with the result and its rejected source branch was deleted.
Validation:
dotnet build FixedPointNano.slnx -c Release- passed, zero warningsdotnet test tests/FixedPointNano.Tests/FixedPointNano.Tests.csproj -c Release --no-build- 1,560 passedgit diff origin/develop...HEAD --check- passed