Skip to content

[release/6.0] Load R2R images on osx-arm64 according to Apple's guidelines #67438

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 13, 2022
Merged

[release/6.0] Load R2R images on osx-arm64 according to Apple's guidelines #67438

merged 3 commits into from
Apr 13, 2022

Conversation

jgiannuzzi
Copy link
Contributor

@jgiannuzzi jgiannuzzi commented Apr 1, 2022 •
edited by mangod9
Loading

Customer Impact

Slow startup of .NET applications on Apple Silicon. ReadyToRun images are not loaded correctly on Apple Silicon. All methods are JITed during startup.

For reference, compiling a medium-size project like ILGPU takes ~17s on a Mac Mini M1 before this change, and ~13s after this change (about 30% faster).

Testing

crossgen2 outerloop tests. Manually verified that the ReadyToRun images are loaded correctly on Apple Silicon and Debugging of managed apps works.

Risk

Low risk. These fixes have been in dotnet/runtime:main for more than a month, however there is a risk of discovering latent Apple Silicon specific bugs in ReadyToRun with this fix.

Closes #64103.

This PR reactivates the previously backported #64104 and fixes its impact on debugging by backporting #67118.
I believe that this change would be limited to osx-arm64 and introduce a substantial performance gain by enabling ReadyToRun on Apple Silicon, which was supposed to work in .NET 6 in the first place but never did.

More details about the rationale of this change can be found in #64103, #64104, #65341, and #67118.

@ghost ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Apr 1, 2022
@ghost
Copy link

ghost commented Apr 1, 2022

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

Issue Details

Closes #64103.

This PR reactivates the previously backported #64104 and fixes its impact on debugging by backporting #67118.
I believe that this change would be limited to osx-arm64 and introduce a substantial performance gain by enabling ReadyToRun on Apple Silicon, which was supposed to work in .NET 6 in the first place but never did.

More details about the rationale of this change can be found in #64103, #64104, #65341, and #67118.

Author: jgiannuzzi
Assignees: -
Labels:

area-CodeGen-coreclr
Milestone: -

@jgiannuzzi
Copy link
Contributor Author

@janvorli @hoyosjs @jkotas @VSadov @mangod9 could you please review?

@jgiannuzzi jgiannuzzi mentioned this pull request Apr 1, 2022
Closed
janvorli
janvorli approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

jeffschwMSFT
jeffschwMSFT approved these changes Apr 1, 2022
View reviewed changes
Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. Please check on the pr failure and fill in the template. We will take for consideration in 6.0.x

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Apr 1, 2022
@jeffschwMSFT jeffschwMSFT added this to the 6.0.x milestone Apr 1, 2022
@mangod9
Copy link
Member

mangod9 commented Apr 1, 2022

Added the template, the SingleFile failure seems to be infra related:
Helix API does not contain an entry for Windows.10.Amd64.Server19H1.ES.Open

will try rerunning.

@mangod9
Copy link
Member

mangod9 commented Apr 1, 2022

Failed again, I have created a new infra issue for that.

@jgiannuzzi
Copy link
Contributor Author

Hi @mangod9, do you have any news about the test infra issue?

@JulieLeeMSFT
Copy link
Member

Should we relabel this to area-ReadyToRun-coreclr instead of codegen?

@leecow leecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Apr 5, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.5 Apr 5, 2022
hoyosjs
hoyosjs approved these changes Apr 6, 2022
View reviewed changes
Copy link
Member

@hoyosjs hoyosjs left a comment

Choose a reason for hiding this comment

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

Validated from a debugger perspective - the code running is r2r and the debugger properly reports exceptions and stacks. Thanks!

@hoyosjs
Copy link
Member

hoyosjs commented Apr 6, 2022

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@hoyosjs
Copy link
Member

hoyosjs commented Apr 6, 2022

Failure is #64153, arm64 queue is not starting work - it might be backlogged.

@runfoapp runfoapp bot mentioned this pull request Apr 6, 2022
Open
@carlossanlop carlossanlop merged commit 9c81d35 into dotnet:release/6.0 Apr 13, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jeffschwMSFT jeffschwMSFT jeffschwMSFT approved these changes

@janvorli janvorli janvorli approved these changes

@hoyosjs hoyosjs hoyosjs approved these changes

@mangod9 mangod9 mangod9 approved these changes

Assignees
No one assigned
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member Servicing-approved Approved for servicing release
Projects
None yet
Milestone
6.0.5
Development

Successfully merging this pull request may close these issues.
8 participants
@jgiannuzzi @mangod9 @JulieLeeMSFT @hoyosjs @jeffschwMSFT @janvorli @carlossanlop @leecow