Skip to content

JsonRpcMessage.Converter.Read optimization #639

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Scooletz
Copy link
Contributor

This PR replaces the copying behavior of JsonRpcMessage.Converter.Read with a single pass deserialization of it. It uses a Union struct that deserializes the raw content of JsonRpcMessage to 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.Read and wanted to make it faster.

How Has This Been Tested?

Benchmarks. Allocations are ~halfed.

Before

Method Mean Error StdDev Gen0 Allocated
DeserializeRequest 1.113 us 0.0167 us 0.0148 us 0.0572 720 B
DeserializeNotification 1.044 us 0.0080 us 0.0062 us 0.0515 664 B
DeserializeResponse 1.053 us 0.0057 us 0.0048 us 0.0515 648 B
DeserializeError 1.333 us 0.0138 us 0.0129 us 0.0954 1200 B

After

Method Mean Error StdDev Gen0 Allocated
DeserializeRequest 528.4 ns 9.85 ns 9.67 ns 0.0324 408 B
DeserializeNotification 443.9 ns 6.79 ns 6.35 ns 0.0305 384 B
DeserializeResponse 472.4 ns 6.77 ns 6.00 ns 0.0291 368 B
DeserializeError 732.5 ns 7.05 ns 6.59 ns 0.0687 864 B

Breaking Changes

Nope

Types of changes

  • Optimization
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

bool hasMethod = root.TryGetProperty("method", out _);
bool hasError = root.TryGetProperty("error", out _);

var rawText = root.GetRawText();
Copy link
Member

Choose a reason for hiding this comment

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

@stephentoub has pointed out that this call is wholly unnecessary. I'm curious what percentage of the performance improvement can be attributed to simply removing this and passing the root directly into JsonSerializer further down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The question that I'd ask, and I might be missing something here, is whether you want to create the root at all? Clearly we'd benefit from not var rawText = root.GetRawText(); and this is one part of the coping. The other is creating JsonDocument that has a few fields and does some parsing. With Union we don't allocate at all beside for the properties that will be used in any case.

What do you want me to do then? Just remove this and rerun benchmarks and provide them in a comment?

Copy link
Member

Choose a reason for hiding this comment

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

The Union approach is clearly the fastest, but it is also the most elaborate. Before we commit to that, I'd like to see how much we can gain by fixing the obvious inefficiencies.

What do you want me to do then? Just remove this and rerun benchmarks and provide them in a comment?

No need to undo any changes yet, but if you could share benchmarks comparing all three versions would be great.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Will do tomorrow (CEST)

@Scooletz Scooletz mentioned this pull request Jul 22, 2025
9 tasks
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