refactor: dedupe stats reduction (#15) + name the data-loss guard (#14) - #21
Merged
Merged
Conversation
…ard (#14) #15: ComputeStats repeated the "Kahan-Neumaier mean + min_element + max_element" triplet three times (ms / pct / gpu), and analyze.py mirrored it. Extract one Summarize() (C++) / _summarize() (Python) so the reduction has a single source -- a change to the summation strategy can no longer drift between the three stat groups or across the two languages. Pure refactor: the merged-CSV header is byte-identical (the 19-test analyzer suite, which checks the header exactly, still passes locally). #14: name the data-loss invariant. The "did this session record any frames?" check was an inline g_frameCounter==0 with a long comment; extract SessionRecordedFrames() as the single definition, used by MergeIntoOutput's zero-frame guard, with the rationale consolidated there (FrameCsvSink's lazy-open is documented as the per-side counterpart). Note: #3 already removed the worst part of this finding -- the removeStale call sites -- by switching the merge to write-temp-then-atomic-rename, so what remained was this naming / comment consolidation; the deeper per-session-filenames fix stays deliberately deferred (one file per PID). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Findings #15 + #14 (code review): cleanup
#15 —
ComputeStatsmean/min/max triplet ×3 (+ analyze.py mirror)ComputeStatsrepeated the "Kahan-Neumaier mean +min_element+max_element" triplet three times (ms / pct / gpu), andanalyze.pymirrored it. Extracted oneSummarize()(C++) /_summarize()(Python) so the reduction has a single source — a change to the summation strategy can no longer drift between the three stat groups or across the two languages (the exact "fixed in one branch, missed in the others" risk the review called out).Pure refactor — the merged-CSV header is byte-identical. The 19-test analyzer suite (which checks the header exactly) still passes:
#14 — name the data-loss invariant
The "did this session record any frames?" check was an inline
g_frameCounter == 0with a long comment. ExtractedSessionRecordedFrames()as the single definition, used byMergeIntoOutput's zero-frame guard, with the rationale consolidated there (FrameCsvSink's lazy-open documented as the per-side counterpart).3 files, +53/−35; no lines over the limit.
🤖 Generated with Claude Code