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

[release/6.0] Fix ReadyToRun loading on Apple Silicon #64104

Merged
merged 3 commits into from
Feb 7, 2022

Conversation

jgiannuzzi
Copy link
Contributor

@jgiannuzzi jgiannuzzi commented Jan 21, 2022

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.

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.


Fixes #64103.

This PR aims at backporting a subset of the changes introduced in #61938 and #62855. The rationale behind this change is explained in more details in #64103. I believe that this change would be limited to osx-arm64 and introduce a substantial performance gain by enabling ReadyToRun on Apple Silicon, which I imagine was supposed to work in .NET 6 in the first place.

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). Given the focus on performance in .NET 6, I would argue that this has to be fixed in .NET 6.

@dotnet-issue-labeler
Copy link

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.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 21, 2022
@dnfadmin
Copy link

dnfadmin commented Jan 21, 2022

CLA assistant check
All CLA requirements met.

@VSadov
Copy link
Member

VSadov commented Jan 24, 2022

Changes in map and layout look good. Thanks!

Copy link
Member

@trylek trylek left a comment

Choose a reason for hiding this comment

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

LGTM but my experience with Apple Silicon is very limited, I'd feel more comfortable if JanV could take a look as well.

Copy link
Member

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

@jgiannuzzi jgiannuzzi changed the title Fix ReadyToRun loading on Apple Silicon (.NET 6) [release/6.0] Fix ReadyToRun loading on Apple Silicon Jan 26, 2022
@jgiannuzzi
Copy link
Contributor Author

I am not sure whether the current test failures are due to my changes or not.
@jkotas @trylek @VSadov do you have any pointers to help me solve this?

@janvorli
Copy link
Member

janvorli commented Jan 27, 2022

The JIT\Directed\rvastatics\RVAOrderingTest is failing on both windows and x64 / arm64 Linux and OSX.
The Loader\binding\tracing\BinderTracingTest.Basic is failing on Windows x64 / arm64 only.
However, the change doesn't touch any Windows and Linux behavior (the changes are under ifdefs so that they occur only on macOS and one of them on Linux too), so it is not clear to me how it could cause these failures.

@jgiannuzzi
Copy link
Contributor Author

I have tried running the JIT/Directed/rvastatics/RVAOrderingTest test locally with CompositeBuildMode=1 and RunCrossGen2=1.
On Linux x64, Linux arm64, and macOS x64, they fail with or without my changes (e.g. 6de5c5b – which my branch is based on – fails, but also bbfc15c – which is the current tip of release/6.0).
On macOS arm64, they only fail after 6e24b2d is applied (my commit enabling R2R loading), which makes sense as without this commit the composite R2R dll does not get loaded and the test is then equivalent to not having RunCrossGen2=1 set.
I did not try on Windows, but I expect the results would be similar.

@jgiannuzzi
Copy link
Contributor Author

Update: the same test also fails on main (869ae1a at the time of writing) and on v6.0.0 on Linux arm64 (and potentially more platforms, but I only tested that one).
There seems to be a bug in composite crossgen2 that copies only the referenced static data, but not the rest. It results in having only s_First, s_Last, and s_1 (referenced as s_Another1) being copied, leaving a bunch of data between s_First and s_Last out.

@jkotas @trylek @VSadov @janvorli what is the best course of action for this particular PR? Is there anything more that is needed to get it merged?
I would love to see it part of the next release so that I can finally setup an osx-arm64 CI pipeline for ParquetSharp using a macOS VM!

@jkotas
Copy link
Member

jkotas commented Feb 2, 2022

what is the best course of action for this particular PR? Is there anything more that is needed to get it merged?

I have filled the request to get it approved for .NET 6 serving. Thank you for your help with this fix!

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. We will take for consideration in 6.0.x.

cc @kdubau and @safern

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

kdubau commented Feb 2, 2022

Thanks @jeffschwMSFT, as of right now we have no immediate plan to enable R2R in VS4Mac. cc @slluis FYI

@jeffschwMSFT
Copy link
Member

@kdubau unless the r2r is stripped from NetCoreApp you would benefit from this change as those packages have r2r enabled.

@leecow leecow removed the Servicing-consider Issue for next servicing release review label Feb 3, 2022
@leecow leecow added the Servicing-approved Approved for servicing release label Feb 3, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.3 Feb 3, 2022
@safern
Copy link
Member

safern commented Feb 7, 2022

@trylek are these crossgen2 failures related? Can we merge this?

@jeffschwMSFT
Copy link
Member

@safern I asked the same question. These failures are unrelated, we just ran more outerloops. It is good to merge.

@safern safern merged commit 8f7dff3 into dotnet:release/6.0 Feb 7, 2022
@safern
Copy link
Member

safern commented Feb 7, 2022

Thanks for confirming.

mangod9 added a commit that referenced this pull request Feb 23, 2022
jeffschwMSFT added a commit that referenced this pull request Feb 23, 2022
Revert "[release/6.0] Fix ReadyToRun loading on Apple Silicon (#64104)"
@ghost ghost locked as resolved and limited conversation to collaborators Mar 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ReadyToRun-coreclr community-contribution Indicates that the PR has been added by a community member Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants