Add trimming support for RuntimeSupport, SystemTextJson serializer and Lambda event packages #1596

normj · 2023-10-25T00:27:50Z

Description of changes:
Updated the Lambda RuntimeSupport, SystemTextJson serializer and Lambda events to support trimming.

Replaced all NETCOREAPP_3_1 preprocessor references to NETCOREAPP3_1_OR_GREATER
Added net8.0 target framework to projects and mark the projects as trimmable.
Marked all serializer in the SystemTextJson as requiring unreferenced code except for the source generator seriralizer
There were a couple places where an event was directly using the SystemTextJson serializer. For .NET 8 I switched it to use the source generator approach.
In Amazon.Lambda.RuntimeSupport marked the code paths used by the class library approach of loading the customer function by reflection and build Expressions as RequiresUnreferencedCode. These code paths are not used when deployed as an executable which is the AOT use case.
Updated the Events.NET6 project to multi target .NET 8. At some point we should rename this project but I don't want to mess with the CI system right now.
Added a new constructor for SourceGeneratorLambdaJsonSerializer that takes in the JsonSerializerContext so the serializer doesn't have to use reflection to get an instance of the generic parameter.

I have verified with these changes I can package a Lambda function using just these libraries and received no trim warnings.

Work not included in this PR

Update the ASP.NET Core bridge to support trimming
Update Lambda Annotations to support .NET 8 and trimming

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…d Lambda event packages

normj · 2023-10-25T00:30:38Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Client/InternalClientAdapted.cs

        {
-            if (value is System.Enum)


This code was not trimming compatible and it turn out our only use of the method was to the value being a string. So I choose to just simplify this method. Along time again this class was generated from a swagger file which I assume the generator just differentiated for all possible use cases.

normj · 2023-10-25T00:32:54Z

Libraries/src/Amazon.Lambda.RuntimeSupport/ExceptionHandling/StackFrameInfo.cs

-                    Label = method.Name;
-                }
-                else
+                var method = stackFrame.GetMethod();


This GetMethod call is marked as RequiresUnreferencedCode. The code is required so I just made the code more defensive in case something gets trimmed. Since we only really need the that is unlikely to be metadata trimmed.

ashovlin

And for the failed CI, can you add a .NET 8 block here?

aws-lambda-dotnet/.github/workflows/source-generator-ci.yml

Lines 17 to 24 in 68396f5

    
           - name: Setup .NET 3.1 
        
             uses: actions/setup-dotnet@v1 
        
             with: 
        
               dotnet-version: 3.1.x 
        
           - name: Setup .NET 6.0 
        
             uses: actions/setup-dotnet@v1 
        
             with: 
        
               dotnet-version: 6.0.x

Libraries/src/Amazon.Lambda.SNSEvents/Amazon.Lambda.SNSEvents.csproj

Libraries/src/Amazon.Lambda.ConnectEvents/Amazon.Lambda.ConnectEvents.csproj

Libraries/test/TestFunctionFSharp/TestFunctionCSharp/TestFunctionCSharp.csproj

philasmar · 2023-10-25T14:36:46Z

Libraries/src/Amazon.Lambda.KinesisEvents/Converters/DictionaryLongToStringJsonConverter.cs

