Repository navigation
internal/json: replace segmentio/encoding with encoding/json/v2 - #1287
nahapetyan-serob wants to merge 7 commits into
Conversation
5818f09 to
d0941b2
Compare
| return err | ||
| } | ||
| return NewDecoder(bytes.NewReader(data)).Decode(v) | ||
| return jsonv2.Unmarshal(data, v, caseSensitiveV1) |
There was a problem hiding this comment.
why not using the Decoder also in this case? In this way the behavior is 100% not changed
There was a problem hiding this comment.
Right. Will go back to using the Decoder.
…rotocol/go-sdk into serob/use-encoding-json-v2
|
Hi @nahapetyan-serob, thanks for this. Dropping the segmentio dependency would be a nice simplification. I noticed one interaction with a change that landed after this PR was opened, and wanted to flag it before it merges. Since #1401, A minimal check, with this branch merged into m, err := jsonrpc2.DecodeMessage([]byte(`{"jsonrpc":"2.0","id":1,"method":"tools/list","Method":"tools/call"}`))
// m.(*jsonrpc2.Request).Method is "tools/call"; on main it is "tools/list".
The merge is clean and the existing case-sensitivity tests only cover On how the two PRs relate: #1454 keeps the current dependency and only changes how |
|
Thanks @raveesh-me, good catch, confirmed. It's fixed on this branch: |
…#1454) `internal/json.Unmarshal` decoded through segmentio's streaming `json.Decoder`, which copies its input into a read buffer of at least 32 KiB on every call. Unmarshal runs several times per request, so these buffers made up most of the memory allocated to serve a tool call (528 KiB per call in `BenchmarkStreamableServing`). Details in #1453. This change decodes in place: - `json.Parse` into a `json.RawMessage` with `DontCopyRawMessage` finds the extent of the first JSON value. It's the same `parseValue` the Decoder runs on its buffer. - That value is then decoded with the same flags as before. - If finding the value fails, the input is malformed and goes through a `Decoder` as before. Errors are therefore exactly today's: `io.EOF` for empty input, `io.ErrUnexpectedEOF` for truncated input, and the same `SyntaxError` and `UnmarshalTypeError` text. Case-sensitive matching and the depth limit are unchanged. One function, `newDecoder`, configures the Decoder, and `parseFlags` gives `json.Parse` the same rules. `TestUnmarshalSameAsStream` and `FuzzUnmarshalSameAsStream` compare the new path with the old one for `Unmarshal` and `UnmarshalUseNumber` into several target types, checking the decoded value, the error type and the error text. Locally the fuzzer ran about 5 million inputs without finding a difference. `TestUnmarshalDoesNotAliasInput` checks that decoded values never share memory with the input, since the value is now found in place and callers may reuse their buffers. `TestUnmarshalUseNumberCaseSensitivity` covers `UnmarshalUseNumber`, which decodes every JSON-RPC envelope since #1401; the existing case-sensitivity tests only cover `Unmarshal`. ``` name old time/op new time/op delta StreamableServing 368.7µs ± 2% 219.3µs ± 1% -40.51% Unmarshal 4.015µs ± 1% 1.337µs ± 1% -66.71% name old alloc/op new alloc/op delta StreamableServing 528.3KiB ± 0% 60.1KiB ± 1% -88.62% Unmarshal 37.41KiB ± 0% 5.39KiB ± 0% -85.59% ``` Run with `go test ./...`, `go test -race ./...`, `go vet ./...`, staticcheck and `go generate ./...` on Go 1.26.6, and `internal/json` and `mcp` on Go 1.25.0. This covers every call site, including the one #1395 changes, and doesn't conflict with #1287. Fixes #1453 Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
segmentio/encodingwas added in #807 for one method:DontMatchCaseInsensitiveStructFields(). Thejson/v2API does strict field-name matching natively, so the dependency isn't needed.Behavior is unchanged.
DefaultOptionsV1()keeps all v1 semantics — duplicate names permitted, invalid UTF-8 replaced, v1 null/merge handling, any-length arrays — andMatchCaseInsensitiveNames(false)overrides the one we don't want. No v2 strictness is adopted here.internal/json/json_test.gois untouched and passes as-is.UnmarshalUseNumber stays on the v1 decoder and remains case-insensitive: no public API combines UseNumber with strict matching.
go-json-experiment/jsonhosts a mirror ofencoding/json/v2with thegoexperiment.jsonv2build tag (which is enabled by default on Go 1.27)Breaking: go.mod goes 1.25.0 → 1.26 (the dependency declares it); Go 1.25 is no longer supported. CI matrix ["1.25","1.26"] → ["1.26","1.27"], which also covers both of the dependency's build paths.