Repository navigation
fix(bench): a cp1252 console must not be able to destroy a finished measurement - #478
Merged
Merged
Conversation
…easurement
The first real run measured both platforms correctly and then the Windows leg
died in `print`:
UnicodeEncodeError: 'charmap' codec can't encode character '−'
The Windows console is cp1252, which has no U+2212 MINUS SIGN — and the results
table header carried one, `vs v2 (− = faster)`. Three release builds and sixty
timed runs, lost to a typographic character in a column heading.
Two changes, because the character was the trigger and not the fault.
**Ordering.** `emit` printed before it wrote. Both file writes name their
encoding, so only stdout was ever at risk, but going first meant a console fault
also cost `bench.log` and left the artifact step with nothing to upload. The
durable outputs now go first — file, then job summary, then stdout — so a
presentation-layer failure can no longer take the results with it.
**The stream, not just the character.** The header loses its U+2212 (a hyphen
says the same thing), and stdout is reconfigured to UTF-8 so the em dashes and
arrows already in the verdict prose cannot fire the same fault from a different
line. `isinstance` rather than `hasattr` for the guard: the duck-typed check
narrows to `object`, which `ty` correctly refuses to call a method on.
Proven against the condition that failed, not against a description of it:
`PYTHONIOENCODING=cp1252` reproduces the original traceback on the old code and
runs clean here, with `bench.log` written.
The measurement itself was not lost — the per-arm figures were in the run log,
and applying the harness's own rule by hand gives Windows a noise floor of
33.9 ms, a control separation of 259.0 ms (62.6%), and a v2/v3 gap of 7.1 ms
(1.7%): indistinguishable, the same verdict Linux reported for itself. This is
so the harness can state that on its own next time.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
✅ Based on
main— mergeable now. Follow-up to #476, from its first real run.What happened
Both legs measured correctly. The Windows leg then died in
print:The Windows console is cp1252, which has no U+2212 MINUS SIGN — and the results
table header carried one:
vs v2 (− = faster). Three release builds and sixtytimed runs, lost to a typographic character in a column heading.
Two changes, because the character was the trigger and not the fault
Ordering — this is the real defect.
emitprinted before it wrote. Bothfile writes name their encoding, so only stdout was ever at risk; but printing
first meant a console fault also cost
bench.logand left the artifact step withnothing to upload. Durable outputs now go first — file, then job summary, then
stdout — so a presentation-layer failure can no longer take the results with it.
The stream, not just the character. The header loses its U+2212 (a hyphen
says the same thing), and stdout is reconfigured to UTF-8 so the em dashes and
arrows already in the verdict prose can't fire the same fault from a different
line. Fixing only the one character would have left the class open.
isinstancerather thanhasattron the guard: the duck-typed check narrows toobject, whichtycorrectly refuses to call a method on.Verified against the condition, not a description of it
PYTHONIOENCODING=cp1252reproduces the original traceback on the old code, andruns clean here with
bench.logwritten. The "before" is the genuine CItraceback rather than a simulated one.
The measurement was not lost
The per-arm figures were in the run log. Applying the harness's own rule by hand:
v2v3system(control)The control separated hard on both, so the instrument is demonstrably resolving
allocator effects — and against that, v3 does not beat v2 on either platform.
The two deltas even point in opposite directions, which is what a null result
looks like.
So narrowing the pin to
cfg(target_os = "macos")would buy Linux and Windowsnothing measurable, and #448's unquantified premise is now quantified. The
Linux verdict is the harness's own; the Windows one is mine, computed by hand —
which is what this PR exists to stop being necessary.
Re-dispatch after merge if you want the harness to state Windows for itself; the
answer is not in doubt, only its provenance.