Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Faster ConstructorInfo.Invoke #86855

Merged
merged 4 commits into from
May 29, 2023
Merged

Faster ConstructorInfo.Invoke #86855

merged 4 commits into from
May 29, 2023

Conversation

MichalStrehovsky
Copy link
Member

Speeds up reflection-invoking constructors by about 30%.

Noticed we spend some time in the ConstructorInfo invocation infrastructure for no good reason.

  • We'd first allocate an instance of the object using a general purpose allocator (that first needs to dissect the MethodTable to figure out how to allocate), and then
  • We'd call into general-purpose Invoke infrastructure that would validate the this is valid.

Neither of those are necessary - we can figure out the right allocator at the time the method invoker is first accessed, and validating this is not necessary because we just allocated the right one.

Cc @dotnet/ilc-contrib

Speeds up reflection-invoking constructors by about 30%.

Noticed we spend some time in the `ConstructorInfo` invocation infrastructure for no good reason.

* We'd first allocate an instance of the object using a general purpose allocator (that first needs to dissect the `MethodTable` to figure out how to allocate), and then
* We'd call into general-purpose Invoke infrastructure that would validate the `this` is valid.

Neither of those are necessary - we can figure out the right allocator at the time the method invoker is first accessed, and validating `this` is not necessary because we _just_ allocated the right one.
@ghost
Copy link

ghost commented May 29, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Speeds up reflection-invoking constructors by about 30%.

Noticed we spend some time in the ConstructorInfo invocation infrastructure for no good reason.

  • We'd first allocate an instance of the object using a general purpose allocator (that first needs to dissect the MethodTable to figure out how to allocate), and then
  • We'd call into general-purpose Invoke infrastructure that would validate the this is valid.

Neither of those are necessary - we can figure out the right allocator at the time the method invoker is first accessed, and validating this is not necessary because we just allocated the right one.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-System.Reflection

Milestone: -

@ghost
Copy link

ghost commented May 29, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

Speeds up reflection-invoking constructors by about 30%.

Noticed we spend some time in the ConstructorInfo invocation infrastructure for no good reason.

  • We'd first allocate an instance of the object using a general purpose allocator (that first needs to dissect the MethodTable to figure out how to allocate), and then
  • We'd call into general-purpose Invoke infrastructure that would validate the this is valid.

Neither of those are necessary - we can figure out the right allocator at the time the method invoker is first accessed, and validating this is not necessary because we just allocated the right one.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ivanpovazan
Copy link
Member

@lambdageek could something like this be done for Mono as well?

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@jkotas
Copy link
Member

jkotas commented May 29, 2023

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@VSadov
Copy link
Member

VSadov commented May 29, 2023

Noticed we spend some time in the ConstructorInfo invocation infrastructure for no good reason.

I've seen the use of ConstructorInfo.Invoke in ASP.net Stage2 app as well. From what I see it is caused by dependency injection, which wants to create service objects this way. This change should make the scenario faster.

Note that there is still an allocation of an object (and I assume this is ByDesign, since this is what DI wants to do), so in a GC-challenged case, it might limit the overall gains somewhat.

Either way this should be an improvement.

@jkotas jkotas merged commit 182591a into dotnet:main May 29, 2023
Comment on lines +111 to +116
private static class RawCalliHelper
{
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T Call<T>(delegate*<IntPtr, T> pfn, IntPtr arg)
=> pfn(arg);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI @yowl; this will need to be adjusted to use the same calling convention transform we do for Activator.CreateInstance in the next integration.

Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @yowl; this will need to be adjusted to use the same calling convention transform we do for Activator.CreateInstance in the next integration.

Is the issue that I used IntPtr instead of MethodTable*? Otherwise this should be just a "regular" calli. It's in a RawCalliHelper to avoid the fat pointer penalty (we know the target doesn't need a generic context) but otherwise just any other user calli.

Copy link
Contributor

Choose a reason for hiding this comment

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

No, the issue is the mismatch of calling conventions (equivalently, the assumption that they are the same between managed and native). This is a managed calli, so it will get the shadow stack argument which the native allocator methods do not have. We solve this downstream via detouring through native code. It could be solved upstream, but not sure there is appetite for that, additionally, I suspect we'll start using custom allocators that have the shadow stack argument sooner or later for performance reasons.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see, thanks and sorry for the extra trouble!

@MichalStrehovsky MichalStrehovsky deleted the ctorinvoke branch May 29, 2023 21:10
@MichalStrehovsky
Copy link
Member Author

I've seen the use of ConstructorInfo.Invoke in ASP.net Stage2 app as well. From what I see it is caused by dependency injection, which wants to create service objects this way. This change should make the scenario faster.

Note that there is still an allocation of an object (and I assume this is ByDesign, since this is what DI wants to do), so in a GC-challenged case, it might limit the overall gains somewhat.

Yep, saw this in Stage2 as well.

WRT the allocations, one place I noticed is this piece of questionable code:

https://github.com/dotnet/aspnetcore/blob/93bf9f4bbf740c570a72f38b62e11a42ebc13a0d/src/Security/Authentication/Core/src/AuthenticationHandler.cs#L119

Accessing RuntimeType.FullName doesn't allocate on CoreCLR, but it does on NativeAOT. It wouldn't explain too much of the difference, but some of it for sure. I've been itching to throw away the NativeAOT reflection stack in favor of the CoreCLR one so that we have the same perf characteristics, but it looks like a ton of work so I backtracked every time. But we need to do it at some point.

@davidfowl
Copy link
Member

That code is a performance problem for multiple reasons (#83609)

@ghost ghost locked as resolved and limited conversation to collaborators Jun 29, 2023
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants