perf(dotnet): avoid redundant JSON event materialization - #2733
Conversation
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>
There was a problem hiding this comment.
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
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.
This comment has been minimized.
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>
This comment has been minimized.
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>
SDK Consistency ReviewScope of PR #2733: Only Nature of the change: This is an internal bug fix in the .NET
Cross-SDK assessment: This class of bug is specific to .NET's
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.
|

Inbound session events currently clone the complete parsed JSON-RPC message, deserialize the event as a
JsonElement, allocate a JSON string withGetRawText(), 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
JsonDocumentis alive and materializes handler arguments before asynchronous dispatch can outlive that document. The session-event handler now receivesSessionEventdirectly, 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:
tsxis not installed; its first run reported fixture startup failures for that missing tool.Generated by Copilot