Support Native AOT #1348

hwoodiwiss · 2024-02-17T12:35:23Z

This PR is a work in progress to get Native AoT working. Posting early, as I think it'll be good to get eyes on early.

AoT support as added by using a new generic serializer type and serialization factory to create it.

fixes #1347 (eventually)

Things still to do:

Work out properly how to get JsonSerializerOptions injected, not sure we should be adding a new dependency on Microsoft.Extensions.Options
Add better behaviour switching for .NET 8 builds, currenlty, .NET 8 builds will default to AOT compatible behaviour.
Cleanup changes made for local testing
Add general test coverage
Add some sort of testing to validate the case of AOT compiled builds
Bump Version (Major?)

This commit gets AoT working to the point that I can actually test it, and see it work AOT locally

hwoodiwiss · 2024-02-17T12:40:48Z

This might be a bit cleaner to integrate if we look to add .NET 8 as a first-class TFM first

codecov · 2024-02-17T12:47:11Z

Codecov Report

Attention: Patch coverage is 49.36709% with 40 lines in your changes missing coverage. Please review.

Project coverage is 78.70%. Comparing base (54551ab) to head (b74891d).
Report is 149 commits behind head on main.

Files with missing lines	Patch %	Lines
...MessageSerialization/SystemTextJsonSerializer`1.cs	48.21%	26 Missing and 3 partials ⚠️
src/JustSaying/Extensions/JsonElementExtensions.cs	0.00%	7 Missing ⚠️
...g/Fluent/ServiceResolver/DefaultServiceResolver.cs	33.33%	1 Missing and 1 partial ⚠️
...njection.Microsoft/IServiceCollectionExtensions.cs	0.00%	1 Missing ⚠️
...erialization/SystemTextJsonSerializationFactory.cs	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1348      +/-   ##
==========================================
- Coverage   79.43%   78.70%   -0.74%     
==========================================
  Files         138      142       +4     
  Lines        3234     3306      +72     
  Branches      450      462      +12     
==========================================
+ Hits         2569     2602      +33     
- Misses        434      468      +34     
- Partials      231      236       +5

Flag	Coverage Δ
linux	`78.70% <49.36%> (-0.74%)`	⬇️
macos	`47.88% <48.10%> (?)`
windows	`47.79% <48.10%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

martincostello · 2024-02-17T13:09:41Z

Add some sort of testing to validate the case of AOT compiled builds

We use this test project in Polly. In theory compiling it with the assemblies trim-rooted is enough to flush out issues that the analysers can't detect.

Bump Version (Major?)

Unless we have to break stuff, I'd class it just as a minor. I'd a minor planned for #1335 once I've validated it internally, so depending on how long both take to complete, we could have either an 8.2 then an 8.3, or ship them together as 8.2.

src/JustSaying.Extensions.DependencyInjection.Microsoft/IServiceCollectionExtensions.cs

...ons.DependencyInjection.Microsoft/JustSaying.Extensions.DependencyInjection.Microsoft.csproj

src/JustSaying/AwsTools/MessageHandling/PublishException.cs

martincostello · 2024-02-17T13:17:51Z

src/JustSaying/AwsTools/QueueCreation/RedrivePolicy.cs

    public override string ToString()
+#if NET8_0_OR_GREATER
+        => System.Text.Json.JsonSerializer.Serialize(this, JustSayingSerializationContext.Default.RedrivePolicy);


this might be problematic for AoT in case it's a derived type.

Yeah, that's a good point, I think it'll end up serializing the base fields, but none added by the derived type.
At the moment, RedrivePolicy is only used internally, and I can't see a way that an API user could derive it and pass it through to any internal usage, but I could be missing it.

If that is the case, I don't think it would be a bad idea to internal sealed it.

martincostello · 2024-02-17T13:19:25Z

src/JustSaying/Fluent/ServiceResolver/DefaultServiceResolver.cs

+            if (RuntimeFeature.IsDynamicCodeSupported)
+            {
+                #pragma warning disable IL2026
+                #pragma warning disable IL3050
+                return new NewtonsoftSerializationFactory();
+                #pragma warning restore
+            }


Slightly surprised it isn't smart enough to not warn about this.

I'll check, I think I had the check backwards when I added the #pragma warning disable's

Yeah, even with that fixed, it still warns.

I've found an issue related to this: dotnet/runtime#97273, looks like the supression in runtime checks just didn't make it into .NET 8, not sure if they plan to backport either.

It also looks like there are plans for a wider design proposal to support feature checks suppressing analyzers: dotnet/runtime#96859

src/JustSaying/JustSayingSerializationContext.cs

martincostello · 2024-02-17T13:22:21Z

src/JustSaying/Messaging/MessageSerialization/SystemTextJsonSerializer`1.cs

+            var dataType = obj.Value.GetProperty("Type").GetString();
+            var dataValue = obj.Value.GetProperty("Value").GetString();


We should make these defensive against the possibility of the properties not being there so we don't NRE.

src/JustSaying/Messaging/MessageSerialization/SystemTextJsonSerializer`1.cs

martincostello · 2024-02-17T13:23:28Z

src/JustSaying/Messaging/MessageSerialization/TypedSystemTextJsonSerializationFactory.cs

+
+namespace JustSaying.Messaging.MessageSerialization;
+
+public class TypedSystemTextJsonSerializationFactory(JsonSerializerOptions options) : IMessageSerializationFactory


Not the biggest fan of the Typed prefix.

Yeah, same, I was at the "get it working at any cost" stage by then. I'll try and think of something a bit sleaker, or merge this back in with the current STJSerializationFactory, and have it use the typed serializer when dynamic code isn't supported.

If we go this way, in the next breaking version, I think it'll make sense to deprecate the non-typed serializer, and to that end, maybe decorate it now.

martincostello · 2024-02-17T13:24:42Z

src/JustSaying/Extensions/JsonSerializerOptionsExtensions.cs

