Skip to content

Add new DiagnosticMethodInfo public API#103220

Merged
MichalStrehovsky merged 6 commits into
dotnet:mainfrom
MichalStrehovsky:diagmethodinfo
Jun 13, 2024
Merged

Add new DiagnosticMethodInfo public API#103220
MichalStrehovsky merged 6 commits into
dotnet:mainfrom
MichalStrehovsky:diagmethodinfo

Conversation

@MichalStrehovsky

@MichalStrehovsky MichalStrehovsky commented Jun 10, 2024

Copy link
Copy Markdown
Member

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

@ghost

ghost commented Jun 10, 2024

Copy link
Copy Markdown

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
@ghost

ghost commented Jun 10, 2024

Copy link
Copy Markdown

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.

@dotnet-policy-service

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

/azp run runtime-nativeaot-outerloop

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky

Copy link
Copy Markdown
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
Copy Markdown
Member Author

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

// 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
Copy Markdown
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
Copy Markdown
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).

@jkotas jkotas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

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