-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 SDK support for perfmap format version #19132
Conversation
This PR addresses the remaining work outlined in the issue https://github.com/dotnet/sdk/issues/18813 As of .NET 6 Preview 7 Crossgen2 supports the option --perfmap-format-version that currently supports two values, 0 (being the legacy value / default) and 1; the only difference is that version 1 uses a different naming scheme for the output perfmap files, dropping the {MVID} part and using the extension ".ni.r2rmap". This PR adds support for the new option in the SDK Crossgen2 publishing logic; for .NET 5 and prior, we naturally need to stick to the legacy naming; for .NET 6, SDK honors a new property PublishReadyToRunPerfmapFormatVersion that can be used to override the default. As of the 1st commit on this PR, the SDK default is set to 1 i.e. the "new format". This basically means that from the point of merging this PR onwards SDK will by default produce the new perfmap file names on Linux. Similarly, once SDK preview 7 combines with the runtime repo (which will likely happen as part of the RC1 fork), the installer partition will start producing the new naming style for the Crossgen2-compiled framework. As this is technically a breaking change, we need to discuss whether that's acceptable (e.g. if there was no prior support for perfmap indexation, we probably don't need to care much); if we need tighter control over the perfmap versioning / naming, there are two different things we can do: 1) For now change the PerfmapFormatVersion default to 0 in the SDK Microsoft.NET.CrossGen.targets script. This means that perfmap files will continue to use the legacy naming scheme until someone explicitly switches over runtime or the SDK itself later on in the future. 2) We can make a counterpart check-in to the runtime repo setting PublishReadyToRunPerfmapFormatVersion to 0 in the installer ReadyToRun.targets script; once we're confident to switch over, we'll just delete the property from the scripts. I'm still hitting a couple of errors in local testing but I'm publishing the PR anyway as I'm hoping to solicit early feedback for the change. Thanks Tomas
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Tomas!
This PR addresses the remaining work outlined in the issue
https://github.com/dotnet/sdk/issues/18813
As of .NET 6 Preview 7 Crossgen2 supports the option
--perfmap-format-version that currently supports two values, 0
(being the legacy value / default) and 1; the only difference is
that version 1 uses a different naming scheme for the output
perfmap files, dropping the {MVID} part and using the extension
".ni.r2rmap".
This PR adds support for the new option in the SDK Crossgen2
publishing logic; for .NET 5 and prior, we naturally need to stick
to the legacy naming; for .NET 6, SDK honors a new property
PublishReadyToRunPerfmapFormatVersion that can be used to override
the default.
As of the 1st commit on this PR, the SDK default is set to 1 i.e.
the "new format". This basically means that from the point of
merging this PR onwards SDK will by default produce the new
perfmap file names on Linux.
Similarly, once the current SDK combines with the runtime repo
(which will likely happen as part of the RC1 fork), the installer
partition will start producing the new naming style for the
Crossgen2-compiled framework.
As this is technically a breaking change, we need to discuss whether
that's acceptable (e.g. if there was no prior support for perfmap
indexation, we probably don't need to care much); if we need tighter
control over the perfmap versioning / naming, there are two different
things we can do:
For now change the PublishReadyToRunPerfmapFormatVersion default
to 0 in the SDK Microsoft.NET.CrossGen.targets script. This means that
perfmap files will continue to use the legacy naming scheme until someone
explicitly switches over runtime or the SDK itself later on in the future.
We can make a counterpart check-in to the runtime repo setting
PublishReadyToRunPerfmapFormatVersion to 0 in the installer
ReadyToRun.targets script; once we're confident to switch over,
we'll just delete the property from the script.
I'm still hitting a couple of errors in local testing but I'm
publishing the PR anyway as I'm hoping to solicit early feedback for the
change.
Thanks
Tomas
/cc @dotnet/crossgen-contrib