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

Add new DiagnosticMethodInfo public API #103220

Merged
merged 6 commits into from
Jun 13, 2024

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jun 10, 2024

This add support for the new public API for all runtime flavors and switches Tasks logging to use it (we could make it a separate PR, I don't mind either way). Switching tasks logging to the new API saves 1.1% in size on webapiaot because the compiler no longer needs to consider all delegates targets of reflection.

TODO:

  • Feature switch to disable this outside native AOT so that <StackTraceSupport>false</StackTraceSupport> really means this doesn't work.
  • Trimming test with the feature switch disabled
  • Native AOT smoke test to exercise some of the "don't have reflection metadata" paths. libraries tests always have reflection metadata for everything.
  • Maybe also use this in BCL instead of get_Method.
  • Finish the feature switch (add ILLink XML)

#96528

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

@MichalStrehovsky MichalStrehovsky marked this pull request as draft June 10, 2024 09:22
Comment on lines +12 to +13
DiagnosticMethodInfo? dmi = DiagnosticMethodInfo.Create(@delegate);
return dmi?.Name ?? "<unknown>";
Copy link
Member Author

Choose a reason for hiding this comment

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

This could alternatively be this, but not sure how important that is:

Suggested change
DiagnosticMethodInfo? dmi = DiagnosticMethodInfo.Create(@delegate);
return dmi?.Name ?? "<unknown>";
if (StackTrace.IsSupported)
{
DiagnosticMethodInfo? dmi = DiagnosticMethodInfo.Create(@delegate);
return dmi!.Name;
}
else
{
return @delegate.Method.Name;
}

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

1.1% size savings on webapiaot! MichalStrehovsky/rt-sz#29 (comment)

@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review June 12, 2024 08:37
@MichalStrehovsky
Copy link
Member Author

This is ready for review, cc @dotnet/ilc-contrib

@@ -1681,7 +1681,7 @@ internal void ScheduleAndStart(bool needsProtection)
{
// For all other task than TaskContinuations we want to log. TaskContinuations log in their constructor
Debug.Assert(m_action != null, "Must have a delegate to be in ScheduleAndStart");
TplEventSource.Log.TraceOperationBegin(this.Id, "Task: " + m_action.Method.Name, 0);
TplEventSource.Log.TraceOperationBegin(this.Id, "Task: " + m_action.GetMethodName(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Is this measurably more expensive than the existing code on regular CoreCLR?

Copy link
Member Author

Choose a reason for hiding this comment

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

On regular coreclr there's the one extra allocation but otherwise I don't expect impact. DiagnosticMethodInfo just wraps the underlying MethodInfo.

On native AOT we unnecessarily materialize type name and class name. I thought about structuring DiagnosticMethodInfo so that it wraps MetadataReader and a bunch of handles but ended up not doing it because we'd still need strings for the open method delegate cases (or somehow wrestle metadata handles out of reflection objects in that case).

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.

LGTM!

@MichalStrehovsky MichalStrehovsky merged commit 5b79b76 into dotnet:main Jun 13, 2024
144 of 147 checks passed
@MichalStrehovsky MichalStrehovsky deleted the diagmethodinfo branch June 13, 2024 07:45
@github-actions github-actions bot locked and limited conversation to collaborators Jul 13, 2024
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