Add AggressiveInlining support

[mjebrahimi]

Add AggressiveInlining support

Description

• Adds support for generating [MethodImpl(MethodImplOptions.AggressiveInlining)] for mapping methods.
• Adds expected code to verify snapshots to pass the tests.

Fixes #1381 (issue)

I did some benchmarks and the results were amazing.

• 27x faster for simple struct types
• 1.3x faster for complex struct types
• Very slightly faster for class types

Checklist

• The existing code style is followed
• The commit message follows our guidelines
• Performed a self-review of my code
• Hard-to-understand areas of my code are commented
• The documentation is updated (as applicable)
• Unit tests are added/updated
• Integration tests are added/updated (as applicable, especially if feature/bug depends on roslyn or framework version in use)

[latonz]

I added a few points to be discussed here: #1381 (comment)

[mjebrahimi]

I updated the code according to our discussion in #1381
Current status: Work in progress (need some help to complete the code)

• I created that enum and named it AggressiveInliningMode as I think it's better naming.
• I didn't set number value for enums because it's not going to be [Flag]able
• I added AggressiveInliningMode to MapperAttribute with defaults to ValueTypes (what you do think about the default value?)
• I added support for this new property wherever I found necessary (such as MapperConfiguration, MapperConfigurationMerger, TestSourceBuilder, TestSourceBuilderOptions) (did I miss anything?)
• I added GenerateMethodImplAttributeListIfNeeded method to MethodMapping class. (is everything ok with it and it's location?)
• I needed to read the value of MapperAttribute.AggressiveInliningMode property but I couldn't find how to (need some help for this)
• As we must skip applying AgressiveInlining if the method has already [MethodImpl] attribute, I wrote a method HasAttribute<TAttribute> in
  Riok.Mapperly/Helpers/MethodDeclarationSyntaxExtensions.cs (however I have some considerations)

Developers can use [MethodImpl] attribute in several ways (including and not limited to bellows)

[MethodImpl]
[MethodImplAttribute]
[System.Runtime.CompilerServices.MethodImpl]
[System.Runtime.CompilerServices.MethodImplAttribute]
[global::System.Runtime.CompilerServices.MethodImpl]
[global::System.Runtime.CompilerServices.MethodImplAttribute]

We could check the string of attribute syntax with any of those possibilities.
For example:

private static readonly string[] _methodImplPossiblities =
[
    "MethodImpl",
    "MethodImplAttribute",
    "System.Runtime.CompilerServices.MethodImpl",
    "System.Runtime.CompilerServices.MethodImplAttribute",
    "global::System.Runtime.CompilerServices.MethodImpl",
    "global::System.Runtime.CompilerServices.MethodImplAttribute",
];
private static bool HasMethodImpleAttribute(MethodDeclarationSyntax methodDeclarationSyntax)
{
    var attributes = methodDeclarationSyntax.AttributeLists.SelectMany(p => p.Attributes);
    return attributes.Any(attributeSyntax => Array.Exists(_methodImplPossiblities, p => attributeSyntax.ToString().Contains(p, StringComparison.Ordinal)));
}

But it seems it's not a clean and robust approach
Even worse when developers use type aliasing! in this way we can't recognize it at all, for example:

using AliasName = System.Runtime.CompilerServices.MethodImplAttribute;

[Mapper]
public static partial class MyMapper
{
    [AliasName(MethodImplOptions.AggressiveInlining)]
    public static partial DateTime MapTo(DateTime dt);
}

So after some research, I found this way that can handle all of those. (this is the current implementation)

public static bool HasAttribute<TAttribute>(this MethodDeclarationSyntax methodDeclarationSyntax) where TAttribute : Attribute
{
    var csharpCompilation = CSharpCompilation.Create(
        assemblyName: null,
        syntaxTrees: [methodDeclarationSyntax!.SyntaxTree],
        references: [MetadataReference.CreateFromFile(typeof(TAttribute).Assembly.Location)]
    );

    var semanticModel = csharpCompilation.GetSemanticModel(methodDeclarationSyntax.SyntaxTree);

    var attributes = methodDeclarationSyntax.AttributeLists.SelectMany(p => p.Attributes);

    return attributes.Any(attributeSyntax =>
    {
        var symbols = semanticModel.GetSymbolInfo(attributeSyntax).CandidateSymbols.OfType<IMethodSymbol>();
        return symbols.Any(p => string.Equals(p.ContainingType.ToString(), typeof(TAttribute).FullName, StringComparison.Ordinal));
    });
}

This method works fine, however, it always Compiles the code to understand the meaning, and I'm not sure if it's ok or not due to performance problems (what do you think?)

Next Step:
After completing the implementation I go for writing some unit/integration tests to ensure its functionality in different conditions. (Anything else?)

[latonz]

Thank you for the updates 😊 💪

• I'm not sure if AggressiveInliningMode is a better name, this limits future changes without needing breaking changes 🤔 Types is much more specific IMO.
• I would prefer a flags enum as this allows more flexibility for the future (for example we may want an extra enum entry etc.). Also this aligns with similar public Mapperly APIs (e.g. RequiredMappingStrategy).
• I'm not sure whether we should check only the source type of the mapping against the configuration, what are your thoughts? The more I think about it, the more I think a simple boolean enable/disable configuration might be easier 🤔🤔
• Default value: why do you think we should default to value types only instead of all? Because of the assembly size / compilation time vs. performance gain?
• Instead of the syntax based evaluation whether the MethodImpl attribute is present I think it would be much easier to do it on the semantic model. Idea:
  • Add a boolean member on the MethodMapping on whether to the MethodImpl attribute needs to be added
  • Whenever instantiating a MethodMapping with an IMethodSymbol set this member to true if the attribute is absent on the user declared method symbol and the configuration is enabled for the given types (e.g. in various methods of UserMethodMappingExtractor, e.g. BuildUserDefinedMapping)
  • Evaluation whether the attribute is present or not should be as simple as ctx.SymbolAccessor.HasAttribute<MethodImplAttribute>(methodSymbol)
  • Access to the configuration in UserMethodMappingExtractor is available through ctx.Configuration.Mapper.AggressiveInliningMode (you cannot read the configuration in the emit / build phase, it is only available when building the mappings)

What else should be done?

• Update the documentation
  • Update the benchmarks
  • Add a section about this feature
• Add a unit test (ensures different scenarios work)
• Add an integration test (ensures the basic feature works on different .NET versions)
• Ensure the CI pipeline passes (since this is your first contribution I manually need to approve each run, but you should be able to run most linters & tests locally, see also the contributors documentation)

[mjebrahimi]

Thank you so much for your help.

I just wanted to sync my branch with the upstream and used the re-sync (with discard commits) feature of GitHub, but didn't know it closes the pull request! 😅

I'm working on it and I'm gonna re-open this PR with new updates by today ✌️

[mjebrahimi]

Thanks @latonz your idea was very helpful to me in understanding the codebase better and finding the right path to implement this feature.

I came with cool updates. 😊

About the previous conversation:

• Default Value: My only concern about defaulting to ValueTypes is that:

  • Given that the performance improvement of AggressiveInlining is mostly for value types, rather than reference types.
  • Developers mostly use class types for their DTOs/Entities and map them.
  • Given the previous, using AggressiveInlining results in increasing the assembly size and compilation time without almost any gain.
  • For more caution, there may be other disadvantages to using AggressiveInlining too much, that we are not yet aware of.

• Configuration: The more I think about having an enum, the more it gets complex unnecessarily.

  • We can have an object with a hierarchy of several class and struct nested properties. Should we decide based on the root mapping method or per each mapping method for each property mapping? Why?
  • We can have a method that maps a struct/value type to a class/ref type. Should we decide based on input type or output type? Why?
  • Is there any need to add more types to that enum in the future? For example what? Is it necessary?

• In the end: I think we don't need that much complexity (over-engineering) and I do agree with you with a bool flag to enable/disable it with defaults to true (the current implementation).

About the current implementation:

• According to your idea I added a bool argument named enableAggressiveInlinig for MappingMethod types.
• For the below types in the UserMethodMappingExtractor class, the value for enableAggressiveInlinig is read from Configuration and !HasAttribute<MethodImple>().
  • UserDefinedNewInstanceGenericTypeMapping
  • UserDefinedExistingTargetMethodMapping
  • UserDefinedNewInstanceMethodMapping
  • UserDefinedNewInstanceRuntimeTargetTypeParameterMapping
• For the below types, the value for enableAggressiveInlinig is read only from Configuration.
• Based on my understanding these mapping methods are generated as non-partial and are not the root/user mapping method, so we don't need to check if the root/user method has [MethodImple] attribute, we can generate AggressiveInlining if it's enabled in the Configuration even when the root/user mapping method has [MethodImple] attribute. (please correct me if I'm wrong)
  • ArrayForEachMapping
  • ArrayForMapping
  • EnumNameMapping
  • ForEachAddEnumerableMapping
  • ForEachSetDictionaryMapping
  • MethodMapping
  • NewInstanceMethodMapping
  • NewInstanceObjectFactoryMemberMapping
  • NewInstanceObjectMemberMethodMapping
  • NewValueTupleExpressionMapping
  • NullDelegateMethodMapping
  • ObjectMemberMethodMapping
  • UserDefinedExistingTargetMethodMapping
  • UserDefinedNewInstanceGenericTypeMapping
  • UserDefinedNewInstanceMethodMapping
  • UserDefinedNewInstanceRuntimeTargetTypeMapping
  • UserDefinedNewInstanceRuntimeTargetTypeParameterMapping
• I found the below types don't need the enableAggressiveInlinig parameter, so passed the false to them. (although all tests were passed, I'm not sure it's correct or if our tests are not complete enough to catch if there is any problem with it. What do you think?)
  • NoOpMapping
  • UnimplementedMapping

Support for UnsafeAccessor methods:

• According to this benchmark AggressiveInlining works fine with UnsafeAccessor methods and is beneficial.
• 
• I added a bool argument named enableAggressiveInlinig to the below types and changed DescriptorBuilder class a bit to read the Configuration and pass the value to that parameter.
  • UnsafeAccessorContext
  • UnsafeConstructorAccessor
  • UnsafeFieldAccessor
  • UnsafeGetPropertyAccessor
  • UnsafeSetPropertyAccessor

Testing:

I ensured all the previous tests were passed correctly and added some new tests to ensure it works fine in different scenarios.

Final benchmark (the outcome🤩):

I added a new benchmark and the result eliminated all my fatigue :)

Documentation:

The .mdx files and the tools that is used for docs were unfamiliar to me, I'm trying to learn it and need some help.
How can I build/run the docs and see the preview of the output?

CI/CD Pipeline:

I'm trying to run/pass most of the workflows locally or on my fork using GitHub Actions.
Could you approve and run the pipeline manually to check out the output?

The checklist:

• Implement feature
• Pass the previous tests (since the feature is enabled by default, all the unit/integration tests use it and were passed)
• Add more tests for different scenarios
• Add benchmarks
• Complete the documents (in progress)
• Pass the CI/CD tests locally (in progress)

---

Update:

I tested the CI/CD pipeline on my branch and this is the result of all workflows:

• benchmark: ✔passed
• lint: ✔passed
• draft-releases: ✔passed
• test: ✔passed
• docs: ✘ failed during the deploy job due to "Version not set".
• pr-validation: ✘ failed due to "PR has no changelog label".
• release: ✘ failed during the release step due to "release not found".
• code-ql: ✘ failed during the build step due to the exception "Partial method StaticMapperWithAggressiveInlining.Map(MyComplexClass) must have an implementation part because it has accessibility modifiers."

I have the same issue when running integration tests in Visual Studio (version 17.11.1) however there is no problem with dotnet cli.
The source generator does not generate mapping methods for all partial methods and throws exceptions like:

Partial method 'xxx' must have an implementation part because it has accessibility modifiers.

What's the problem? could you help me?

[latonz]

Thanks again for the great updates and sorry for my slow response. Since this is your first contribution to Mapperly I need to manually approve each CI run 🤷

• Configuration looks great 😊
• UnsafeAccessors: I don't think the attribute has any effect on the extern accessor methods. Have you read / tested anything related to this? Otherwise I would revert these code changes to keep the code as simple as possible, the related tests could also be removed.
• The implementation looks quite good. I noticed that with the current approach the enableAggressiveInlining needs to be passed to each and every method mapping instance. I think this could probably be simplified with the following approach:
  • Similar approach to the reference handling (DescriptorBuilder.BuildReferenceHandlingParameters and MethodMapping.EnableReferenceHandling)...
  • Add a method DescriptorBuilder.EnableAggressiveInlining calling it in DescriptorBuilder.Build just after BuildReferenceHandlingParameters
  • In this method if aggressive inlining is disabled, return early, otherwise loop through _mappings.MethodMappings and call a new method EnableAggressiveInlining
  • Add bool _hasMethodImplAttribute to the MethodMapping, defaulting to false and for all user-defined methods setting it to true when the method impl attribute is present.
  • Add a method MethodMapping.EnableAggressiveInlining which sets _enableAggressiveInlining to !_hasMethodImplAttribute
    This would have the great benefit that it would "just work" for all method mappings and there is no need to carry the enableAggressiveInlining around. I'm really sorry that I didn't come up with this idea earlier, I just had it while reviewing 🙄 WDYT?
• Small comment for the tests, otherwise they look good 😊
• Documentation: there is a bit of contributors documentation on how to update Mapperly's documentation here, hope that helps. (npm i + npm run start should help).
• CI: i approved the run and set the changelog labels. As noted in the review comment, I'd not add the benchmark to main as I don't see a benefit in continuously monitoring the performance difference with enabled/disabled aggressive inlining. All other jobs seem to be green 🥳

[latonz]

I don't think setting the attribute on the external accessor methods doesn't have an effect. Therefore all unsafe access code changes could probably be reverted.

[latonz]

This benchmark is great for this PR, but I don't think that this adds value on the long run. There is IMO no need to continuously monitor the performance difference between enabled/disabled aggressive inlining. Therefore I would not merge this into main.

[latonz]

If the new configuration option is added to TestSourceBuilderOptions, this can be simplified using the TestSourceBuilder.Mapping method. See also other tests calling this method. Same for the other tests in this class.

[latonz]

I'll close this for now... If you don't have the time/motivation to update again, I'll do it as soon as I find the time.

[mjebrahimi]

Unfortunately, I have been too busy and didn't have enough time to complete it.
Hope for future contributions :)