+        var typeInfo = options.GetTypeInfo(typeof(T));
+        if (typeInfo is not JsonTypeInfo<T> genericTypeInfo)
+        {
+            throw new JsonException($"Could not find type info for the specified type {typeof(T).Name}");


This could probably be a bit more informative.

It ends up being redundant anyway tbh, GetTypeInfo(Type) throws if it can't find the type info anyway.

hwoodiwiss · 2024-02-17T14:22:26Z

Unless we have to break stuff, I'd class it just as a minor. I'd a minor planned for #1335 once I've validated it internally, so depending on how long both take to complete, we could have either an 8.2 then an 8.3, or ship them together as 8.2.

That makes sense. It makes sense to me that we should add the net8.0 tfm before this, in part just to reduce the overall size/complexity of this change, so maybe we could do:
8.2 - Batch Publish and net8.0 tfm
8.3 - AOT Support if it doesn't require breaking changes.

(Pushing rather than stashing)

slang25 · 2024-02-18T16:19:39Z

This is looking very nice 🙂 I had looked at this a while back in a local branch and the thing that I was concerned about was the JsonStringEnumConverter behaviour, which in an AOT environment will need to be added to the source generated serializer per-enum using JsonStringEnumConverter<TEnum>, which is fine but I think something that will be easily missed by consumers.

hwoodiwiss · 2024-02-18T18:52:48Z

This is looking very nice 🙂 I had looked at this a while back in a local branch and the thing that I was concerned about was the JsonStringEnumConverter behaviour, which in an AOT environment will need to be added to the source generated serializer per-enum using JsonStringEnumConverter<TEnum>, which is fine but I think something that will be easily missed by consumers.

Yeah, I'd agree, I think this is a known enough sharp edge of AOT compilation, that we could just guard the addition of the default JsonStringEnumConverter with RuntimeFeature.IsDynamicCodeSupported, I'm not sure if there's a better way that we could surface a warning around this to API consumers.

slang25 · 2024-02-18T22:30:37Z

Another thought I had while browsing this change, now might be the time to create separate interfaces and implementation for the "message envelope" and message serializer/deserializer.

src/JustSaying/Messaging/MessageSerialization/JustSayingJsonSerializerOptions.cs

hwoodiwiss · 2024-02-20T17:32:50Z

Another thought I had while browsing this change, now might be the time to create separate interfaces and implementation for the "message envelope" and message serializer/deserializer.

Yeah, I had originally thought this change would require doing that anyway but found a way to avoid it.
I think I'd prefer a v1 of AoT support to not alter the public API, at the cost of ergonomics for the AoT use-case, then we can look at building first-class support as part of the API changes that come for v8.

…uce API change

hwoodiwiss · 2024-02-28T08:47:45Z

src/JustSaying/Messaging/MessageSerialization/NewtonsoftSerializer.cs

+#if NET8_0_OR_GREATER
+[RequiresUnreferencedCode(Constants.SerializationUnreferencedCodeMessage)]
+[RequiresDynamicCode(Constants.SerializationDynamicCodeMessage)]
+#endif
 public class NewtonsoftSerializer : IMessageSerializer


This is an assumption at the moment. Newtonsoft.Json isn't currently annotated for AOT/Trimming compatibility.

src/JustSaying/Extensions/JsonSerializerOptionsExtensions.cs

src/JustSaying/Messaging/MessageSerialization/SystemTextJsonSerializer.cs

Co-authored-by: Martin Costello <[email protected]>

TODO: Make test names/namespaces not the worst

hwoodiwiss added 2 commits February 16, 2024 22:32

Initial work to get Native AOT working

84f51ac

Get AoT working

0a7db70

This commit gets AoT working to the point that I can actually test it, and see it work AOT locally

hwoodiwiss changed the title ~~Support Native Aot~~ Support Native AOT Feb 17, 2024

Fix inverted DynamicCodeSupported check

74ed1f0

hwoodiwiss added enhancement dependencies Pull requests that update a dependency file .NET Pull requests that update .net code labels Feb 17, 2024

martincostello reviewed Feb 17, 2024

View reviewed changes

Some changes per review

68d150e

(Pushing rather than stashing)

hwoodiwiss mentioned this pull request Feb 17, 2024

Add .NET 8 TFM #1349

Merged

Merge branch 'main' into hwoodiwiss-native-aot

067b3a9

martincostello reviewed Feb 20, 2024

View reviewed changes

src/JustSaying/Messaging/MessageSerialization/JustSayingJsonSerializerOptions.cs Outdated Show resolved Hide resolved

Fix merge issue

0f4cb43

hwoodiwiss added 7 commits February 22, 2024 09:37

Add unreferenced code annotations to JS.E.AWS

61aa172

WIP: Some restructuring to make usage a little more seamless, and red…

f05572c

…uce API change

Get message attributes more defensively

773f7fc

Start undoing behavioural changes

4334fe7

Remove removed type from public API

ffa2d31

Update MEDI annotations

470e3fe

Shift specific serializer options to sample

0cf53a3

hwoodiwiss commented Feb 28, 2024

View reviewed changes

martincostello reviewed Feb 28, 2024

View reviewed changes

src/JustSaying/Extensions/JsonSerializerOptionsExtensions.cs Outdated Show resolved Hide resolved

src/JustSaying/Messaging/MessageSerialization/SystemTextJsonSerializer.cs Outdated Show resolved Hide resolved

hwoodiwiss and others added 6 commits February 28, 2024 09:38

Update obsolete comment

bb28ec4

Co-authored-by: Martin Costello <[email protected]>

Update comment

b5e6c64

Mark RedrivePolicy as obsolete

5ae305a

Add tests

aadf5dc

TODO: Make test names/namespaces not the worst

Add test for options extensions

9744507

Fix test name

b74891d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Native AOT #1348

Support Native AOT #1348

hwoodiwiss commented Feb 17, 2024 •

edited

Loading

hwoodiwiss commented Feb 17, 2024

codecov bot commented Feb 17, 2024 •

edited

Loading

martincostello commented Feb 17, 2024

martincostello Feb 17, 2024

hwoodiwiss Feb 17, 2024

martincostello Feb 17, 2024

hwoodiwiss Feb 17, 2024

hwoodiwiss Feb 17, 2024

hwoodiwiss Feb 22, 2024

martincostello Feb 17, 2024

martincostello Feb 17, 2024

hwoodiwiss Feb 17, 2024

martincostello Feb 17, 2024

hwoodiwiss Feb 17, 2024

hwoodiwiss commented Feb 17, 2024

slang25 commented Feb 18, 2024

hwoodiwiss commented Feb 18, 2024

slang25 commented Feb 18, 2024

hwoodiwiss commented Feb 20, 2024

hwoodiwiss Feb 28, 2024

		var dataType = obj.Value.GetProperty("Type").GetString();
		var dataValue = obj.Value.GetProperty("Value").GetString();


		namespace JustSaying.Messaging.MessageSerialization;

		public class TypedSystemTextJsonSerializationFactory(JsonSerializerOptions options) : IMessageSerializationFactory

Support Native AOT #1348

Are you sure you want to change the base?

Support Native AOT #1348

Conversation

hwoodiwiss commented Feb 17, 2024 • edited Loading

hwoodiwiss commented Feb 17, 2024

codecov bot commented Feb 17, 2024 • edited Loading

Codecov Report

martincostello commented Feb 17, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwoodiwiss commented Feb 17, 2024

slang25 commented Feb 18, 2024

hwoodiwiss commented Feb 18, 2024

slang25 commented Feb 18, 2024

hwoodiwiss commented Feb 20, 2024

Choose a reason for hiding this comment

hwoodiwiss commented Feb 17, 2024 •

edited

Loading

codecov bot commented Feb 17, 2024 •

edited

Loading