Add NerdbankMessagePackFormatter #1100
Conversation
- Upgraded several package versions in `Directory.Packages.props`, including `Microsoft.Bcl.AsyncInterfaces`, `System.Collections.Immutable`, `System.IO.Pipelines`, and `System.Text.Json`. Added `Nerdbank.MessagePack`. - Modified `nuget.config` to add a new package source for `nuget.org` and included package source mappings. - Updated `StreamJsonRpc.csproj` to target only `net8.0`, removing `net6.0`, and added a reference to `Nerdbank.MessagePack`. - Changed `Benchmarks.csproj` to target `net8.0` instead of `net6.0`. - Adjusted `StreamJsonRpc.Tests.csproj` to exclusively target `net8.0`.
Replaces the `ProgressFormatterResolver` and `AsyncEnumerableFormatterResolver` with `ProgressConverterResolver` and `AsyncEnumerableConverterResolver`.
Significant modifications to the `NerdbankMessagePackFormatter` and related classes to improve serialization context and type shape provider functionalities. - Introduced new methods in `ISerializationContextBuilder.cs` for registering various type converters, enhancing flexibility for asynchronous enumerables, duplex pipes, and streams. - Replaced `PipeFormatterResolver` with `PipeConverterResolver` in `NerdbankMessagePackFormatter` to align with new converter registration methods. - Updated `FormatterContextBuilder` to utilize new context and type shape provider structures for a more modular design.
src/StreamJsonRpc/NerdbankMessagePackFormatter.ICompositeTypeShapeProviderBuilder.cs
Outdated
Show resolved
Hide resolved
| /// <summary> | ||
| /// Provides a builder interface for adding type shape providers. | ||
| /// </summary> | ||
| public interface ICompositeTypeShapeProviderBuilder |
There was a problem hiding this comment.
Hopefully this whole interface will be unnecessary when eiriktsarpalis/PolyType#91 is fulfilled.
There was a problem hiding this comment.
I simplified this further by removing this interface and merging the methods in the FormatterContectBuilder. But yes, it would be nice to rely on built-in support from PolyType for composing type shape providers.
src/StreamJsonRpc/NerdbankMessagePackFormatter.ISerializationContextBuilder.cs
Outdated
Show resolved
Hide resolved
…rmatterContextBuilder` class. The `NerdbankMessagePackFormatter` class has been modified to eliminate the `NBMP` namespace prefix, favoring direct usage of `MessagePack` types.
src/StreamJsonRpc/NerdbankMessagePackFormatter.FormatterContextBuilder.cs
Outdated
Show resolved
Hide resolved
|
@AArnott : Current test stats: 2157 Passed |
|
Current test results: @AArnott - Currently seeing a lot of test failures around exceptions and having difficulty tracing the issues. Many errors are caused by an NB.MP throwing b/c an exception is not serializable. While adding additional formatter configuration for the tests to pass, I'm seeing some The need to pre-register converters in the serializer without the ability to resolve and cache at runtime like legacy MessagePack-CSharp resolvers, is currently the cause of the most repeated builder calls. I suppose we could make a converter act similar to a resolver, or make some converters more open to supporting a wider set of types? Edit |
|
Benchmarks:
|
Seeing the exception type, message and stack trace usually helps me understand and suggest fixes for issues like this.
FYI I haven't looked closely at the core of your pull request yet, since I'm trying to stay 'OOF' 😉 so I can't make a firm recommendation at this point.
I recently removed the exception that is thrown when you register more converters after serialization has happened. But it still resets the converter cache so it's a runtime hit that should be avoided if possible. Is your concern mostly around generics? I'm still working on a better solution for that from NB.MP that I hope will make this simpler for you.
Historically, separating envelope from user data has been useful in protecting the integrity of the JSON-RPC protocol and our extensions, while allowing the user to have flexibility in how types are serialized. If we have just one serializer object, it seems the user will either be disallowed to customize how types are serialized that we internally must serialize, or the user's customizations could invalidate the objects we serialize for the envelope. And this set of sensitive types may change over time as we change what or how we serialize objects as part of the core protocol or the exotic types. If I'm missing the mark of what you're thinking of, please correct me.
Most of those look pretty good. The last one seems to mix in making the connection itself, which seems likely to be noisy. But I have no idea why NB.MP would make the process of connecting so much slower than the other libraries. Do you? |
Totally makes sense.
Mostly, yes. With MP-CS resolvers, the right formatter is found by working through an ordered list of resolvers for any given type. This allows for behaviors like a resolver intercepting intrinsic rpc-marhalables before they matched to a more conventional formatter. Same for things like Pipes and Streams as arguments or return types. And IAsyncEnumerable as a property on a class. With NB.MP, a type like Basically, not being able to intercept the exotic types as they are seen (for best perf) means the StreamJsonRpc consumer needs to be very explicit up-front.
Not yet :-)
Totally understandable. I should probably step away from this myself for awhile. |
|
Latest Test Results: |
Updated serialization and deserialization processes in the NerdbankMessagePackFormatter to utilize ReadOnlySequence<byte> instead of RawMessagePack. Introduced new classes for handling enumerator results and enhanced comments for clarity. Updated tests to ensure compatibility with the new serialization methods and added data contract attributes for better interoperability.
|
Latest Test Results: |
Added new type shape providers and registered exception types in the NerdbankMessagePackFormatter class. Updated AddTypeShapeProvider to initialize with a capacity of 3. Removed methods for duplex pipes and streams, reflecting a shift in supported types. Registered new converters for handling exceptions like RemoteInvocationException. Updated CommonErrorData with shape generation attributes and modified test classes to align with these changes.
|
Test Results: 2275 Passed |
Latest benchmarks hint that there is a correlation between time and number of serializer runtime register calls. In the latest commit, many more converters and new sub type maps are added in the ctor of NerdbankMessagePackFormatter. The time could also be a result of the calls to converter resolvers. |
|
Does this benchmark include the RegisterConverter calls themselves? If so, we should probably isolate that in a "connection creation" benchmark, leaving a benchmark that measures only the incremental cost of an RPC call. |
No description provided.