Skip to content

Conversation

@trylek
Copy link
Member

@trylek trylek commented Jul 21, 2021

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:

  1. 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.

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

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

ghost commented Jul 21, 2021

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.

@trylek trylek self-assigned this Jul 21, 2021
@trylek trylek requested review from davmason and jkotas July 21, 2021 23:16
Copy link
Member

@AntonLapounov AntonLapounov left a comment

Choose a reason for hiding this comment

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

Looks good!

Copy link

@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 Tomas!

@trylek trylek merged commit 43c8f12 into dotnet:main Jul 23, 2021
@trylek trylek deleted the PerfmapFormatVersion branch July 23, 2021 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants