Skip to content

Clean up crossgen related build scripts also generate native symbols for R2R images #16297

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
TravisEz13 merged 3 commits into from
Oct 27, 2021
Merged

Clean up crossgen related build scripts also generate native symbols for R2R images #16297

TravisEz13 merged 3 commits into from
Oct 27, 2021

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Oct 22, 2021

PR Summary

Supersede #16265

The .NET team has confirmed that ALL .NET 6 runtime assemblies should be R2R images. See dotnet/runtime#60779 for details.
During my verification, I found some runtime assemblies that are not R2R images, but it turns out that is by design because those assemblies as pure façade assemblies. See dotnet/runtime#60782 for details.

I have also verified that <PublishReadyToRun>true</PublishReadyToRun> actually crossgen'ed all the package assemblies pwsh depends on, including all that in the $commonAssembliesForAddType array today. The following is the pdb generated for those crossgen'ed assemblies.

InputObject                                           SideIndicator
-----------                                           -------------
Markdig.Signed.ni.pdb                                 =>
Microsoft.ApplicationInsights.ni.pdb                  =>
Microsoft.CodeAnalysis.CSharp.ni.pdb                  =>
Microsoft.CodeAnalysis.ni.pdb                         =>
Microsoft.Extensions.ObjectPool.ni.pdb                =>
Microsoft.Management.Infrastructure.CimCmdlets.ni.pdb =>
Microsoft.Management.Infrastructure.Native.ni.pdb     =>
Microsoft.Management.Infrastructure.ni.pdb            =>
Microsoft.PowerShell.Commands.Diagnostics.ni.pdb      =>
Microsoft.PowerShell.Commands.Management.ni.pdb       =>
Microsoft.PowerShell.Commands.Utility.ni.pdb          =>
Microsoft.PowerShell.ConsoleHost.ni.pdb               =>
Microsoft.PowerShell.CoreCLR.Eventing.ni.pdb          =>
Microsoft.PowerShell.GraphicalHost.ni.pdb             =>
Microsoft.PowerShell.MarkdownRender.ni.pdb            =>
Microsoft.PowerShell.Security.ni.pdb                  =>
Microsoft.WSMan.Management.ni.pdb                     =>
Microsoft.WSMan.Runtime.ni.pdb                        =>
Namotion.Reflection.ni.pdb                            =>
Newtonsoft.Json.ni.pdb                                =>
NJsonSchema.ni.pdb                                    =>
pwsh.ni.pdb                                           =>
System.ComponentModel.Composition.ni.pdb              =>
System.ComponentModel.Composition.Registration.ni.pdb =>
System.Data.Odbc.ni.pdb                               =>
System.Data.OleDb.ni.pdb                              =>
System.Data.SqlClient.ni.pdb                          =>
System.DirectoryServices.AccountManagement.ni.pdb     =>
System.DirectoryServices.Protocols.ni.pdb             =>
System.IO.Ports.ni.pdb                                =>
System.Management.Automation.ni.pdb                   =>
System.Management.ni.pdb                              =>
System.Net.Http.WinHttpHandler.ni.pdb                 =>
System.Private.ServiceModel.ni.pdb                    =>
System.Reflection.Context.ni.pdb                      =>
System.Runtime.Caching.ni.pdb                         =>
System.ServiceModel.Syndication.ni.pdb                =>
System.ServiceProcess.ServiceController.ni.pdb        =>
System.Speech.ni.pdb                                  =>

Given all above, we should just remove the Start-CrossGen code, and also remove the -CrossGen switch parameter from Start-PSBuild, because we have <PublishReadyToRun Condition=" '$(Configuration)' != 'Debug' ">true</PublishReadyToRun> in PowerShell.Common.props file, so unless explicitly disabling R2R (for fxdependent* and minSize packages), our build will generate R2R images for release builds.

