Remove assembly scanning from DynamoDBEntryConversion

[Dreamescaper]

Description

Replace assembly scanning in DynamoDBEntryConversion which finds converter types with explicit types created.

Motivation and Context

Assembly scanning has a cold start performance impact, which is especially important for serverless applications. In this case, it scans own assembly for types, which doesn't have much sense, as the results of that scan is static, it will never change. Therefore, it doesn't make sense to find those types in runtime.

Besides, it is more AOT-friendly, and it probably allows to remove most of RequiresUnreferencedCode attributes (not sure).

Testing

I have created a small benchmark to test cold start performance.

Benchmark

  [SimpleJob(RunStrategy.ColdStart, launchCount: 20, iterationCount: 1, invocationCount: 1)]
  [MinColumn, MaxColumn, MeanColumn, MedianColumn]
  public class ConversionBenchmark
  {
      [Benchmark]
      public void CreateConversions()
      {
          _ = DynamoDBEntryConversion.V1;
          _ = DynamoDBEntryConversion.V2;
      }
  }

Before:

Method	Mean	Error	StdDev	Min	Max	Median
CreateConversions	44.38 ms	4.562 ms	5.253 ms	33.70 ms	53.47 ms	44.96 ms

After:

Method	Mean	Error	StdDev	Min	Max	Median
CreateConversions	14.87 ms	2.421 ms	2.788 ms	10.34 ms	22.29 ms	14.33 ms

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

[normj]

I like this idea of getting rid of the Assembly scan. This was written at a time predating Lambda when cold starts weren't so critical. We need a way to make sure if somebody adds a new converter it doesn't get forgotten to be added here. I'm thinking a simple approach is add a ConverterCount property to DynamoDBEntryConversion. Then you could add a unit test to checks the known number and then run the assembly scan in their now to compute the actual number for the unit test.

[Dreamescaper]

@normj
I have added a unit test verifying that all converters are registered (had to add InternalsVisibleTo attribute).

(Not sure I like it though, focusing on the implementation details, but probably better than nothing).

[normj]

The AssemblyInfo.cs file is generated so this change would get whipped out the next time we do a build. You would need to add that change to the tt file in the generator here and then parameterize the service name. https://github.com/aws/aws-sdk-net/blob/02ec2a6188696a765eebbf50c5fd0bf2f1321b11/generator/ServiceClientGeneratorLib/Generators/SourceFiles/AssemblyInfo.tt

To run the generator you open the AWSSDKGenerator.sln solution and then run the ServiceClientGenerator project. That will regenerate the the AssemblyInfo.cs. You don't want to include the generated code in the PR because then the PR will be huge.

[Dreamescaper]

@normj
I have updated the generator to sign UnitTests projects, and include them in InternalsVisibleTo attributes.
Let me know if you'd prefer any other approach.

Thanks for the help!

[Dreamescaper]

@normj
The other option would be to remove this IVT attribute, and add unit tests for DynamoDBEntryConversion itself.
E.g. test that DynamoDBEntryConversion is able to convert to and from byte instead of testing that ByteConverter is registered.
(I wasn't able to find existing tests for that, correct me if I'm wrong).

On its own, that wouldn't prevent from adding a converter and forgetting to add it to the schema, but hopefully it will encourage to add corresponding tests.

WDYT?

[normj]

Sorry @Dreamescaper for going silent I was out of the office for a bit.

I feel like getting the InternalVisibleTo working is making this PR too complicated. With your PR right now the uber unit test projects that include all of the unit tests fail. I'm thinking we should simplify this and right the unit tests using reflection on the internals. Below is my version of the unit tests using reflection and I don't need the generator changes or the InternalVisibleTo attributes.

using Amazon.DynamoDBv2;
using Microsoft.VisualStudio.TestTools.UnitTesting;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;

namespace AWSSDK_DotNet35.UnitTests
{
    [TestClass]
    public class DynamoDBEntryConversionTests
    {
        [TestMethod]
        public void ValidateAllConvertersAreRegisteredForConversionV1()
        {
            AssertAllConvertersAreRegistered(DynamoDBEntryConversion.V1, "ConverterV1");
        }

        [TestMethod]
        public void ValidateAllConvertersAreRegisteredForConversionV2()
        {
            AssertAllConvertersAreRegistered(DynamoDBEntryConversion.V2, "ConverterV2");
        }

        private void AssertAllConvertersAreRegistered(DynamoDBEntryConversion conversion, string suffix)
        {
            var converters = GetConverters(suffix);

            var tryGetConverterInfo = conversion.GetType().GetMethod("TryGetConverter", BindingFlags.NonPublic | BindingFlags.Instance);

            foreach (var converter in converters)
            {
                var getTargetTypeInfo = converter.GetType().GetMethod("GetTargetTypes");
                IEnumerable<Type> targetTypes = (IEnumerable < Type >)getTargetTypeInfo.Invoke(converter, new object[0]);
                foreach (var type in targetTypes)
                {
                    var tryGetConverterParams = new object[] { type, null };
                    tryGetConverterInfo.Invoke(conversion, tryGetConverterParams);
                    var registeredConverter = tryGetConverterParams[1];

                    Assert.IsNotNull(registeredConverter);
                    Assert.AreEqual(converter.GetType(), registeredConverter.GetType());
                }
            }
        }

        private IEnumerable<object> GetConverters(string suffix)
        {
            const string converterTypeName = "Amazon.DynamoDBv2.Converter";
            var assembly = typeof(DynamoDBEntryConversion).Assembly;

            var allTypes = assembly.GetTypes();
            var typedConverterType = allTypes.FirstOrDefault(x => string.Equals(converterTypeName, x.FullName));

            foreach (var type in allTypes)
            {
                if (type.IsAbstract)
                    continue;

                if (!type.Name.EndsWith(suffix, StringComparison.Ordinal))
                    continue;

                if (!typedConverterType.IsAssignableFrom(type))
                    continue;

                var constructor = type.GetConstructor(BindingFlags.NonPublic | BindingFlags.Public | BindingFlags.Instance, null, new Type[0], null);
                yield return constructor.Invoke(new object[0]);
            }
        }
    }
}

[Dreamescaper]

@normj I have updated the PR with your suggestion

[normj]

Looks good. I'm running the PR through our internal validation system.

[normj]

Internal dry run was successful. Let me get another reviewer to approve the change.

[normj]

The PR went out today as part of version 3.7.302.22 of AWSSDK.DynamoDBv2. Thanks for the PR and helping improve the SDK's performance.