JsonRpcMessage.Converter.Read optimization#639
Closed
Scooletz wants to merge 6 commits intomodelcontextprotocol:mainfrom
Closed
JsonRpcMessage.Converter.Read optimization#639Scooletz wants to merge 6 commits intomodelcontextprotocol:mainfrom
Scooletz wants to merge 6 commits intomodelcontextprotocol:mainfrom
Conversation
Contributor
Author
|
@eiriktsarpalis Could you comment on the perf findings? It looks like it's worth it? |
Member
|
@Scooletz I think we can keep your changes as-is, on the understanding that this only touches the core JSON-RPC infrastructure and is therefore unlikely to require evolution in the future. |
| /// <summary> | ||
| /// The union to deserialize. | ||
| /// </summary> | ||
| internal struct Union |
Member
There was a problem hiding this comment.
@Scooletz I would recommend marking the Union struct as private and hand-craft a static Parse(ref Utf8JsonReader reader) that populates an instance of the type. This should further improve performance while also minimizing the amount of source generated code (aka there wouldn't exist any serialization code for the type).
scutuatua-crypto
approved these changes
Jan 10, 2026
8 tasks
Member
|
Superseded by #1138 |
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
3 participants
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.
This PR replaces the copying behavior of
JsonRpcMessage.Converter.Readwith a single pass deserialization of it. It uses aUnionstruct that deserializes the raw content ofJsonRpcMessageto a bag of properties. Then it uses it to check the conditions and properly construct underlying objects. This makes deserialization ~50% faster and also reduced the allocations.Motivation and Context
I noticed the double deserialization in
JsonRpcMessage.Converter.Readand wanted to make it faster.How Has This Been Tested?
Benchmarks. Allocations are ~halfed.
Before
After
Using JsonDocument.Deserialize
The attempt in using
doc.Deserialize(options.GetTypeInfo<TMessageType>())for deserializingBreaking Changes
Nope
Types of changes
Checklist