-
Notifications
You must be signed in to change notification settings - Fork 859
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
base: v4-development
Are you sure you want to change the base?
Conversation
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. |
FYI I have written for NServiceBus DynamoDB persistence a system.text.json based wrapper that also allows to plug in source generated json context. 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 DeserializationNested Class Serialization and DeserializationAnd these numbers are already quite old and would probably look even better today on NET8 |
@normj Still, if you think that it's too big of a change or goes in unwanted direction - feel free to close this PR, I would be fine with that as well :) |
result = t; | ||
return output; | ||
} | ||
|
||
protected virtual bool TryFrom(DynamoDBBool b, Type targetType, out T result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a separate targetType
parameter for a generic Converter kindof defeats the purpose, and not really AOT-friendly, therefore I have removed it.
The only Converter to actually use it is EnumConverter: Converter<Enum>
. But it doesn't make any value to have an Enum
generic argument, so I've updated it to inherit from non-generic Converter
.
(Not a public API, so no breaking changes here)
@@ -777,29 +751,20 @@ protected virtual bool TryFrom(Document d, Type targetType, out T result) | |||
internal class ConverterCache | |||
{ | |||
private static Type EnumType = typeof(Enum); | |||
private Dictionary<Type, Converter> Cache = new Dictionary<Type, Converter>(); | |||
private readonly ConcurrentDictionary<Type, Converter> Cache = new ConcurrentDictionary<Type, Converter>(); | |||
private readonly List<ConverterFactory> Factories = new List<ConverterFactory>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously all the converters were created upfront, but now they are created lazily, when requested. Therefore, ConcurrentDictionary is used for cache, as it is possible that converter is requested from multiple threads at the same time.
ItemsToIList(targetType, items, out result); //targetType is IList or has Add method. | ||
} | ||
|
||
public static bool ItemsToCollection<T, TCollection>(IEnumerable<T> items, out TCollection result) where TCollection : ICollection<T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing methods ItemsToCollection
(above) theoretically supports any collection type with Add
method.
This new one - only array, List, and HashSet.
However, CollectionConverter
doesn't support any other collection types (and never did), so no breaking changes here.
I've added explaining comments, removed unrelated changes, and added tests. |
Updated target branch to v4-development |
@@ -368,6 +366,14 @@ internal IEnumerable<object> ConvertFromEntries(Type elementType, IEnumerable<Dy | |||
yield return ConvertFromEntry(elementType, entry); | |||
} | |||
|
|||
internal IEnumerable<T> ConvertFromEntries<T>(IEnumerable<DynamoDBEntry> entries) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an existing generic method ConvertFromEntry<T>, but there was no generic method for multiple entries
Related to #2542.
I was looking into improving AOT compatibility for DynamoDB.
With this PR all DynamoDbEntryConverion-related types are AOT compatible.
(That doesn't make DynamoDbContext AOT compatible as a whole, but it makes it much better, and is an important building block on the way there)
Description
GetTargetTypes
method anymore, therefore they don't need to construct generic types.Motivation and Context
Make DynamoDb closer to being AOT-compatible. Having Converters fully AOT-compatible allows to use them in source-generated code in future.
Even this PR on itself heavily improves AOT compatibility. You still have to set your assembly as
TrimmerRootAssembly
(because of reflection), but at least DynamoDbContext internals don't fail anymore.And the performance is better (both steady and cold-start).
Testing
I've added a lot of tests for
DynamoDBEntryConversion
for both V1 and V2.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
Benchmark results
Simple job (main):
Simple job (this branch):
Cold start (main):
Cold start (this branch):
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
Checklist
License