Skip to content
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

Try rooting less stuff for reflectable method signatures #92994

Merged
merged 1 commit into from
Nov 6, 2023

Conversation

MichalStrehovsky
Copy link
Member

I don't think we need a working MethodTable for everything in the method signature.

@Sergio0694 also noticed IUnknown** roots IUnknown. This doesn't address that (just reference types), but maybe non-ref pointers could be workable in a similar way (we mostly go for IntPtr?).

@ghost
Copy link

ghost commented Oct 4, 2023

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

Issue Details

I don't think we need a working MethodTable for everything in the method signature.

@Sergio0694 also noticed IUnknown** roots IUnknown. This doesn't address that (just reference types), but maybe non-ref pointers could be workable in a similar way (we mostly go for IntPtr?).

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review October 5, 2023 06:45
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

@Sergio0694 also noticed IUnknown** roots IUnknown. This doesn't address that (just reference types), but maybe non-ref pointers could be workable in a similar way (we mostly go for IntPtr?).

So the pointers are a lot trickier and I'd need to add even more special casing for them on the classlib side. It would probably not bring any meaningful savings for normal .NET apps, but bring a lot of risk that we regress something (we'd need to duplicate a bunch of intricate rules such as "method that takes a pointer to SomeEnum can also be invoked by Pointer.Boxing int*", etc.).

I'm already a bit uncomfortable with this change that limits it to reference types only, but it does save 0.1% on ASP.NET scenarios. We'd get triple that if we could also do this for value types, but it doesn't look likely we can. Pointers would be noise-level savings.

@jkotas no hard feelings if you say doing this for reference types is not worth it either. It does add a bit of mental overhead to DynamicInvokeInfo and 0.1% might not be worth the trouble. I was hoping for more. It could be more for some apps.

Cc @dotnet/ilc-contrib

@jkotas
Copy link
Member

jkotas commented Oct 6, 2023

It does add a bit of mental overhead to DynamicInvokeInfo

I do not think that this mental overhead is a big deal. I see this as small change giving us small improvement.

@MichalStrehovsky MichalStrehovsky merged commit be45599 into dotnet:main Nov 6, 2023
118 of 120 checks passed
@MichalStrehovsky MichalStrehovsky deleted the lessstuff branch November 6, 2023 22:14
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants