refactor: Use sonic for JSON un/marshaling#3588
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3588 +/- ##
==========================================
+ Coverage 75.81% 76.00% +0.18%
==========================================
Files 387 388 +1
Lines 34975 35000 +25
==========================================
+ Hits 26518 26600 +82
+ Misses 6595 6530 -65
- Partials 1862 1870 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Refactors the codebase’s JSON handling (especially along the RPC path) to use a new utils/jsonx wrapper backed by bytedance/sonic, aiming to improve JSON encode/decode performance while pinning key behavioral semantics via tests.
Changes:
- Introduce
utils/jsonxas a centralized marshal/unmarshal/decoder wrapper aroundsonic. - Migrate many call sites from
encoding/jsontojsonxacross VM, RPC versions, clients, compiler, and core hashing. - Rework
rpccore.LimitSlicedecoding to use sonic’s lazy AST iteration (plus add/adjust tests for sonic-specific behavior and error message differences).
Reviewed changes
Copilot reviewed 43 out of 44 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vm/vm.go | Switch state diff/trace/receipt JSON (un)marshal calls to jsonx. |
| vm/transaction.go | Switch transaction envelope marshaling to jsonx. |
| vm/trace.go | Switch custom MarshalJSON implementation to jsonx. |
| vm/class.go | Switch class info marshaling to jsonx. |
| utils/jsonx/jsonx.go | Add sonic-backed JSON helper package (Marshal, Unmarshal, Decoder, etc.). |
| utils/jsonx/jsonx_test.go | Add tests pinning jsonx semantics (UseNumber, RawMessage, ordering, decoder surface, malformed input). |
| starknet/compiler/compiler.go | Switch compiler request/response JSON handling to jsonx. |
| starknet/compiler/compile_ffi.go | Switch FFI JSON handling to jsonx. |
| starknet/class.go | Switch several JSON marshal/unmarshal paths to jsonx while keeping encoding/json types. |
| rpc/v9/transaction.go | Switch gateway payload JSON handling to jsonx. |
| rpc/v9/trace.go | Switch custom MarshalJSON to jsonx. |
| rpc/v9/sync.go | Switch custom MarshalJSON to jsonx. |
| rpc/v9/subscriptions.go | Use jsonx and replace map[string]any params with typed SubscriptionParams for stable field order. |
| rpc/v9/class.go | Switch declared class decode to jsonx. |
| rpc/v9/block.go | Switch BlockID unmarshal to jsonx. |
| rpc/v8/transaction.go | Switch gateway payload JSON handling to jsonx. |
| rpc/v8/trace.go | Switch custom MarshalJSON to jsonx. |
| rpc/v8/sync.go | Switch custom MarshalJSON to jsonx. |
| rpc/v8/subscriptions.go | Use jsonx and replace map[string]any params with typed SubscriptionParams. |
| rpc/v8/class.go | Switch declared class decode to jsonx. |
| rpc/v8/block.go | Switch BlockID unmarshal to jsonx. |
| rpc/v10/transaction_types.go | Switch custom MarshalJSON to jsonx. |
| rpc/v10/transaction_test.go | Add regression test to lock contract_class passthrough behavior with sonic. |
| rpc/v10/transaction.go | Switch gateway payload JSON handling and class decode to jsonx. |
| rpc/v10/trace_invocation.go | Switch custom MarshalJSON to jsonx. |
| rpc/v10/sync.go | Switch custom MarshalJSON to jsonx. |
| rpc/v10/subscriptions.go | Switch subscription response marshaling to jsonx and use typed params. |
| rpc/v10/subscription_types.go | Introduce SubscriptionParams type (shared by v10 subscriptions). |
| rpc/v10/storage.go | Switch response flag decode + response marshaling to jsonx. |
| rpc/v10/simulation.go | Switch conditional response marshaling to jsonx. |
| rpc/v10/response_flags_test.go | Update expected error substring for sonic-driven decode error. |
| rpc/v10/response_flags.go | Switch flags/tags unmarshal to jsonx. |
| rpc/v10/events.go | Switch AddressList unmarshal to jsonx. |
| rpc/v10/block_id.go | Switch BlockID unmarshal to jsonx. |
| rpc/rpccore/limit_slice_test.go | Update tests to use jsonx and add laziness/edge-case coverage for new AST-based decoder. |
| rpc/rpccore/limit_slice.go | Reimplement LimitSlice.UnmarshalJSON using sonic AST lazy iteration to enforce limit early. |
| node/upgrader/upgrader.go | Switch GitHub release JSON decode to jsonx decoder. |
| jsonrpc/server_test.go | Adjust tests to tolerate sonic’s arch-dependent parse error text; add helper to ignore error.data contents where needed. |
| jsonrpc/server.go | Switch decoding/encoding to jsonx, add sonic pretouch compilation, and log pretouch stats on registration. |
| core/class_hash.go | Switch class definition marshaling to jsonx before hashing. |
| clients/gateway/gateway.go | Switch request/response JSON handling to jsonx. |
| clients/feeder/feeder.go | Switch feeder JSON decoding to jsonx decoder and class decode to jsonx. |
| go.mod | Add github.com/bytedance/sonic (and related indirect deps). |
| go.sum | Add checksums for sonic and its transitive dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
here's the benchmarking data: tl;dr: sonic uses less CPU (
|
| //nolint:lll // We can't break the json value | ||
| res: `[{"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid Request","data":"placeholder"},"id":null},{"jsonrpc":"2.0","error":{"code":-32600,"message":"Invalid Request","data":"placeholder"},"id":null}]`, |
| if test.res != "" { | ||
| assert.JSONEq(t, test.res, string(res)) | ||
| if test.ignoreErrorData { | ||
| assertResponseIgnoringErrorData(t, test.res, string(res)) |
There was a problem hiding this comment.
This function seems unnecessary, why aren't the JSONs populated with the right error data?
There was a problem hiding this comment.
tl:dr; AMD and ARM builds produce different error outputs because they use different parsers under the hood, so we do exact quality checks in our tests, they will fail in CI on different archs.
The idea is captured in the function doc comment:
https://github.com/NethermindEth/juno/pull/3588/changes#diff-a2e8c524803b22cc1bb50a1992a011374f1a29e432d04b54c984cb5ec9c7396dR24-R30
| // TestPretouchProductionHandlers registers Juno's real RPC methods | ||
| // (v8, v9, v10) on jsonrpc.Server instances and dumps the resulting | ||
| // PretouchedTypes list. The list is the actual set of Go types that | ||
| // sonic compiled encode/decode paths for at server startup, with no | ||
| // placeholder/stub handlers. | ||
| // | ||
| // Network and storage interfaces (bcReader, syncReader, vm) are passed | ||
| // nil — those are only touched when handlers run, not at registration. | ||
| func TestPretouchProductionHandlers(t *testing.T) { | ||
| logger := log.NewNopZapLogger() |
There was a problem hiding this comment.
This tests looks out of place, it should be in the RPC package
| } | ||
|
|
||
| type SubscriptionParams struct { | ||
| Result any `json:"result"` |
There was a problem hiding this comment.
Why Result can be Any?
Also define the sub type above the parent type
There was a problem hiding this comment.
That's how it's used currently - we pass various types as a Result. Potentially, we could use a generic type here, but I believe it's out of scope for this PR. Will move the type above the parent.
|
Some bench data before/after f1e08fe (local arm64 run)
The bytes regression is sec/op per axis
allocs/op per axis
(no-params, batch, response axes unchanged at 18 / 117 / 888 / 18) cpu.out.zip to access it, use: Preview for one of the tests: 
|
|
Allocations main: sonic: And here's the GC metrics data (as reported by node) before and after running the tests: |
|
CPU data from AMD64 arch running end-to-end tests tl;dr: Un/Marshaling took 11% vs 7% overal cpu time (main vs sonic) or 5.5s vs 3.4s CPU time, so ~70% decrease. main: sonic: |
This PR replaces
encoding/jsonwithsonicfor JSON un/marshaling.Initially, I was planning to add it only for the RPCv10, but to reduce the complexity/conditioning, I decided to migrate all RPC versions.
( ! ) Note: It introduces breaking changes - the error messages (that user may encounter) are differs from what
encoding/jsonwere producing, also, they are different between ARM64 and AMD64 architectures.Changes:
LimitSlicedecoding to use sonic’s lazy AST iteration.TODO: Pretouch logging needs to be removed before merging the PR.