Skip to content
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

DynamoDBEntryConversion AOT compatibility #3300

Draft
wants to merge 4 commits into
base: main-staging
Choose a base branch
from

Conversation

Dreamescaper
Copy link
Contributor

@Dreamescaper Dreamescaper commented Apr 24, 2024

Related to #2542.

I was looking into improving AOT compatibility for DynamoDB, and decided to submit a POC for further discussion.
With this PR all DynamoDbEntryConverion-related types are AOT compatible.

Description

  • Converters do not have GetTargetTypes method anymore, therefore they don't need to construct generic types.
  • Converters are created lazily using Factories, which create a converter based on target type.
  • Converters themselves are mostly reflection free, e.g. CollectionConverters create and populate collections via ToList/ToArray/etc methods instead of reflection.
  • In order to do that, Collection converters are generic now.

Motivation and Context

Testing

As I mentioned, I haven't added any unit tests until it is discussed, since I wasn't sure that this approach would be approved or even considered.

I've added a small Aot project, which I have tested using Local DynamoDb. I failed due triming issues before, but works fine now.
It doesn't support nested object (which is expected without a source generator), but it could be resolved by adding TrimmerRootAssembly to csproj file.

In order to test performance impact, I have created the following benchmark:

Benchmark
  [SimpleJob]
  [SimpleJob(RunStrategy.ColdStart, launchCount: 40, iterationCount: 1, invocationCount: 1)]
  [MemoryDiagnoser]
  [MinColumn, MaxColumn, MeanColumn]
  public class ConversionBenchmark
  {
      private static readonly int[] Array = [1, 2, 3, 4, 5, 6];
      private static readonly List<int> List = [1, 2, 3, 4, 5, 6];
      private static readonly Primitive IntPrimitive = new("42", true);
      private static readonly PrimitiveList PrimitiveList = new(DynamoDBEntryType.Numeric) { Entries = [new("1", true), new("2", true), new("3", true)] };
      private static readonly DynamoDBList DynamoDBList = new([new Primitive("1", true), new Primitive("2", true), new Primitive("3", true)]);

      [Benchmark]
      public void ConvertToEntry_Int()
      {
          _ = DynamoDBEntryConversion.V2.ConvertToEntry(128);
      }

      [Benchmark]
      public void ConvertToEntry_Array()
      {
          _ = DynamoDBEntryConversion.V2.ConvertToEntry(Array);
      }

      [Benchmark]
      public void ConvertToEntry_List()
      {
          _ = DynamoDBEntryConversion.V2.ConvertToEntry(List);
      }

      [Benchmark]
      public void ConvertFromEntry_IntPrimitive()
      {
          _ = DynamoDBEntryConversion.V2.ConvertFromEntry<int>(IntPrimitive);
      }

      [Benchmark]
      public void ConvertFromEntry_PrimitiveListToArray()
      {
          _ = DynamoDBEntryConversion.V2.ConvertFromEntry<int[]>(PrimitiveList);
      }

      [Benchmark]
      public void ConvertFromEntry_DynamoDbListToList()
      {
          _ = DynamoDBEntryConversion.V2.ConvertFromEntry<List<int>>(DynamoDBList);
      }
  }
Benchmark results

Simple job (main):

Method Mean Error StdDev Min Max Gen0 Allocated
ConvertToEntry_Int 28.88 ns 0.592 ns 0.770 ns 27.83 ns 30.23 ns 0.0134 56 B
ConvertToEntry_Array 841.20 ns 16.028 ns 15.742 ns 814.28 ns 864.23 ns 0.1812 761 B
ConvertToEntry_List 586.41 ns 11.369 ns 17.700 ns 560.29 ns 631.60 ns 0.1850 776 B
ConvertFromEntry_IntPrimitive 25.09 ns 0.493 ns 0.506 ns 23.89 ns 25.88 ns 0.0057 24 B
ConvertFromEntry_PrimitiveListToArray 824.03 ns 16.343 ns 18.166 ns 783.82 ns 853.90 ns 0.2537 1064 B
ConvertFromEntry_DynamoDbListToList 644.30 ns 12.341 ns 12.120 ns 618.45 ns 660.40 ns 0.1812 760 B

Simple job (this branch):

Method Mean Error StdDev Min Max Gen0 Allocated
ConvertToEntry_Int 26.24 ns 0.550 ns 0.654 ns 25.41 ns 27.77 ns 0.0134 56 B
ConvertToEntry_Array 289.38 ns 3.258 ns 2.888 ns 284.92 ns 295.16 ns 0.1526 640 B
ConvertToEntry_List 311.13 ns 6.162 ns 7.792 ns 301.78 ns 327.60 ns 0.1545 648 B
ConvertFromEntry_IntPrimitive 22.75 ns 0.480 ns 0.673 ns 21.79 ns 24.12 ns 0.0057 24 B
ConvertFromEntry_PrimitiveListToArray 165.50 ns 3.303 ns 6.205 ns 156.90 ns 184.56 ns 0.0610 256 B
ConvertFromEntry_DynamoDbListToList 158.07 ns 2.776 ns 2.597 ns 154.61 ns 162.72 ns 0.0591 248 B

Cold start (main):

Method Mean Error StdDev Min Max
ConvertToEntry_Int 9.856 ms 0.5121 ms 0.9103 ms 8.934 ms 13.84 ms
ConvertToEntry_Array 12.668 ms 0.4000 ms 0.7111 ms 11.368 ms 15.49 ms
ConvertToEntry_List 12.843 ms 0.5912 ms 1.0509 ms 11.667 ms 16.64 ms
ConvertFromEntry_IntPrimitive 11.168 ms 0.8159 ms 1.4503 ms 9.972 ms 18.01 ms
ConvertFromEntry_PrimitiveListToArray 12.760 ms 0.9900 ms 1.7596 ms 11.370 ms 22.73 ms
ConvertFromEntry_DynamoDbListToList 12.646 ms 0.6049 ms 1.0752 ms 11.445 ms 17.80 ms

Cold start (this branch):

Method Mean Error StdDev Min Max
ConvertToEntry_Int 3.625 ms 0.2369 ms 0.4212 ms 3.182 ms 5.348 ms
ConvertToEntry_Array 6.181 ms 0.2995 ms 0.5323 ms 5.355 ms 7.493 ms
ConvertToEntry_List 6.324 ms 0.4865 ms 0.8648 ms 5.448 ms 9.297 ms
ConvertFromEntry_IntPrimitive 5.166 ms 0.3534 ms 0.6283 ms 4.324 ms 6.921 ms
ConvertFromEntry_PrimitiveListToArray 6.329 ms 0.4266 ms 0.7582 ms 5.549 ms 9.364 ms
ConvertFromEntry_DynamoDbListToList 6.303 ms 0.2682 ms 0.4767 ms 5.527 ms 7.334 ms

As you can see, there is a significant performance improvement both in speed and memory use, mostly for collection conversions.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@@ -41,7 +39,9 @@ public partial class DynamoDBContext : IDynamoDBContext
/// <param name="value">Object to save.</param>
/// <param name="cancellationToken">Token which can be used to cancel the task.</param>
/// <returns>A Task that can be used to poll or wait for results, or both.</returns>
public Task SaveAsync<T>(T value, CancellationToken cancellationToken = default(CancellationToken))
#pragma warning disable IL2095 // 'DynamicallyAccessedMemberTypes' on the generic parameter of method or type don't match overridden generic parameter method or type. All overridden members must have the same 'DynamicallyAccessedMembersAttribute' usage.
public Task SaveAsync<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>(T value, CancellationToken cancellationToken = default(CancellationToken))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to test AOT changes. I would create a separate PR with such annotations if approved.

@@ -0,0 +1,53 @@
#if !NET6_0_OR_GREATER
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just added it here to avoid constant #if NET8_0_OR_GREATER diectives

@Dreamescaper
Copy link
Contributor Author

I was looking into ways to improve AOT compatibility (and performance), and decided to rework a bit DynamoDBEntryConversion.

Since it's a substantial piece of work, I decided to create a Draft PR (without tests, maybe something else) for further discussion.

@danielmarbach
Copy link
Contributor

FYI I have written for NServiceBus DynamoDB persistence a system.text.json based wrapper that also allows to plug in source generated json context.

https://github.com/Particular/NServiceBus.Persistence.DynamoDB/blob/main/src/NServiceBus.Persistence.DynamoDB/Serialization/Mapper.cs

The approach is fairly library like and we would be happy to contribute it back to the AWS SDK if it can be plugged into the dynamodb context.

Simple Class Serialization and Deserialization

image

image

Nested Class Serialization and Deserialization

image

image

Benchmark

And these numbers are already quite old and would probably look even better today on NET8

@dscpinheiro dscpinheiro requested a review from normj April 25, 2024 17:35
@dscpinheiro dscpinheiro changed the base branch from main to main-staging April 25, 2024 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants