Skip to content

ngen arm64 binaries as arm64#7975

Merged
Forgind merged 1 commit intodotnet:mainfrom
benvillalobos:arm64-ngen
Oct 10, 2022
Merged

ngen arm64 binaries as arm64#7975
Forgind merged 1 commit intodotnet:mainfrom
benvillalobos:arm64-ngen

Conversation

@benvillalobos
Copy link
Member

@benvillalobos benvillalobos commented Sep 15, 2022

Customer Impact

Slower arm64 MSBuild due to assemblies NOT being ngen'd for the arm64 architecture.

Testing

Verified no regressions in https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/426628

Code Reviewers

raines
Forgind

Description of fix

Change setting in our .swr file to ngen relevant assemblies as arm64.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

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

Our msbuild binaries mostly reference what's in bin/, but if we're on an arm64 machine and we reference what's in bin/, don't those need to be ngen'd for arm64?

They specify ngenArchitecture=all

file source=$(X86BinPath)Microsoft.Build.dll vs.file.ngenApplications="[installDir]\Common7\IDE\vsn.exe" vs.file.ngenApplications="[installDir]\MSBuild\Current\Bin\MSBuild.exe" vs.file.ngenArchitecture=all vs.file.ngenPriority=1

so IMO if that's not working it's a VS Setup engine problem that we should talk to that team about.

@Forgind
Copy link
Contributor

Forgind commented Sep 15, 2022

We're supposed to have arm64 bits on non-arm machines too, right? So then would this mean they wouldn't be ngen'd for that scenario? Is the decision here that arm64 bits are most important on arm machines, so if there's a VS setup issue, we should prioritize arm-on-arm for now?

@kasperk81
Copy link
Contributor

isn't ngen deprecated?
PublishAot with OutputType=Library (default) gets you native share lib on windows, linux and mac.
some os/architectures don't support aot, there fallback to PublishReadyToRun for libraries and PublishSingleFile for executable.

@rainersigwald
Copy link
Member

isn't ngen deprecated?

ngen is the best JIT-reduction/AOT technology available on .NET Framework 4.8, which is relevant to MSBuild because it's what Visual Studio, one of our major partners, uses.

In the .NET SDK on .NET 6/7, we use the newer crossgen approach.

@benvillalobos
Copy link
Member Author

Testing this here: https://dev.azure.com/devdiv/DevDiv/_build/results?buildId=6707696&view=results if it gets to the VS PR stage with no issues then we can merge this

@Forgind
Copy link
Contributor

Forgind commented Sep 26, 2022

@benvillalobos, looks like you hit a missing optprof data issue. You presumably have to retarget where you're getting your data from the same way we did for main.

@benvillalobos
Copy link
Member Author

I kicked off a separate pipeline build with the right data, but didn't notice the release failed. Kicked another one and that seems to be moving forward: https://dev.azure.com/devdiv/DevDiv/_releaseProgress?_a=release-pipeline-progress&releaseId=1395478

@benvillalobos
Copy link
Member Author

@benvillalobos
Copy link
Member Author

Passed RPS, this should be good to go.

@Forgind Forgind added merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. and removed merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. labels Oct 3, 2022
@rainersigwald rainersigwald added this to the VS 17.4 milestone Oct 3, 2022
@Forgind Forgind modified the milestones: VS 17.4, VS 17.5 Oct 6, 2022
@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Oct 7, 2022
@Forgind Forgind merged commit 1c76c5f into dotnet:main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants