Serialization Simplification #171

mtmk · 2023-10-26T18:47:38Z

https://github.com/nats-io/nats.net.v2/blob/serialization-proposal/docs/documentation/serialization.md

Simplified INatsSerializer interface (using IBufferWriter)
Replace default JSON serializer with JSON context aware serializers
Enable trim warnings (AOT)
Native AOT test
Serialization docs

resolves #92 - NativeAOT support
resolves #137 - Reconsider serialization approach

IBufferWriter is part of the standard runtime and since we're not using the property introduced by our interface ICountableBuffer (which extends IBufferWriter)there is no reason to keep that dependency on the public interface.

* Simplified INatsSerializer a little more. (Serialize method's return value wasn't used) * Removed the default JSON serializer which was serializing any object using reflection. Reflection isn't possible with native AOT. This also makes serialization more explicit. * Added a UTF8 primitives serializer in the default serializer chain to cover simple use cases like sending strings or ints. * Added JSON serializer contexts for JS, Obj and Services models.

jasper-d

I've only taken a quick look because I'm vacationing and have unfortunately little time right now. Left a few comments but nothing major.

I think this is an improvement. But I'm still not convinced that the non-generic ISerializer is the best interface, both in terms of discoverability of the serialization API and performance.

src/NATS.Client.Core/INatsSerializer.cs

jasper-d · 2023-10-30T21:47:23Z

src/NATS.Client.Core/INatsSerializer.cs

+
+        var span = bufferWriter.GetSpan(128);
+
+        // int


How were the supported types chosen? int and double seem kinda arbitrary.

Somewhat is. Ideally I wanted to support all types Utf8Parser supports.

That is a neat idea, I like the idea of supporting all of the Utf8Formatter/Parser types

jasper-d · 2023-10-30T21:48:46Z

src/NATS.Client.Core/INatsSerializer.cs

+            throw new NatsException($"Can't serialize {typeof(T)}");
+        }
+
+        Next.Serialize(bufferWriter, value);


What's the use case for this chaining? To have composition support for serializers I presume?

Yes, composition and reuse. Though I realized it doesn't need to be enforced by the interface. So I removed the Next property from the interface and it really is an optional convention now.

jasper-d · 2023-10-30T21:52:52Z

src/NATS.Client.Core/INatsSerializer.cs

    {
-        Utf8JsonWriter writer;
-        if (_jsonWriter == null)
+        if (_context.GetTypeInfo(typeof(T)) is JsonTypeInfo<T> jsonTypeInfo)


A minor downside of the non-generic INatsSerializer is that it requires a dictionary lookup for the JsonTypeInfo<T> as opposed to a generic serializer where the required type info object would be known.

We could have a generic interface where we chain multiple types when used in connection options.

I gave this a go but there is way too much change. I'll leave it to another PR.

src/NATS.Client.Core/Internal/FixedArrayBufferWriter.cs

src/NATS.Client.Core/NatsBufferWriter.cs

Co-authored-by: Jasper <[email protected]>

mtmk · 2023-10-30T23:49:29Z

Thank you @jasper-d very helpful. Already commited your suggestions. Will have a look at your other comments tomorrow. Have a nice vacation 🍹

@jasper-d

Also removed method inlining on getters (based on Jasper's advice: Presumably JIT should always inline simple field accesses and the other getters look like they are likely inlined without AggressiveInlining when on a hot path.) Thank you @jasper-d

Note that chaining serializers is implemented by convention and doesn't need to be enforced by the INatsSerializer interface since the next serializer would not be exposed to external users of the interface.

caleblloyd

I like the approach!

What about people who don't care about trimming/AOT? Should we offer a regular JSON Serializer that doesn't depend on defining a JSON Context? Could that be in the same assembly or would we need to move it out to another assembly that does not have <IsTrimmable>true</IsTrimmable>

caleblloyd · 2023-10-31T13:45:49Z

src/NATS.Client.Core/INatsSerializer.cs

+
+        var span = bufferWriter.GetSpan(128);
+
+        // int


That is a neat idea, I like the idea of supporting all of the Utf8Formatter/Parser types

mtmk · 2023-10-31T14:03:55Z

would we need to move it out to another assembly that does not have <IsTrimmable>true</IsTrimmable>

I think so. NATS.Client.Core.Serializers.Json or something?

caleblloyd · 2023-10-31T14:10:01Z

I think so. NATS.Client.Core.Serializers.Json or something?

Yes, if it would introduce problems with AOT we should definitely isolate it somewhere like that

All primitives supported by Utf8Parser and Utf8Formatter are implemented.

caleblloyd

LGTM!

mtmk added 7 commits October 26, 2023 19:20

NATS buffer writer

b987f00

Example native AOT project

b9398db

GH workflow with .NET 8.x

31ca589

Simplify serializer interface with IBufferWriter

1c050d2

IBufferWriter is part of the standard runtime and since we're not using the property introduced by our interface ICountableBuffer (which extends IBufferWriter)there is no reason to keep that dependency on the public interface.

Context JSON serializer for native AOT

2c43029

Updated working example using native AOT

bd84448

Merge branch 'main' into serialization-proposal

a0b2e7e

mtmk self-assigned this Oct 27, 2023

mtmk added 11 commits October 27, 2023 16:00

Enabled project trim warnings

a68be08

Merge branch 'main' into serialization-proposal

e035240

Native AOT tests

e577da9

Serialization docs

2b69bea

Reverted test debug code

2faa786

Reverted test debug code

49be4db

Project naming fix

81e410c

Fixed warnings and format

1f819ab

Reverted test debug code

a2ba2cd

Reverted release workflow

101fd9f

mtmk changed the title ~~[WIP] Serialization proposal~~ Serialization Simplification Oct 30, 2023

mtmk marked this pull request as ready for review October 30, 2023 20:50

mtmk requested a review from caleblloyd October 30, 2023 20:50

mtmk mentioned this pull request Oct 30, 2023

Reconsider serialization approach #137

Closed

mtmk added 2 commits October 30, 2023 21:52

Additional serializer docs

faac0a1

Tweaked Native AOT checks

7af8987

jasper-d reviewed Oct 30, 2023

View reviewed changes

mtmk and others added 3 commits October 30, 2023 23:32

Update src/NATS.Client.Core/INatsSerializer.cs

397d613

Co-authored-by: Jasper <[email protected]>

Update src/NATS.Client.Core/Internal/FixedArrayBufferWriter.cs

39b3ce3

Co-authored-by: Jasper <[email protected]>

Update src/NATS.Client.Core/NatsBufferWriter.cs

292e31f

Co-authored-by: Jasper <[email protected]>

mtmk added 3 commits October 31, 2023 00:07

Fixed build

27beaf0

Removed unused buffer interface

3625e2f

Also removed method inlining on getters (based on Jasper's advice: Presumably JIT should always inline simple field accesses and the other getters look like they are likely inlined without AggressiveInlining when on a hot path.) Thank you @jasper-d

Remove next serializer from the interface

de4a3d7

Note that chaining serializers is implemented by convention and doesn't need to be enforced by the INatsSerializer interface since the next serializer would not be exposed to external users of the interface.

caleblloyd reviewed Oct 31, 2023

View reviewed changes

mtmk added 3 commits October 31, 2023 15:58

All primitives serialization implemented

54db6af

All primitives supported by Utf8Parser and Utf8Formatter are implemented.

JSON serializer for non AOT

efbae94

Renamed NATS.Client.Serializers.Json package

9d720ef

caleblloyd approved these changes Oct 31, 2023

View reviewed changes

mtmk merged commit a000cb0 into main Oct 31, 2023
9 checks passed

mtmk deleted the serialization-proposal branch October 31, 2023 19:53

mtmk mentioned this pull request Nov 7, 2023

Release prep for 2.0.0-beta.0 #183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialization Simplification #171

Serialization Simplification #171

mtmk commented Oct 26, 2023 •

edited

Loading

jasper-d left a comment

jasper-d Oct 30, 2023

mtmk Oct 31, 2023

caleblloyd Oct 31, 2023

jasper-d Oct 30, 2023

mtmk Oct 31, 2023

jasper-d Oct 30, 2023

mtmk Oct 30, 2023

mtmk Oct 31, 2023

mtmk commented Oct 30, 2023

caleblloyd left a comment •

edited

Loading

caleblloyd Oct 31, 2023

mtmk commented Oct 31, 2023

caleblloyd commented Oct 31, 2023

caleblloyd left a comment

Serialization Simplification #171

Serialization Simplification #171

Conversation

mtmk commented Oct 26, 2023 • edited Loading

jasper-d left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk commented Oct 30, 2023

caleblloyd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtmk commented Oct 31, 2023

caleblloyd commented Oct 31, 2023

caleblloyd left a comment

Choose a reason for hiding this comment

mtmk commented Oct 26, 2023 •

edited

Loading

caleblloyd left a comment •

edited

Loading