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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,8 @@ docs/api

# Rider
.idea/
.idea_modules/
.idea_modules/

# Benchmarkdotnet

benchmarks/ModelContextProtocol.Benchmarks/BenchmarkDotNet.Artifacts/
1 change: 1 addition & 0 deletions Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -79,5 +79,6 @@
<PackageVersion Include="xunit.runner.visualstudio" Version="3.1.1" />
<PackageVersion Include="System.Net.Http" Version="4.3.4" />
<PackageVersion Include="JsonSchema.Net" Version="7.3.4" />
<PackageVersion Include="BenchmarkDotNet" Version="0.13.12" />
</ItemGroup>
</Project>
3 changes: 3 additions & 0 deletions ModelContextProtocol.slnx
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,7 @@
<Project Path="tests/ModelContextProtocol.TestServer/ModelContextProtocol.TestServer.csproj" />
<Project Path="tests/ModelContextProtocol.TestSseServer/ModelContextProtocol.TestSseServer.csproj" />
</Folder>
<Folder Name="/benchmarks/">
<Project Path="benchmarks/ModelContextProtocol.Benchmarks/ModelContextProtocol.Benchmarks.csproj" />
</Folder>
</Solution>
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
using System.Text.Json;
using System.Text.Json.Nodes;
using BenchmarkDotNet.Attributes;
using ModelContextProtocol.Protocol;

namespace ModelContextProtocol.Benchmarks;

[MemoryDiagnoser]
public class JsonRpcMessageSerializationBenchmarks
{
private byte[] _requestJson = null!;
private byte[] _notificationJson = null!;
private byte[] _responseJson = null!;
private byte[] _errorJson = null!;

private JsonSerializerOptions _options = null!;

[GlobalSetup]
public void Setup()
{
_options = McpJsonUtilities.DefaultOptions;

_requestJson = JsonSerializer.SerializeToUtf8Bytes(
new JsonRpcRequest
{
Id = new RequestId("1"),
Method = "test",
Params = JsonValue.Create(1)
},
_options);

_notificationJson = JsonSerializer.SerializeToUtf8Bytes(
new JsonRpcNotification
{
Method = "notify",
Params = JsonValue.Create(2)
},
_options);

_responseJson = JsonSerializer.SerializeToUtf8Bytes(
new JsonRpcResponse
{
Id = new RequestId("1"),
Result = JsonValue.Create(3)
},
_options);

_errorJson = JsonSerializer.SerializeToUtf8Bytes(
new JsonRpcError
{
Id = new RequestId("1"),
Error = new JsonRpcErrorDetail { Code = 42, Message = "oops" }
},
_options);
}

[Benchmark]
public JsonRpcMessage DeserializeRequest() =>
JsonSerializer.Deserialize<JsonRpcMessage>(_requestJson, _options)!;

[Benchmark]
public JsonRpcMessage DeserializeNotification() =>
JsonSerializer.Deserialize<JsonRpcMessage>(_notificationJson, _options)!;

[Benchmark]
public JsonRpcMessage DeserializeResponse() =>
JsonSerializer.Deserialize<JsonRpcMessage>(_responseJson, _options)!;

[Benchmark]
public JsonRpcMessage DeserializeError() =>
JsonSerializer.Deserialize<JsonRpcMessage>(_errorJson, _options)!;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>net9.0</TargetFramework>
<ImplicitUsings>enable</ImplicitUsings>
<Nullable>enable</Nullable>
<IsPackable>false</IsPackable>
</PropertyGroup>

<ItemGroup>
<PackageReference Include="BenchmarkDotNet" />
</ItemGroup>

<ItemGroup>
<ProjectReference Include="..\..\src\ModelContextProtocol.Core\ModelContextProtocol.Core.csproj" />
</ItemGroup>

</Project>
3 changes: 3 additions & 0 deletions benchmarks/ModelContextProtocol.Benchmarks/Program.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
using BenchmarkDotNet.Running;

BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args);
3 changes: 3 additions & 0 deletions src/ModelContextProtocol.Core/McpJsonUtilities.cs
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,9 @@ internal static bool IsValidMcpToolSchema(JsonElement element)
[JsonSerializable(typeof(JsonRpcNotification))]
[JsonSerializable(typeof(JsonRpcResponse))]
[JsonSerializable(typeof(JsonRpcError))]

