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

Don't emit nor generate the manifest for NativeRuntimeEventSource #86850

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

nickyMcDonald
Copy link
Contributor

In issue #83127 @devmason requests that the manifest for NativeRuntimeEventSource i.e., Microsoft-Windows-DotNETRuntime no longer be generated.

This PR intends to rectify that issue by adding optional functionality to the ManifestBuilder to continue with all its checks, however at a much lower memory cost. And have the NativeRuntimeEventSource utilize this functionality.

After building and testing the runtime changes in DotNet7 here are some results.

We can see that there are far fewer string and char[] allocations and no big byte[] allocation which makes sense considering that was the intent.

DotNet7 no changes:
OldNet7Screenshot 2023-05-28 110155

DotNet7 with changes:
NewNET7Screenshot 2023-05-28 105813

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 28, 2023
@ghost
Copy link

ghost commented May 28, 2023

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

Issue Details

In issue #83127 @devmason requests that the manifest for NativeRuntimeEventSource i.e., Microsoft-Windows-DotNETRuntime no longer be generated.

This PR intends to rectify that issue by adding optional functionality to the ManifestBuilder to continue with all its checks, however at a much lower memory cost. And have the NativeRuntimeEventSource utilize this functionality.

After building and testing the runtime changes in DotNet7 here are some results.

We can see that there are far fewer string and char[] allocations and no big byte[] allocation which makes sense considering that was the intent.

DotNet7 no changes:
OldNet7Screenshot 2023-05-28 110155

DotNet7 with changes:
NewNET7Screenshot 2023-05-28 105813

Author: n77y
Assignees: -
Labels:

area-System.Diagnostics.Tracing, community-contribution

Milestone: -

Copy link
Member

@davmason davmason 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 for being so patient. I took a look and I think it is correct from an implementation point of view, but there are some formatting things I would like you to change.

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Jun 17, 2023
@nickyMcDonald nickyMcDonald requested a review from davmason June 18, 2023 11:06
Copy link
Member

@davmason davmason left a comment

Choose a reason for hiding this comment

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

Thanks for sticking with this!

@davmason davmason merged commit 548aafd into dotnet:main Jun 30, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jul 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Tracing community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants