Move the unmarshaler handoff to internal/unmarshal - #110
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #110 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 6 +1
Lines 310 310
=========================================
Hits 310 310 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
creasty
force-pushed
the
claude/v1-11-internal-unmarshal
branch
2 times, most recently
from
September 16, 2026 02:04
c017ab6 to
61ad5c5
Compare
Before parsing a tag by kind, fillField offers it to the field's own
UnmarshalText and then to its UnmarshalJSON, and takes whatever one of
them makes of it. Which unmarshaler is asked first, what each is not
asked at all, and whose rejection is reported when neither took the tag
are rules about unmarshalers, not about defaults: nothing in them needs
a field, a tag or a walk.
They are now internal/unmarshal.Tag(target any, value string), in a file
of its own with its own tests. The package takes what an unmarshaler is
implemented on rather than a reflect.Value, so reflect stays out of its
contract and its tests pass ordinary pointers; fillField hands it
field.Addr().Interface(). set.go loses the encoding import, and keeps
encoding/json, which the kind switch still decodes tags with.
One thing changes beyond the move: the old code called
field.Addr().Interface() twice, once per type assertion, and the call
site calls it once for both. That is work removed from the path every
tagged zero field takes.
Why a package. The handoff has four rules the suite could only show
through Set, which needs a struct and a tag to ask any of them, and
set_unmarshaler_test.go spends 617 lines doing so. Under internal/ it is
no more public than it was, and each rule is a line.
internal/unmarshal/unmarshal_test.go, black box (in package
unmarshal_test), covers them: a value taken by UnmarshalText, by
UnmarshalJSON where there is no UnmarshalText, and by UnmarshalJSON
after UnmarshalText refused it; a target implementing both asked
through UnmarshalText, with UnmarshalJSON never offered the value; both
refusing, so the first rejection is the error, and UnmarshalJSON's when
it was the only one asked; and the values neither is offered, which
report nothing: an empty value to either, "{}" and "[]" to
UnmarshalJSON, which reads them as an empty object or array, though
UnmarshalText is offered them like any other value, and anything at all
to a target implementing neither.
The fixtures keep what they were offered, so a test can tell an
unmarshaler that was never asked from one that was asked and took it,
and each carries the error it refuses with, so no sentinel has to live
at package level: every test names the rejection it expects.
Behavior is master's; nothing is flipped. set_unmarshaler_test.go's 12
tests pass unchanged through the new call site.
Measured: make bench-compare BASE=origin/master, 6 interleaved rounds at
400ms, Go 1.26.5 darwin/arm64. Allocations are identical on every row.
The rows that reach the handoff with a tag on a zero field are 3 to 12%
faster: every Parse row but escaped_tag, -2.90% to -7.61%; Set/scalars
-5.79% and Set/composites -6.47%; Walk/pointer/tagged_ints -12.32% and
Walk/pointer/tagged_chain/depth=4 -10.67%; Fail/nested -7.80%. The rows
that never reach it do not move: Walk/struct/untagged,
Walk/struct/nested_values, Walk/slice/structs, Walk/slice/pointers and
Walk/pointer/caller_chain all read ~. That is the Interface() call this
commit removes, not placement: set.go has three field.Addr().Interface()
calls where master has four, and the two the handoff made are now one.
Walk/map/slices reads +1.65%, the one row against the pattern, and it
parses no tag; geomean -2.60%.
Parse/unmarshaler/text reads -1.87% there but 109 to 114 ns on both
sides over 12 fresh processes, so take that row as unmoved.
Walk/slice/pointers, which split into two modes while the path index
was written, is 184 to 187 us across 12 processes here.
The handoff was a call before and is a call now: master cannot inline
unmarshalByInterface, at cost 438, and Tag costs 196, so the package
boundary adds nothing. -gcflags=-m still reports "path does not escape"
for set, setField and fillField, and moves nothing to the heap.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
creasty
force-pushed
the
claude/v1-11-internal-unmarshal
branch
from
September 16, 2026 02:05
61ad5c5 to
79bab8c
Compare
creasty
added a commit
that referenced
this pull request
Sep 16, 2026
internal/method and internal/unmarshal come back into the root package as method.go and unmarshal.go, and the path check moves out of set.go into path.go. No internal/ directory is left. A package under internal/ is not free where hot inlined code crosses it. The walk's repeatsNear, repeatsFar and leave inline at twelve call sites in set.go; behind internal/ they measured 1.7% to 4.1% slower across seven of the eleven Walk/slice and Walk/map rows, with Walk/slice/pointers 186.8 to 194.5 us at p=0.000 over 12 processes, though the compiler's inlining decisions and escape analysis were identical either way. Four controls put that cost on the boundary and nowhere else: the same block split into a same-package file is free, inert dead code placed ahead of set.go is free, and an inert second package linked into the binary is free. What internal/ was bought for was tests. Machinery the suite can reach only through Set now lives in a file of its own with its tests beside it, in package defaults rather than defaults_test: method_test.go with 8 cases, unmarshal_test.go with 12 and path_test.go with 10. Those three files are the only exception to the black-box rule, and CLAUDE.md says so with the measurement, so the boundary is not tried again. Two things carry over rather than revert. unmarshalTag keeps #110's shape, taking what an unmarshaler is implemented on rather than a reflect.Value, so the call site still makes field.Addr().Interface() once for both type assertions rather than twice. isPromotedMethod names no method of its own; setter.go keeps the one line that names SetDefaults. One thing changes. repeats is repeatsNear, since callers ask here.repeatsNear() || here.repeatsFar() and neither half answers for the other, which the old name claimed. They stay two methods: one holding the scan and the call to repeatsInIndex costs 101 against the inliner's budget of 80 and does not inline, and the call that would leave in the walk made Walk/slice/pointers take 1.6 times as long in about half the runs. Measured: make bench-compare BASE=origin/master, Go 1.26.5 darwin/arm64. Allocations are identical on every row, and the geomean reads -0.04% over 6 rounds. The walk family is flat over 12 rounds, every row ~, with Walk/slice/pointers 186.1 to 187.7 us at p=0.219. Folding the two packages back is not free either, in the other direction: Set/scalars +0.80%, Set/composites +0.62%, Parse/int +0.76%, Parse/string +0.64%, Parse/unmarshaler/rejected +0.63% and Parse/kinds +0.51%, each over 12 rounds. The same controls rule out placement there, so those rows lose whatever the boundary was giving them. Both effects are real and they point opposite ways, so a package boundary does not simply cost: it perturbs code generation, which helped the parse path slightly and hurt the walk's hot path materially. This takes 0.8% on those rows to be rid of the walk's 4.1%, and to keep one rule rather than two. make test, make cover and make bench-smoke pass, the last with all 46 cases. Coverage is 100.0%, with every moved function covered where it lives rather than through Set. -gcflags=-m reports "path does not escape" for set, setField and fillField, moves nothing to the heap, and still inlines the twelve walk call sites. golangci-lint and the Go 1.22 leg are left to CI, as ever. Co-authored-by: Claude Opus 5 <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.
Before parsing a tag by kind,
fillFieldoffers it to the field's ownUnmarshalTextand then to itsUnmarshalJSON, and takes whatever one of them makes of it. Which unmarshaler is asked first, what each is not asked at all, and whose rejection is reported when neither took the tag are rules about unmarshalers, not about defaults: nothing in them needs a field, a tag or a walk.This is the second of three moves into
internal/, after #109: the promoted-method check, the unmarshaler handoff here, and then the state a walk carries down a path.The change
The handoff becomes
internal/unmarshal.Tag(target any, value string) (bool, error). The package takes what an unmarshaler is implemented on rather than areflect.Value, soreflectstays out of its contract and its tests pass ordinary pointers:set.goloses theencodingimport, and keepsencoding/json, which the kind switch still decodes tags with.One thing changes beyond the move. The old code called
field.Addr().Interface()twice, once per type assertion; the call site calls it once for both. That is work removed from the path every tagged zero field takes, and it shows up in the benchmarks below.Why a package
The handoff has four rules the suite could only show through
Set, which needs a struct and a tag to ask any of them —set_unmarshaler_test.gospends 617 lines doing so. Underinternal/it is no more public than it was, and each rule is a line. The handoff was a call before the move and is a call after it (unmarshalByInterfacecost 438 against the inliner's budget of 80;Tagcosts 196), so the boundary adds nothing.Behavior change
None.
set_unmarshaler_test.go's 12 tests pass unchanged through the new call site.Pins
internal/unmarshal/unmarshal_test.go, black box (package unmarshal_test), 12 cases. The fixtures keep what they were offered, so a test can tell an unmarshaler that was never asked from one that was asked and took it, and each carries the error it refuses with, so the suite keeps no package-level sentinels and every test names the rejection it expects.UnmarshalText; byUnmarshalJSONwhere there is noUnmarshalText; byUnmarshalJSONafterUnmarshalTextrefused it.UnmarshalText, andUnmarshalJSONis never offered the value.UnmarshalJSON's when it was the only one asked."{}"and"[]"toUnmarshalJSON, which would read them as an empty object or array, whileUnmarshalTextis offered them like any other value; anything at all to a target implementing neither.Verified
gofmt -s -l .andgo vet ./...are clean.make testpasses.make cover(-race -shuffle=on,./...) gives 100.0% in all three packages,Tagamong them.make bench-smokepasses all 46 cases.-gcflags=-mstill reportspath does not escapeforset,setFieldandfillField, and moves nothing to the heap.make bench-compare BASE=origin/master, 6 interleaved rounds at 400ms, Go 1.26.5 darwin/arm64. Allocations identical on every row. The rows that reach the handoff with a tag on a zero field are 3–12% faster — everyParserow butescaped_tag(−2.90% to −7.61%),Set/scalars−5.79%,Set/composites−6.47%,Walk/pointer/tagged_ints−12.32%,Walk/pointer/tagged_chain/depth=4−10.67%,Fail/nested−7.80% — while the rows that never reach it do not move (Walk/struct/untagged,Walk/struct/nested_values,Walk/slice/structs,Walk/slice/pointers,Walk/pointer/caller_chain). That pattern is the removedInterface()call rather than code placement.Walk/map/slicesreads +1.65%, the one row against the pattern, and it parses no tag. Geomean −2.60%.Parse/unmarshaler/textreads −1.87% in the table but 109–114 ns on both sides, so take it as unmoved;Walk/slice/pointers, which split into two modes while the path index was written, is 184–187 µs here.golangci-lint(the installed v1 cannot read the v2 config) and the Go 1.22 leg. CI covers both.🤖 Generated with Claude Code