Also add <PublishReadyToRunEmitSymbols>true</PublishReadyToRunEmitSymbols> to generate the native symbols for the R2R images when publishing R2R, because profiler needs the symbols generated by <PublishReadyToRunEmitSymbols> to examine the R2R files. However, those symbols are only useful for profiling the startup scenario of PowerShell because PowerShell enables tiered compilation which will overwrite the R2R code.

PR Context

PR Checklist

@ghost ghost assigned iSazonov Oct 22, 2021
@daxian-dbw daxian-dbw marked this pull request as draft October 23, 2021 00:01
@daxian-dbw daxian-dbw marked this pull request as ready for review October 23, 2021 01:50
@iSazonov
Copy link
Collaborator

iSazonov commented Oct 23, 2021

However, those symbols are only useful for profiling the startup scenario of PowerShell because PowerShell enables tiered compilation which will overwrite the R2R code.

For profiling other scenarios users can set .Net environment variable DOTNET_TieredCompilation = 0

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 23, 2021

Could you please see search results https://github.com/PowerShell/PowerShell/search?l=PowerShell&q=Crossgen I see more files with crossgen word.

@iSazonov iSazonov added the Area-Maintainers-Build specific to affecting the build label Oct 23, 2021
@iSazonov iSazonov added CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log and removed Area-Maintainers-Build specific to affecting the build labels Oct 23, 2021
TravisEz13
TravisEz13 approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

Should we take this for 7.2?

@TravisEz13 TravisEz13 merged commit f2d5ae7 into PowerShell:master Oct 27, 2021
@adityapatwardhan
Copy link
Member

adityapatwardhan commented Oct 27, 2021

@daxian-dbw Can we do a quick startup perf check before and after the change?

@daxian-dbw daxian-dbw deleted the crossgen branch October 27, 2021 20:24
@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 27, 2021

OK, will do so.

@daxian-dbw
Copy link
Member Author

daxian-dbw commented Oct 30, 2021

I don't see a noticeable difference on both Windows and Linux with or without this change.

  • on my Windows machine, they are both in the range of 580 - 620 ms (sometimes slower than 620ms, and sometimes faster than 580ms)
  • on my WSL v2 with Ubuntu 18.04, they are both in the range of 660 - 700 ms (sometimes slower than 700ms, and sometimes faster than 660ms)

@ghost
Copy link

ghost commented Nov 8, 2021

🎉v7.2.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost
Copy link

ghost commented Dec 16, 2021

🎉v7.3.0-preview.1 has been released which incorporates this pull request.:tada:

Handy links:

TrapGodBrim pushed a commit to TrapGodBrim/PowerShell that referenced this pull request Jan 19, 2022
@daxian-dbw @TrapGodBrim
Clean up crossgen related build scripts also generate native symbol…
ee6093f 
…s for `R2R` images (PowerShell#16297)

* Clean up crossgen related build scripts

* Fix ci.psm1

* Clean up '-CrossGen' use in a few other files
daxian-dbw added a commit to daxian-dbw/PowerShell that referenced this pull request Mar 9, 2022
@daxian-dbw @adityapatwardhan
Clean up crossgen related build scripts also generate native symbol…
c0339d3 
…s for `R2R` images (PowerShell#16297)

* Clean up crossgen related build scripts

* Fix ci.psm1

* Clean up '-CrossGen' use in a few other files
@TravisEz13 TravisEz13 mentioned this pull request May 24, 2022
Merged
@TravisEz13 TravisEz13 mentioned this pull request Jun 7, 2022
Merged
22 tasks
@TravisEz13 TravisEz13 mentioned this pull request Apr 16, 2024
Closed
@TravisEz13 TravisEz13 mentioned this pull request Jul 9, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@TravisEz13 TravisEz13 TravisEz13 approved these changes

@iSazonov iSazonov iSazonov approved these changes

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan

@anmenaga anmenaga Awaiting requested review from anmenaga

Assignees

@iSazonov iSazonov

Labels

Backport-7.2.x-Done CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@daxian-dbw @iSazonov @adityapatwardhan @TravisEz13