Using AggressiveInlining for generated mapping methods

[mjebrahimi]

According to this benchmark, it shows using [MethodImpl(MethodImplOptions.AggressiveInlining)] for mapping structs is much faster than not using it.

I must confess that much performance difference was surprising to me but in fact, it is! and why don't we advantage of it? :)

The generated mapping methods are simple and seem safe to use with AggressiveInlining, so I think there's nothing wrong with it (at least it's worth a try).

Note: mapping methods in this benchmark are mapperly-generated and I just edited them to decorate with AggressiveInlining.

[mjebrahimi]

By the way, I updated my .NET Mappers Benchmark and wanted to share it with you @latonz

[latonz]

Thanks for the benchmark and the PR contribution. The performance improvements on the struct methods are impressive 😮👍 . I'm still not sure if applying aggressive inlining to all mapping methods is the best option. We should probably discuss this in more detail... My initial thoughts:

• What are the disadvantages of applying it to all methods?
  • Could there be an unexpected effect on the CPU caches?
  • If there are many callers, the size of the assembly could increase significantly.
    • A mapping method should not have many different callers, but in a large object hierarchy a method for an object mapping nested deep in the object hierarchy would be duplicated a lot.
• Why doesn't the compiler inline these methods without the attribute?
  • Does the benchmark lead to the same results if the mapper is static?
• As the attributes on partial methods are merged, could an alternative be to document this in detail and let the user apply the attribute to the partial definition themselves?
  • This won't work for nested methods generated by Mapperly...

What do you think?

[mjebrahimi]

I agree it needs to be discussed more.
Unfortunately, there is not enough information and documents for it on microsoft docs.
Some developers say something about it but most of them are not based on references and cannot be cited.

After a lot of research, the disadvantages that I'm aware and sure of, are:

• As you mentioned, inlining methods can result in duplicating code at each call site, which can increase the size of the compiled code. If the method being inlined is large or called from multiple locations, it impacts the code size and the memory footprint of the application.

• Also some wired behavior would happen if you intend to override that method at Runtime because the inlined method is compiled at the caller location. (probably it applies to some reflection use cases as well)

• Unnecessary use of this attribute can reduce performance. The attribute might cause implementation limits to be encountered that will result in slower generated code. (Always measure performance to ensure it's helpful to apply this attribute)

One thing that I read somewhere (not in docs/reference) but I'm not sure about:

• Longer Compilation Time: Inlining can lead to increased compilation time because the compiler needs to process and duplicate the code at each call site. If there are many inlined methods, it can significantly slow down the compilation process. (I'm not sure, maybe the compiler can cache the method and re-use it multiple times instead of compiling each time!)

There is another thing that is worth mentioning:

• Using AggressiveInlining does not guarantee that the method will be inlining, it's just a hint to give more chance/priority for inlining. if the compiler identifies it should not be inlined then it doesn't. (of course, the compiler is not smart enough to identify NOT qualified methods for inlining)

The only main advise that I have seen many places:

The .NET JIT compiler is designed to make intelligent inlining decisions. Trust it to handle the majority of inlining decisions, only use AggressiveInlining when you have clear evidence it will benefit performance.

About your questions:

but in a large object hierarchy a method for an object mapping nested deep in the object hierarchy would be duplicated a lot.

Yes, duplicate calls can leads to increased the size of the assembly, but on the otherside in a large object hierarchy with many nested mapping, using AggressiveInlining has more peformance improvement impact (as its main advantage is reducing method calls which applies more to every nested mapping methods)

Why doesn't the compiler inline these methods without the attribute?

The compiler is smart to identify most of the qualified methods for inlining but not smart enough to identify all (otherwise, it should not exist at all).
And that is why developers used aggressive inlining hints in many places in .net/aspnetcore/efcore /roslyn teams.

Does the benchmark lead to the same results if the mapper is static?

No there is not difference between static and instance mapper classes.
Note: I expanded the level of nested mapping methods to see better the impact of using AggressiveInlining.

As the attributes on partial methods are merged, could an alternative be to document this in detail and let the user apply the attribute to the partial definition themselves?

As you mentioned, it does not apply to nested methods, so we lose the main avantage of it (eliminating the cost of method calls)

As a conclusion I thinks it's worth to experiment this feature in real large projects within community and get feedback from them. So we can release this feature as a preview version of library to get feedbacks.

Thanks for bringing up various points to discuss, I'm looking forward to hearing your opinions and moving this discuss further.

[latonz]

Thank you for your research! My conclusion is this: I think it makes sense to implement it, but I think we need to think about the following:

• A configuration to enable/disable aggressive inlining
  • This ensures that it can be easily disabled if it doesn't work or has an unexpected effect.
  • Not sure what we should expose for this property, a boolean to enable/disable or a MethodImplOptions property that defaults to AggressiveInlining to allow other options to be set.
• If the MethodImpl attribute is present on the partial method definition, it should not be emitted by Mapperly on the generated method implementation.
  • This allows the user to override the attribute on certain methods.
  • This also prevents a major breaking change that could lead to duplicate attributes.

The main branch targets Mapperly 4.0 (major with breaking changes), so right now would be a good time to implement this 😊

[mjebrahimi]

Thanks for your good points! and sorry for my late resposding :)

A configuration to enable/disable aggressive inlining

I know your concern about the feature of this featuer but exposing an Enum property is not a good choies IMO.
Because other values of this enum has not much use-cases and also can lead to problems if the developer use it wrongly (such as MethodImplOptions.Synchronized)

I thinks it's better to keep it simple (KISS principle) like a bool property flag to enable/disable it.
Or maybe a 3 options enum (Disabled, Enabled, and EnabledForStruct) since appreantly it's benefital almost for struct types and is not significant for class types.

If the MethodImpl attribute is present on the partial method definition, it should not be emitted by Mapperly on the generated method implementation.

Why not emitting both user specified values and our disered value?
This enum is decorated with [System.Flags] so we can emit both values (user value and our value) togheter like (MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization).
If the user does not want to override by us, they can easily disable it by out exposing property.

This also prevents a major breaking change that could lead to duplicate attributes.

We can add AggressiveInlining only if it's not already existed to prevent duplicaion.

The main branch targets Mapperly 4.0 (major with breaking changes), so right now would be a good time to implement this 😊

Very good! so if you agree with any of these please let me know so I can help with the implementation.

[latonz]

My thoughts:

• Configuration on the MapperAttribute: I think exposing a flags enum property would fit the best:

public class MapperAttribute
{
+   public AggressiveInliningTypes AggressiveInliningTypes { get; set; } = AggressiveInliningTypes.All;
}

+ public enum AggressiveInliningTypes
+ {
+    None = 0,
+    ValueTypes = 1 << 0,
+    ReferenceTypes = 1 << 1,
+    All = ~None,
+ }

Isn't this

If the user does not want to override by us, they can easily disable it by out exposing property.
in contradiction to
We can add AggressiveInlining only if it's not already existed to prevent duplicaion.

How would overriding work with partial methods? If the user already defines the attribute on the partial definition (e.g. as [MethodImpl(MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization)], applying the attribute again (even with merged flags) on the implementation part of the method leads to a duplicated attribute, doesn't it?

Would be happy to review an implementation PR 😊

[latonz]

@mjebrahimi are you still interested in implementing this? 😊

[mjebrahimi]

Hi @latonz, sorry for the late response, I was on vacation. And thanks for waiting :)

I updated the code for PR #1384 (comment) according to our discussion.

[latonz]

I hope you had a wonderful vacation and thanks for the updates 😊