Skip to content

docs(benchmarks): refresh post-#699 baseline and update the Claude git rules - #754

Merged
tylerkron merged 2 commits into
mainfrom
cursor/benchmarks-docs-claude-rules-5b39
Sep 21, 2026
Merged

tylerkron merged 2 commits into
mainfrom
cursor/benchmarks-docs-claude-rules-5b39

Conversation

@tylerkron

@tylerkron tylerkron commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

What was wrong

How it was fixed

Docs and comments only; no production code.

…t rules

Keep the 2a59fd1 table as the pre-#699 before, record the after numbers from
that PR, and stop telling agents that decode allocation is off zero. Stub the
Claude git-workflow rule to CONTRIBUTING.md instead of a third dialect that
still says master.

Co-authored-by: Tyler Kron <tylerkron@gmail.com>
@tylerkron
tylerkron requested a review from a team as a code owner September 18, 2026 10:19
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Refresh post-#699 benchmarks and centralize git workflow guidance

📝 Documentation ⚙️ Configuration changes 🕐 Less than 10 minutes

Grey Divider

AI Description

• Records #699’s protobuf latency and allocation improvements beside preserved pre-fix baselines.
• Corrects stream decode guidance to acknowledge unavoidable subscriber event allocations.
• Replaces stale Claude git instructions with canonical contribution guidance.
Diagram

graph TD
  A["Claude agents"] --> B["Git rule stub"] --> C["CONTRIBUTING.md"]
  D["PR #699 results"] --> E["Benchmark README"]
  F["Stream benchmark"] -->|"documents allocation floor"| E
Loading
High-Level Assessment

The approach is appropriate: preserving the original baseline while adding same-machine #699 results maintains historical comparability, and replacing duplicated Claude instructions with a short CONTRIBUTING.md pointer prevents workflow drift. Rerunning benchmarks elsewhere would produce less comparable timing data, while deleting the Claude rule entirely would reduce discoverability.

Files changed (3) +22 / -80

Documentation (2) +19 / -12
README.mdDocument protobuf benchmark improvements from #699 +17/-11

Document protobuf benchmark improvements from #699

• Relabels the existing '2a59fd1' results as the pre-#699 baseline and adds same-machine before-and-after measurements from #699. It explains the first-sample latency, full-drain, and allocation improvements without referring to #697 as still open.

src/Daqifi.Core.Benchmarks/README.md

StreamDecodeBenchmarks.csClarify the stream decode allocation baseline +2/-1

Clarify the stream decode allocation baseline

• Updates benchmark remarks to explain that regressions appear as allocation growth from the existing approximately 1.38 KB baseline, rather than growth from zero. The baseline comes from 'SampleReceived' event arguments paid by real subscribers.

src/Daqifi.Core.Benchmarks/StreamDecodeBenchmarks.cs

Other (1) +3 / -68
git-workflow.mdDelegate Claude git workflow rules to CONTRIBUTING.md +3/-68

Delegate Claude git workflow rules to CONTRIBUTING.md

• Replaces duplicated branch, commit, and pull-request recipes with a concise pointer to the repository’s canonical contribution policy. It explicitly prohibits pushing 'main' and inventing a separate commit-message convention.

.claude/rules/git-workflow.md

@qodo-code-review

qodo-code-review Bot commented Sep 18, 2026 •

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Allocation source is misstated ✓ Resolved 🐞 Bug ⚙ Maintainability
Description
The StreamDecodeBenchmarks remarks attribute the full ~1.38 KB per-frame allocation to
SampleReceivedEventArgs, although decoding also creates one DataSample object per enabled
channel. This misdirects future allocation investigations because the 16 sample objects and 16
event-argument objects both contribute to the reported baseline.
Code

src/Daqifi.Core.Benchmarks/StreamDecodeBenchmarks.cs[R27-28]