// JSON-RPC union to make it faster to deserialize messages
[JsonSerializable(typeof(JsonRpcMessage.Converter.Union))]

// MCP Notification Params
[JsonSerializable(typeof(CancelledNotificationParams))]
Expand Down
4 changes: 3 additions & 1 deletion src/ModelContextProtocol.Core/Protocol/JsonRpcError.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ namespace ModelContextProtocol.Protocol;
/// </remarks>
public sealed class JsonRpcError : JsonRpcMessageWithId
{
internal const string ErrorPropertyName = "error";

/// <summary>
/// Gets detailed error information for the failed request, containing an error code,
/// message, and optional additional data
/// </summary>
[JsonPropertyName("error")]
[JsonPropertyName(ErrorPropertyName)]
public required JsonRpcErrorDetail Error { get; init; }
}
99 changes: 78 additions & 21 deletions src/ModelContextProtocol.Core/Protocol/JsonRpcMessage.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
using ModelContextProtocol.Server;
using System.ComponentModel;
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Text.Json.Serialization;

namespace ModelContextProtocol.Protocol;
Expand All @@ -16,6 +17,8 @@ namespace ModelContextProtocol.Protocol;
[JsonConverter(typeof(Converter))]
public abstract class JsonRpcMessage
{
private const string JsonRpcPropertyName = "jsonrpc";

/// <summary>Prevent external derivations.</summary>
private protected JsonRpcMessage()
{
Expand All @@ -25,7 +28,7 @@ private protected JsonRpcMessage()
/// Gets the JSON-RPC protocol version used.
/// </summary>
/// <inheritdoc />
[JsonPropertyName("jsonrpc")]
[JsonPropertyName(JsonRpcPropertyName)]
public string JsonRpc { get; init; } = "2.0";

/// <summary>
Expand Down Expand Up @@ -75,6 +78,48 @@ private protected JsonRpcMessage()
[EditorBrowsable(EditorBrowsableState.Never)]
public sealed class Converter : JsonConverter<JsonRpcMessage>
{
/// <summary>
/// The union to deserialize.
/// </summary>
internal struct Union
{
/// <summary>
/// <see cref="JsonRpcMessage.JsonRpc"/>
/// </summary>
[JsonPropertyName(JsonRpcPropertyName)]
public string JsonRpc { get; set; }

/// <summary>
/// <see cref="JsonRpcMessageWithId.Id"/>
/// </summary>
[JsonPropertyName(JsonRpcMessageWithId.IdPropertyName)]
public RequestId Id { get; set; }

/// <summary>
/// <see cref="JsonRpcRequest.Method"/>
/// </summary>
[JsonPropertyName(JsonRpcRequest.MethodPropertyName)]
public string? Method { get; set; }

/// <summary>
/// <see cref="JsonRpcRequest.Params"/>
/// </summary>
[JsonPropertyName(JsonRpcRequest.ParamsPropertyName)]
public JsonNode? Params { get; set; }

/// <summary>
/// <see cref="JsonRpcError.Error"/>
/// </summary>
[JsonPropertyName(JsonRpcError.ErrorPropertyName)]
public JsonRpcErrorDetail? Error { get; set; }

/// <summary>
/// <see cref="JsonRpcResponse.Result"/>
/// </summary>
[JsonPropertyName(JsonRpcResponse.ResultPropertyName)]
public JsonNode? Result { get; set; }
}

/// <inheritdoc/>
public override JsonRpcMessage? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
{
Expand All @@ -83,51 +128,63 @@ public sealed class Converter : JsonConverter<JsonRpcMessage>
throw new JsonException("Expected StartObject token");
}

using var doc = JsonDocument.ParseValue(ref reader);
var root = doc.RootElement;
var union = JsonSerializer.Deserialize(ref reader, options.GetTypeInfo<Union>());

// All JSON-RPC messages must have a jsonrpc property with value "2.0"
if (!root.TryGetProperty("jsonrpc", out var versionProperty) ||
versionProperty.GetString() != "2.0")
if (union.JsonRpc != "2.0")
{
throw new JsonException("Invalid or missing jsonrpc version");
}

// Determine the message type based on the presence of id, method, and error properties
bool hasId = root.TryGetProperty("id", out _);
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eiriktsarpalis I added the benchmark for JsonDocument.Deserialize in the PR description. My machine seems to be sluggish a bit today, but still, the union approach seems to be a winner. As this is what is executed a lot, I'd still vote for the union approach.


// Messages with an id but no method are responses
if (hasId && !hasMethod)
if (union.Id.HasValue && union.Method is null)
{
// Messages with an error property are error responses
if (hasError)
if (union.Error != null)
{
return JsonSerializer.Deserialize(rawText, options.GetTypeInfo<JsonRpcError>());
return new JsonRpcError
{
Id = union.Id,
Error = union.Error,
JsonRpc = union.JsonRpc,
};
}

// Messages with a result property are success responses
if (root.TryGetProperty("result", out _))
if (union.Result != null)
{
return JsonSerializer.Deserialize(rawText, options.GetTypeInfo<JsonRpcResponse>());
return new JsonRpcResponse
{
Id = union.Id,
Result = union.Result,
JsonRpc = union.JsonRpc,
};
}

throw new JsonException("Response must have either result or error");
}

// Messages with a method but no id are notifications
if (hasMethod && !hasId)
if (union.Method != null && !union.Id.HasValue)
{
return JsonSerializer.Deserialize(rawText, options.GetTypeInfo<JsonRpcNotification>());
return new JsonRpcNotification
{
Method = union.Method,
JsonRpc = union.JsonRpc,
Params = union.Params,
};
}

// Messages with both method and id are requests
if (hasMethod && hasId)
if (union.Method != null && union.Id.HasValue)
{
return JsonSerializer.Deserialize(rawText, options.GetTypeInfo<JsonRpcRequest>());
return new JsonRpcRequest
{
Id = union.Id,
Method = union.Method,
JsonRpc = union.JsonRpc,
Params = union.Params,
};
}

throw new JsonException("Invalid JSON-RPC message format");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ namespace ModelContextProtocol.Protocol;
/// </remarks>
public abstract class JsonRpcMessageWithId : JsonRpcMessage
{
internal const string IdPropertyName = "id";

/// <summary>Prevent external derivations.</summary>
private protected JsonRpcMessageWithId()
{
Expand All @@ -25,6 +27,6 @@ private protected JsonRpcMessageWithId()
/// <remarks>
/// Each ID is expected to be unique within the context of a given session.
/// </remarks>
[JsonPropertyName("id")]
[JsonPropertyName(IdPropertyName)]
public RequestId Id { get; init; }
}
7 changes: 5 additions & 2 deletions src/ModelContextProtocol.Core/Protocol/JsonRpcRequest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,19 @@ namespace ModelContextProtocol.Protocol;
/// </remarks>
public sealed class JsonRpcRequest : JsonRpcMessageWithId
{
internal const string MethodPropertyName = "method";
internal const string ParamsPropertyName = "params";

/// <summary>
/// Name of the method to invoke.
/// </summary>
[JsonPropertyName("method")]
[JsonPropertyName(MethodPropertyName)]
public required string Method { get; init; }

/// <summary>
/// Optional parameters for the method.
/// </summary>
[JsonPropertyName("params")]
[JsonPropertyName(ParamsPropertyName)]
public JsonNode? Params { get; init; }

internal JsonRpcRequest WithId(RequestId id)
Expand Down
4 changes: 3 additions & 1 deletion src/ModelContextProtocol.Core/Protocol/JsonRpcResponse.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@ namespace ModelContextProtocol.Protocol;
/// </remarks>
public sealed class JsonRpcResponse : JsonRpcMessageWithId
{
internal const string ResultPropertyName = "result";

/// <summary>
/// Gets the result of the method invocation.
/// </summary>
/// <remarks>
/// This property contains the result data returned by the server in response to the JSON-RPC method request.
/// </remarks>
[JsonPropertyName("result")]
[JsonPropertyName(ResultPropertyName)]
public required JsonNode? Result { get; init; }
}
5 changes: 5 additions & 0 deletions src/ModelContextProtocol.Core/Protocol/RequestId.cs
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,11 @@ public RequestId(long value)
/// <remarks>This will either be a <see cref="string"/>, a boxed <see cref="long"/>, or <see langword="null"/>.</remarks>
public object? Id => _id;

/// <summary>
/// Returns true if the underlying id is set.
/// </summary>
public bool HasValue => _id != null;

/// <inheritdoc />
public override string ToString() =>
_id is string stringValue ? stringValue :
Expand Down
Loading