-
Notifications
You must be signed in to change notification settings - Fork 440
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
base: main
Are you sure you want to change the base?
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
using BenchmarkDotNet.Running; | ||
|
||
BenchmarkSwitcher.FromAssembly(typeof(Program).Assembly).Run(args); |
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; | ||
|
@@ -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() | ||
{ | ||
|
@@ -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> | ||
|
@@ -75,6 +78,48 @@ private protected JsonRpcMessage() | |
[EditorBrowsable(EditorBrowsableState.Never)] | ||
public sealed class Converter : JsonConverter<JsonRpcMessage> | ||
{ | ||
/// <summary> | ||
/// The union to deserialize. | ||
/// </summary> | ||
public 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) | ||
{ | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 What do you want me to do then? Just remove this and rerun benchmarks and provide them in a comment? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
No need to undo any changes yet, but if you could share benchmarks comparing all three versions would be great. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Will do tomorrow (CEST) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eiriktsarpalis I added the benchmark for |
||
|
||
// 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"); | ||
|
Uh oh!
There was an error while loading. Please reload this page.