+/// as allocation climbing, not as a departure from zero — the table is already ~1.38 KB from the
+/// SampleReceived args a real subscriber pays for.
Relevance

●●● Strong

Recent benchmark review accepted correcting misleading allocation attribution in documentation.

PR-#706
PR-#698

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The benchmark subscribes to SampleReceived for every enabled channel, while the decoder explicitly
constructs a DataSample for each decoded channel and SetActiveSample separately constructs
SampleReceivedEventArgs when invoking the subscriber. Therefore, the measured per-frame allocation
cannot be attributed solely to the event arguments.

src/Daqifi.Core.Benchmarks/StreamDecodeBenchmarks.cs[103-110]
src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs[595-596]
src/Daqifi.Core/Channel/AnalogChannel.cs[342-344]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The benchmark remarks incorrectly say the full ~1.38 KB baseline comes from `SampleReceivedEventArgs`, while each decoded channel also allocates a `DataSample`.

## Fix Focus Areas
- src/Daqifi.Core.Benchmarks/StreamDecodeBenchmarks.cs[27-28]

## Recommended Fix
Revise the remarks to say the baseline includes both each channel's decoded `DataSample` and the `SampleReceivedEventArgs` created for the benchmark subscriber, without attributing the entire measured amount to either object type.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
Review mode: ⏭️ Skipped: The push only changes documentation and benchmark comments, with no runtime, configuration, test, or build behavior changes.

Grey Divider

Tip of the day
💡 Did you know, you can add REVIEW.md to your repo root and Qodo follows it on every PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread src/Daqifi.Core.Benchmarks/StreamDecodeBenchmarks.cs Outdated
…enchmark claims

The git-workflow rule is loaded into every Claude Code session here and
was mostly right. Keep its guardrails, drop the master/generic-recipe
bits, and state what the repo actually does: merge-queue-gated
squash-only main, branch from origin/main, conventional-commit titles,
PR descriptions that lead with the problem, no git stash across
worktrees.

StreamDecodeBenchmarks: the ~1.38 KB baseline is DataSample plus
SampleReceivedEventArgs, not the event args alone. README: the after
table is #699's own run, not verifiably the same machine as the table
above it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@tylerkron tylerkron changed the title docs(benchmarks): refresh post-#699 baseline and drop stale Claude git rules docs(benchmarks): refresh post-#699 baseline and update the Claude git rules Sep 18, 2026
@tylerkron

Copy link
Copy Markdown
Contributor Author

/agentic_review

@qodo-code-review

Copy link
Copy Markdown

Code review by qodo was updated up to the latest commit 9a61dd1

@tylerkron

Copy link
Copy Markdown
Contributor Author

Reviewed (Claude): partial - the benchmark refresh is right and its numbers are #699's real measurements (now labelled as copied, not re-run); the Claude workflow rules were mostly correct live guardrails, so they're updated to the actual merge-queue/squash/conventional-commit process instead of being cut to a two-line stub. Qodo-clean on 9a61dd1, CI green — ready for review.

@tylerkron
tylerkron added this pull request to the merge queue Sep 21, 2026
Merged via the queue into main with commit 2f3e095 Sep 21, 2026
4 checks passed
@tylerkron
tylerkron deleted the cursor/benchmarks-docs-claude-rules-5b39 branch September 21, 2026 01:50
tylerkron added a commit that referenced this pull request Sep 27, 2026
…leset

README.md is packed as the nuget.org PackageReadmeFile, where relative links 404, so the new CONTRIBUTING/SECURITY links are absolute GitHub URLs. In CONTRIBUTING: replying to a review thread does not clear the conversation-resolution gate, only resolving it does; the squash commit is the PR title plus (#N) with only Co-authored-by trailers in the body; and the agent rule files (#735, #754, now landed) already defer here, so say that instead of asking them to stop restating the process.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

perf(sdcard): the protobuf log parser spends 1.6ms and 6.7MB before its first sample, while CSV and JSON take 2us

2 participants