Skip to content

refactor: read tag.value in fillField rather than aliasing it - #112

Merged
creasty merged 1 commit into
masterfrom
claude/remove-defaultval-alias-48cf99
Sep 16, 2026
Merged

creasty merged 1 commit into
masterfrom
claude/remove-defaultval-alias-48cf99

Conversation

@creasty

@creasty creasty commented Sep 16, 2026

Copy link
Copy Markdown
Owner

fillField opened with defaultVal := tag.value, and a comment saying the parsing below reads better against a plain name. It does not: tag.value says where the value comes from, and defaultVal makes the reader carry a second name for it through the 28 uses that follow. The alias and its comment are gone, and every use reads tag.value.

set keeps its own defaultVal, which is not an alias. It holds what Tag.Lookup returned, before there is a fieldTag to read the value from.

No test is flipped or touched. make test, make cover and make bench-smoke pass, the last with all 46 cases; coverage is 100.0%, gofmt and go vet are clean.

The codegen is not identical

set and setField are unchanged. fillField grows 32 bytes of text, 11744 to 11776, and loses 16 bytes of frame, locals 0x7b8 to 0x7a8 — the alias was a slot holding the string, and the field is now re-read from the tag argument at each use.

Measured

make bench-compare BASE=origin/master, Go 1.26.5 darwin/arm64. Allocations are identical on every row. Over 6 rounds every row reads ~ but three, and over 12 rounds of those three:

row base head
Walk/map/pointers/entries=100 23.26 µs ± 2% 23.15 µs ± 1% ~ (p=0.843)
Walk/slice/pointers/elements=1000 187.3 µs ± 1% 186.9 µs ± 67% ~ (p=0.478)
Walk/map/slices/entries=100 30.50 µs ± 1% 33.60 µs ± 15% ~ (p=0.799)

The medians hide a mode this branch adds

Walk/slice/pointers runs at about 187 µs on master in all 30 processes tried, and on this branch in 14 of 30; the other 16 run at about 312 µs — the slow mode #108 recorded for this row. A process picks a mode and holds it for its whole run, which is what puts the head side at ±67% where master reads ±1%.

It is not code placement. set, setField and fillField sit at the same addresses in both test binaries, and an inert function added to either side is eliminated by the linker before it can perturb anything, so that control says nothing either way. What is left is fillField's own body, and which of the two codegen differences brings on the mode is not established here.

So this is a readability change that may cost a hot row two thirds of its time in half of all processes. Worth deciding on before merging.

🤖 Generated with Claude Code

fillField opened with `defaultVal := tag.value` and a comment saying the
parsing below reads better against a plain name. It does not: tag.value
says where the value comes from, and defaultVal makes the reader carry a
second name for it through the 28 uses that follow. The alias and its
comment are gone, and every use reads tag.value.

set keeps its own defaultVal, which is not an alias. It holds what
Tag.Lookup returned, before there is a fieldTag to read the value from.

Not a behavior change: no test is flipped or touched. make test, make
cover and make bench-smoke pass, the last with all 46 cases; coverage is
100.0%, gofmt and go vet are clean, and lint and the Go 1.22 leg are
left to CI, as ever.

The codegen is not identical, though set and setField are. fillField
grows 32 bytes of text, 11744 to 11776, and loses 16 bytes of frame,
locals 0x7b8 to 0x7a8: the alias was a slot holding the string, and the
field is now re-read from the tag argument at each use.

Measured: make bench-compare BASE=origin/master, Go 1.26.5 darwin/arm64.
Allocations are identical on every row. Over 6 rounds every row reads ~
but three, and over 12 rounds of those three, Walk/map/pointers is flat,
Walk/slice/pointers reads 187.3 -> 186.9 us (p=0.478) and
Walk/map/slices 30.50 -> 33.60 us (p=0.799).

Those medians hide a mode this commit adds. Walk/slice/pointers runs at
about 187 us on master in all 30 processes tried, and on this commit in
14 of 30; the other 16 run at about 312 us, the slow mode #108 recorded
for this row. A process picks a mode and holds it for its whole run,
which is what puts the head side at +-67% where master reads +-1%.

It is not code placement. set, setField and fillField sit at the same
addresses in both test binaries, and an inert function added to either
side is eliminated by the linker before it can perturb anything, so that
control says nothing either way. What is left is fillField's own body,
and which of the two codegen differences brings on the mode is not
established here.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (08a42cd) to head (47bfe5a).

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #112   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            7         7           
  Lines          310       309    -1     
=========================================
- Hits           310       309    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@creasty
creasty merged commit 10a16ef into master Sep 16, 2026
7 checks passed
@creasty
creasty deleted the claude/remove-defaultval-alias-48cf99 branch September 16, 2026 04:09
@creasty

creasty commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

Decided: the tradeoff is accepted, and the alias stays gone. tag.value says where the value comes from, and carrying a second name for it through 28 uses does not pay for itself.

Keeping the alias to hold that mode off would mean keeping a local because of the machine code the compiler happens to generate around it. If we are that sensitive, the same argument says to hoist tag.fieldName and tag.present into locals as well, which is ridiculous.

Measured again while drafting the release notes, twelve fresh processes per commit at -test.benchtime 200ms: Walk/slice/pointers/elements=1000 runs at about 313 µs in 8 of 12 processes on master and about 187 µs in the other 4, against 12 of 12 fast at #111 (~187 µs) and at v1.10.0 (~183 µs). The next release's notes record the row and both modes under Performance.

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.

1 participant