Skip to content

perf(dotnet): avoid redundant JSON event materialization - #2733

Merged
stephentoub merged 3 commits into
mainfrom
stephentoub-net-json-performance
Sep 20, 2026
Merged

stephentoub merged 3 commits into
mainfrom
stephentoub-net-json-performance

Conversation

@stephentoub

Copy link
Copy Markdown
Collaborator

Inbound session events currently clone the complete parsed JSON-RPC message, deserialize the event as a JsonElement, allocate a JSON string with GetRawText(), and parse that string again into generated event types. Large, deeply nested tool payloads make this synchronous read-loop work and allocation traffic significant.

This change routes messages while the owning JsonDocument is alive and materializes handler arguments before asynchronous dispatch can outlive that document. The session-event handler now receives SessionEvent directly, eliminating the full-root clone and string serialization/reparse while preserving response ownership, cancellation, notification recovery, session ordering, unknown-event compatibility, and handler isolation.

A focused local Release benchmark using 256 framed 87,632-byte events with 512 nested rows measured allocated bytes per event dropping from 735,389 to 187,600 (about 74%). Wall time dropped from 695.5 ms to 369.2 ms in the recorded before/after runs, though timing varied with host load.

Validation:

  • Focused JSON-RPC/session-event tests: 7 passed
  • .NET unit tests: 424 passed
  • Full multi-target solution build: passed with no warnings
  • Formatting verification: passed
  • The full E2E-inclusive test command could not run in this environment because tsx is not installed; its first run reported fixture startup failures for that missing tool.

Generated by Copilot

Deserialize inbound session events directly while preserving JSON-RPC ownership, ordering, and malformed-event recovery semantics.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@stephentoub
stephentoub requested a review from a team as a code owner September 19, 2026 17:42
Copilot AI balanced review requested due to automatic review settings September 19, 2026 17:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The JSON element lifetime test reads payloads before document disposal and does not validate the core ownership guarantee.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Optimizes .NET inbound JSON-RPC event handling by avoiding redundant JSON cloning and reparsing.

Changes:

  • Materializes handler arguments before asynchronous dispatch.
  • Deserializes session events directly into generated types.
  • Adds event recovery, isolation, and lifetime tests.
File Description
dotnet/​src/​JsonRpc.cs Routes messages within document lifetime and materializes arguments synchronously.
dotnet/​src/​Client.cs Dispatches deserialized events directly.
dotnet/​test/​Unit/​JsonRpcTests.cs Adds transport lifetime and malformed-event tests.
dotnet/​test/​Unit/​ClientSessionLifetimeTests.cs Tests malformed input recovery and handler isolation.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dotnet/test/Unit/JsonRpcTests.cs
@github-actions

This comment has been minimized.

Retain deserialized elements until the JSON-RPC stream completes, then inspect them after the source documents have been disposed.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

This comment has been minimized.

Prevent null or string response identifiers from terminating the inbound read loop, and verify a subsequent numeric response still completes.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

SDK Consistency Review

Scope of PR #2733: Only .NET files are touched (dotnet/src/Client.cs, dotnet/src/JsonRpc.cs, and corresponding unit tests). No other language SDK (Node.js, Python, Go, Java, Rust) is modified.

Nature of the change: This is an internal bug fix in the .NET JsonRpc read loop, addressing a use-after-dispose/buffer-reuse hazard where a JsonElement (a non-owning view over a pooled/reused byte buffer in System.Text.Json) could be read after the underlying JsonDocument/buffer was recycled by a subsequent read iteration, especially once handler dispatch became asynchronous. The fix:

  • Routes and materializes (clones/deserializes) message data synchronously while the JsonDocument is still alive, before any await point.
  • Deserializes handler arguments eagerly (DeserializeHandlerArguments) prior to invoking async handlers, so no JsonElement values are held across a suspension point.
  • Adds defensive handling for malformed/non-numeric response id values so a single malformed message doesn't take down the read loop.
  • Adds targeted unit tests reproducing the buffer-reuse race and malformed-input resilience.

Cross-SDK assessment: This class of bug is specific to .NET's System.Text.Json JsonElement/JsonDocument model, where JsonElement values returned from JsonDocument.Parse are non-owning references into pooled buffers that get disposed/reused by the read loop. Other SDKs use different memory models for their JSON-RPC read loops:

  • Go (internal/jsonrpc2) and Rust (jsonrpc.rs) deserialize into owned values (serde_json::Value, Go structs/maps) rather than exposing zero-copy views over reused buffers.
  • Python and Node.js rely on garbage-collected, owned objects from their native JSON parsers, so there is no equivalent "buffer reuse under an async boundary" hazard.

No equivalent latent bug pattern was found reproduced in the other SDKs' read loops, and this is not a new public API/feature — it's an internal correctness/robustness fix confined to a single SDK's implementation. Per review guidelines, this is a language-specific fix and does not need to be mirrored elsewhere. ✅ No cross-SDK consistency issues found.

Generated by SDK Consistency Review Agent for #2733 · copilot · sonnet50 · 29.4 AIC · ⌖ 11.5 AIC · ⊞ 7.8K ·

@stephentoub
stephentoub merged commit ca166d3 into main Sep 20, 2026
61 checks passed
@stephentoub
stephentoub deleted the stephentoub-net-json-performance branch September 20, 2026 14:56
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.

2 participants