@@ -47,8 +47,12 @@ public class DictionaryLongToStringJsonConverter : JsonConverter<Dictionary<stri

        public override void Write(Utf8JsonWriter writer, Dictionary<string, string> value, JsonSerializerOptions options)
        {
+#if NET8_0_OR_GREATER
+            JsonSerializer.Serialize(writer, value, typeof(Dictionary<string, string>), new DictionaryStringStringJsonSerializerContext(options));


It would be helpful to add a comment here explaining why we're doing it this way for .NET8 and above

philasmar · 2023-10-25T14:36:56Z

Libraries/src/Amazon.Lambda.DynamoDBEvents/Converters/DictionaryLongToStringJsonConverter.cs

@@ -47,8 +47,12 @@ public class DictionaryLongToStringJsonConverter : JsonConverter<Dictionary<stri

        public override void Write(Utf8JsonWriter writer, Dictionary<string, string> value, JsonSerializerOptions options)
        {
+#if NET8_0_OR_GREATER
+            JsonSerializer.Serialize(writer, value, typeof(Dictionary<string, string>), new DictionaryStringStringJsonSerializerContext(options));


It would be helpful to add a comment here explaining why we're doing it this way for .NET8 and above

philasmar · 2023-10-25T14:38:34Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Amazon.Lambda.RuntimeSupport.csproj

+  <PropertyGroup Condition="'$(TargetFramework)' == 'net8.0'">
+    <WarningsAsErrors>IL2026,IL2067,IL2075</WarningsAsErrors>
+    <IsTrimmable>true</IsTrimmable>
+	<EnableTrimAnalyzer>true</EnableTrimAnalyzer>


Why does RuntimeSupport need EnableTrimAnalyzer but the rest don't?

Added EnableTrimAnalyzer to the other projects. I added this property so we can see future breaks to trimming at compile time for these libraries.

philasmar · 2023-10-25T14:40:23Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Bootstrap/LambdaBootstrap.cs

@@ -112,6 +112,11 @@ private LambdaBootstrap(HttpClient httpClient, LambdaBootstrapHandler handler, L
        /// </summary>
        /// <param name="cancellationToken"></param>
        /// <returns>A Task that represents the operation.</returns>
+#if NET8_0_OR_GREATER
+        [System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026",
+            Justification = "Unreferenced code paths are excluding when RuntimeFeature.IsDynamicCodeSupported is false.")]


"excluding" should be "excluded"

philasmar · 2023-10-25T14:43:19Z

Libraries/src/Amazon.Lambda.Serialization.SystemTextJson/Converters/ConstantClassConverter.cs

@@ -41,10 +41,14 @@ public override bool CanConvert(Type typeToConvert)
        /// <param name="typeToConvert"></param>
        /// <param name="options"></param>
        /// <returns></returns>
+#if NET8_0_OR_GREATER
+        [System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessage("ReflectionAnalysis", "IL2067",
+            Justification = "Constant classes are only used in the DynamoDB event referencing the SDK. S3 originally referenced the SDK but has been rewritten to no longer reference the SDK or ConstantClass. Suppressing this trim warning because we have marked the DynamoDB event as ")]


sentence incomplete? what did we mark the DynamoDB event as?

philasmar

Approved, assuming you fix the failing PR checks

normj · 2023-10-26T16:25:03Z

And for the failed CI, can you add a .NET 8 block here?

aws-lambda-dotnet/.github/workflows/source-generator-ci.yml

Lines 17 to 24 in 68396f5

- name: Setup .NET 3.1

uses: actions/setup-dotnet@v1

with:

dotnet-version: 3.1.x

- name: Setup .NET 6.0

uses: actions/setup-dotnet@v1

with:

dotnet-version: 6.0.x

Fixed the CI build. Since .NET 8 isn't GA I had to set the workflow to the current RC. Will need to update that to 8.0.x once .NET 8 has been released.

Beau-Gosse-dev · 2023-10-26T17:03:01Z

Libraries/src/Amazon.Lambda.RuntimeSupport/Bootstrap/UserCodeInit.cs

@@ -140,6 +140,9 @@ public static void LoadStringCultureInfo()
            }
        }

+#if NET8_0_OR_GREATER
+    [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCode("PreJitAssembly is not used for Native AOT")]


Technically someone could want to trim their assembly and not use native AOT, although obviously native AOT is the big use case. In general some of the explanations assume trimming = native AOT, and it might help to be a little more clear why that is going forward.

Add trimming support for RuntimeSupport, SystemTextJson serializer an…

e088468

…d Lambda event packages

normj force-pushed the normj/add-trimming-support branch from deba8a9 to e088468 Compare October 25, 2023 00:28

normj commented Oct 25, 2023

View reviewed changes

ashovlin requested changes Oct 25, 2023

View reviewed changes

philasmar requested changes Oct 25, 2023

View reviewed changes

Address PR comments

0f20ff5

philasmar approved these changes Oct 26, 2023

View reviewed changes

normj added 3 commits October 26, 2023 09:09

Add .NET 8 to GitHub workflow

c4ad8c0

Add .NET 8 to GitHub workflow

971e36c

Add .NET 8 to GitHub workflow

75b3c5b

ashovlin approved these changes Oct 26, 2023

View reviewed changes

normj changed the base branch from master to dev October 26, 2023 16:47

normj merged commit 0022859 into dev Oct 26, 2023
3 checks passed

normj deleted the normj/add-trimming-support branch October 26, 2023 16:47

Beau-Gosse-dev reviewed Oct 26, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add trimming support for RuntimeSupport, SystemTextJson serializer and Lambda event packages #1596

Add trimming support for RuntimeSupport, SystemTextJson serializer and Lambda event packages #1596

normj commented Oct 25, 2023 •

edited

Loading

normj Oct 25, 2023

normj Oct 25, 2023

ashovlin left a comment

philasmar Oct 25, 2023

normj Oct 25, 2023

philasmar Oct 25, 2023

normj Oct 25, 2023

philasmar Oct 25, 2023

normj Oct 25, 2023

philasmar Oct 25, 2023

normj Oct 25, 2023

philasmar Oct 25, 2023

normj Oct 25, 2023

philasmar left a comment

normj commented Oct 26, 2023

Beau-Gosse-dev Oct 26, 2023 •

edited

Loading

	- name: Setup .NET 3.1
	uses: actions/setup-dotnet@v1
	with:
	dotnet-version: 3.1.x
	- name: Setup .NET 6.0
	uses: actions/setup-dotnet@v1
	with:
	dotnet-version: 6.0.x

Add trimming support for RuntimeSupport, SystemTextJson serializer and Lambda event packages #1596

Add trimming support for RuntimeSupport, SystemTextJson serializer and Lambda event packages #1596

Conversation

normj commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashovlin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philasmar left a comment

Choose a reason for hiding this comment

normj commented Oct 26, 2023

Beau-Gosse-dev Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

normj commented Oct 25, 2023 •

edited

Loading

Beau-Gosse-dev Oct 26, 2023 •

edited

Loading