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

[draft] Improve startup and warmup performance for invoke by reducing emit usage #109901

Draft
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

steveharter
Copy link
Member

[verifying tests]

@steveharter steveharter added this to the 10.0.0 milestone Nov 17, 2024
@steveharter steveharter self-assigned this Nov 17, 2024
Copy link
Contributor

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

if (argLen > MaxStackAllocArgCount)
object? obj;

Span<object?> copyOfArgs;
Copy link
Member

Choose a reason for hiding this comment

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

Can you please declare the variables at the assignment below? I think it is a preferred style in the coreclr runtime and it makes it easier to see the actual type the "new" allocates.

@janvorli
Copy link
Member

@steveharter do you plan to add support for other well known signatures than the zero arg with return value and one arg with void return ones? Based on our past discussion, we would like to eventually add ones that are on startup path of common types of apps like e.g. aspnet ones. Or were these mostly just those two basic cases?

@steveharter
Copy link
Member Author

@steveharter do you plan to add support for other well known signatures than the zero arg with return value and one arg with void return ones? Based on our past discussion, we would like to eventually add ones that are on startup path of common types of apps like e.g. aspnet ones. Or were these mostly just those two basic cases?

That approach (zero arg + one arg) was for property getters\setters however it didn't work for instance methods since function pointers don't support instance methods (sometimes it worked for simple methods, but other times it cause heap corruption) so I had to change it to very-hard-coded list of specific types and methods used during startup. That could be extended for other types based on the applications we want to check that are sensitive to startup\warmup (e.g. ASP.NET, WinForms)

However, that is somewhat untenable so we should really get function pointers to support instance methods, so I created the language proposal at dotnet/csharplang#8709. This would automatically bring in a ton of all methods, perhaps up to 50%, since it would cover most property getters and setters and other simple methods.

Another approach, compatible with the function pointer + instance methods above, is to add an API to let the application specify their own delegate for method that they do not want to cause emit\jit. E.g.:

public class MethodBase
{
    // We would support only our built-in delegates that are for property getters\setters, <= 4 args, byref args, etc.
+    void SetInvokeImplementation(Delegate delegate);
}
   

@jkotas
Copy link
Member

jkotas commented Dec 3, 2024

function pointers to support instance methods,

We can emit the function-pointer based thunks for arbitrary instance methods as runtime intrinsics. We do not need first class C# support for instance method function pointers to make it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants