docs(benchmarks): trim restating narration - #791
Merged
Merged
Conversation
Drop benchmark comments that repeat the README, the workflow, or old tickets. Keep the remarks that say why a number is a fair measurement. Co-authored-by: Tyler Kron <tylerkron@gmail.com>
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you |
PR Summary by QodoTrim redundant benchmark narration
AI Description
High-Level Assessment
Files changed (5)
|
Restore the reasons the trim dropped: why the benchmark workflow never runs on a pull request, the SimpleJob route for net9.0 numbers, the Directory.Build.targets effect of IsPackable=false, the unit mapping and no-allocation expectation for the scaling benchmarks. Reword the IsTestProject note: without it the VSTest target already skips the project (with a low-importance notice), so the property is an explicit opt-out, not what stops a test-host launch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 7bd09c7 |
Contributor
Author
|
Qodo-clean, CI green — ready for review |
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
The benchmark project's comments repeated the README, the workflow and the ticket history at length. This PR cuts them down to the reason behind each non-obvious choice. Only comments and the workflow header change. No benchmark logic, TFM, MSBuild property value or CI step changes.
Program.cs: drop the XML doc that restated the README andbenchmarks.yml. TheBenchmarkSwitcherone-liner stays.Daqifi.Core.Benchmarks.csproj: one short comment per property.TargetFramework: net10.0 only, sodotnet run -c Releaseneeds no-f. For net9.0 numbers, add aSimpleJobrather than a second TFM.IsPackable: this is a measuring instrument, not a shipped package. The property also keeps theDirectory.Build.targetspackage metadata off the project, because that file is gated onIsPackable.IsTestProject: explicitly not a test project, sodotnet teston the solution skips it.ChannelScalingBenchmarks: shorten the class remark. It keeps the raw count to volts to engineering units mapping, the per-sample cost rationale, "neither should allocate", and why the channel lock is inside the measurement. The method summary that repeated the lock note is gone. The per-sampleOperationsPerInvokenote, the pre-generated ADC counts, and the transducer/identity case notes stay.StreamDecodeBenchmarks: drop the perf(streaming): steady-state decode allocates per frame what it could cache — channel snapshot+sort, double buffer copy, unconditional wrappers #490/test(flake): Record_AfterTheFirstSamplePerChannel_AllocatesNothing fails on a full net10 run, passes in isolation #531 retelling, which the README table already cites. These stay: the whole-path note (snapshot cache, timestamps, gap detection, unpacking, event dispatch), the per-frameOperationsPerInvokenote, the ~1.38 KBDataSampleplus event-args sentence, and theDecodeCasemonotonic-clock and 32-bit rollover remarks.benchmarks.yml: the header is now two lines. It still says the workflow is on demand only and never runs on a pull request, because shared-runner timings are too noisy to gate a merge on (perf: no benchmark harness — nine perf tickets were measured by hand and nothing guards the wins #640), and it points to the README. The filter-injection comment is unchanged.Note on
IsTestProjectI checked this against the .NET 10.0.203 SDK's
Microsoft.TestPlatform.targets. Without the property, theVSTesttarget already skips a project that lacks the test SDK: it logs a low-importance "Skipping running test for project" notice and never starts a test host. So the property is an explicit opt-out, and the comment now says that. The earlier wording claimed the property is what stops a test-host launch, which isn't true.BenchmarkProjectTestsstill pins the value.Overlap with other open PRs
StreamDecodeBenchmarks.csbaseline, README) and ci: SHA-pin actions, drop net9 from benchmarks, build timeout #768 (benchmarks.ymlSHA pins, net9SimpleJobcomment) edit the same files in different hunks. Agit merge-treeof this branch with each PR head merges cleanly, so no textual conflict is expected in either merge order.Test plan
origin/mainagainst this branch. C# with comments stripped: identical token streams. csproj with comments dropped: identical XML trees.benchmarks.yml:yaml.safe_loadoutput identical, and non-comment lines identical and in the same order. The checker does catch real changes: it flags bench: baseline DecodeRawAnalogFrame (AnalogInData) #787's code change and chore: GHA permissions/timeouts, Any-CPU-only solution, TFM comment #741'stimeout-minutes.dotnet build Daqifi.Core.sln -c Release: 0 warnings. Benchmark project--no-incremental: 0 warnings.Daqifi.Core.Tests.Buildtests (includingBenchmarkProjectTests) pass on net9.0 and net10.0 (62 each).🤖 Generated with Claude Code