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

Switch to full NGEN #6764

Merged
merged 6 commits into from
Aug 27, 2021
Merged

Switch to full NGEN #6764

merged 6 commits into from
Aug 27, 2021

Conversation

ladipro
Copy link
Member

@ladipro ladipro commented Aug 17, 2021

Context

We are currently pre-compiling only code that gets executed as part of our IBC training scenarios. This results in smaller native images but the coverage is not perfect and it's easy to miss a code path and cause JITting at run-time.

Changes Made

With Visual Studio switching to 64-bit, address space is no longer a concern and the positive impact of pre-compiling everything outweighs the cost of larger image sizes.

Testing

Experimental VS insertion showing improvements in # of methods JITted and wall-clock time.

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.

LGTM per our offline discussion of "as long as it's ok with the VS perf folks".

Are we missing MSBuild.exe?

@ladipro
Copy link
Member Author

ladipro commented Aug 17, 2021

Are we missing MSBuild.exe?

I don't see IBC data generated for MSBuild.exe here: internal link

Do you happen to know how the set of assemblies to profile with OptProf is defined?

@rainersigwald
Copy link
Member

I don't, unfortunately. This is promising but blank:

"assemblies": [
]

@ladipro
Copy link
Member Author

ladipro commented Aug 17, 2021

I've pushed a shot in the dark commit. Will have results tomorrow.

@@ -10,10 +10,12 @@ vs.dependencies
vs.dependency id=Microsoft.VisualStudio.PackageGroup.NuGet

vs.relatedProcessFiles
vs.relatedProcessFile Path="[InstallDir]\MSBuild\Current\Bin\MSBuild.exe"
Copy link
Member

@rainersigwald rainersigwald Aug 17, 2021

Choose a reason for hiding this comment

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

As I recall relatedProcessFile means "use the RestartManager API to see if this file is in use and if so complain to the user rather than failing to overwrite it during setup/upgrade."

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, it didn't work.

@genlu, may I ask for your expert advice here? Does setting the ApplyNgenOptimization prop enable OptProf end-to-end or is there some other place where we indicate for which assemblies optimization data should be created?

This is our OptProf training pipeline: internal link

Copy link
Member Author

Choose a reason for hiding this comment

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

ApplyNgenOptimization doesn't do it, we need to enable generating optimization data for the assembly first.

##[error].packages\microsoft.dotnet.arcade.sdk\6.0.0-beta.21379.2\tools\OptimizationData.targets(111,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) No optimization data found for assemblies: C:\a\1\s\artifacts\obj\MSBuild\Release\net472\MSBuild.exe

Let's get this in then and work on MSBuild.exe separately.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry I missed your question, notification from msbuild repo is being filtered out from my inbox. Please let me know if there's anything else I could help :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@genlu yes, the question is still valid. How does the OptProf infrastructure determine for which assemblies it should create optimization data? We noticed that it's not being done for MSBuild.exe and we would like to fix it. Thank you!

Copy link
Member

@genlu genlu Sep 16, 2021

Choose a reason for hiding this comment

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

This is specified in our optprof runsetting file, which points to the config files we update with each of our insertion (in the directory shown in the screenshot below)
image

The content of those config files are auto-generated based on this file during our build.

@@ -20,7 +20,7 @@
<IsPackable>true</IsPackable>
<PackageDescription>This package contains the $(MSBuildProjectName) assembly which is used to create, edit, and evaluate MSBuild projects.</PackageDescription>
<IncludeSatelliteOutputInPack>false</IncludeSatelliteOutputInPack>
<ApplyNgenOptimization Condition="'$(TargetFramework)' == '$(FullFrameworkTFM)'">partial</ApplyNgenOptimization>
<ApplyNgenOptimization Condition="'$(TargetFramework)' == '$(FullFrameworkTFM)'">full</ApplyNgenOptimization>
Copy link
Member

Choose a reason for hiding this comment

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

Why are all these only on full?

I was momentarily confused as to why you'd have to change it for four projects individually rather than just one change for them all, but it doesn't really make sense to ngen anything from other assemblies, so 👍

Copy link
Member

Choose a reason for hiding this comment

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

Why are all these only on full?

ngen is only a thing on .NET Framework; in .NET Core it was replaced with "crossgen" (and now crossgen2) which has a different set of rules.

Copy link
Member

Choose a reason for hiding this comment

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

I think you've told me that before...I'll try to remember this time.

@rainersigwald
Copy link
Member

For the record, I'm fine with taking this (without the MSBuild.exe changes) and then separately figuring out MSBuild.exe training data.

@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 Aug 19, 2021
@Forgind
Copy link
Member

Forgind commented Aug 19, 2021

We got buy-off for this part (that is, without MSBuild.exe changes) from the VS perf team, right? So this is good to merge when possible?

@ladipro
Copy link
Member Author

ladipro commented Aug 19, 2021

Yes, please merge.

@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 Aug 19, 2021
@Forgind Forgind merged commit 414393f into main Aug 27, 2021
@ladipro ladipro deleted the exp/full-ngen branch October 25, 2021 11:19
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. performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants