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.

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

Turn on ReadyToRun #12361

Merged
merged 7 commits into from Apr 22, 2020
Merged

Turn on ReadyToRun #12361

merged 7 commits into from Apr 22, 2020

Conversation

iSazonov
Copy link
Collaborator

@iSazonov iSazonov commented Apr 17, 2020 •
edited

PR Summary

Turn on ReadyToRun for PowerShell dll-s.

We can use dotnet/runtime#35062 (comment) to check dll-s.

PR Context

Now all .Net dll-s is in ReadyToRun format and PowerShell distributive doubled in size. So we can turn on the feature too. It will add ~20Mb (10%).

Docs https://docs.microsoft.com/en-us/dotnet/core/whats-new/dotnet-core-3-0#readytorun-images

PR Checklist

@iSazonov iSazonov added the CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log label Apr 17, 2020
@iSazonov iSazonov added this to the 7.1.0-preview.2 milestone Apr 17, 2020
@iSazonov iSazonov requested a review from anmenaga as a code owner April 17, 2020 17:04
@TravisEz13
Copy link
Member

We should disable the crossgen, but only for our binaries... let me see if I can do this change...

@TravisEz13
Copy link
Member

I need to do some testing.

@TravisEz13
Copy link
Member

@daxian-dbw FYI

iSazonov
iSazonov commented Apr 18, 2020
View reviewed changes
build.psm1
}

$fullAssemblyList = $commonAssembliesForAddType + $psCoreAssemblyList
$fullAssemblyList = $commonAssembliesForAddType
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems we should disable Start-CrossGen function.

Copy link
Member

Choose a reason for hiding this comment

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

We should probably disable Start-CrossGen until we can verify PublishReadyToRun is a good replacement

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

CrossGen and ReadyToRun are the same thing, well, it's a switch to crossgen.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can use set COMPlus_ReadyToRunLogFile=log.txt from issue I referenced above to check dll-s. Start-CrossGen does not create R2R dll-s.

Copy link
Member

Choose a reason for hiding this comment

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

@iSazonov crossgen with a switch is what is used to generate readytorun binaries in the dotnet project

Copy link
Member

Choose a reason for hiding this comment

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

And I switched us to that method, but per DotNet we need to use the csproj if we want our symbols to work: https://github.com/PowerShell/PowerShell/blob/master/build.psm1#L2217

Copy link
Member

Choose a reason for hiding this comment

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

But can your explain why crossgen should be disabled on the rest of the binaries?

Copy link
Member

Choose a reason for hiding this comment

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

I forgot that we also crossgen assemblies not part of the build that we include in the package. We should probably still crossgen those.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This make sense for third-party assemblies. For PSReadline and MI we could move to R2R format.

@iSazonov crossgen with a switch is what is used to generate readytorun binaries in the dotnet project

With set COMPlus_ReadyToRunLogFile=log.txt logs show that after Start-CrossGen our dll-s are not initialized as R2R but with PublishReadyToRun are initialized.

TravisEz13
TravisEz13 requested changes Apr 20, 2020
View reviewed changes
build.psm1 Outdated
Write-Log "Run ResGen (generating C# bindings for resx files)"
Start-ResGen
}
# TODO: remove Start-ResGen after testing ReadyToRun
Copy link
Member

Choose a reason for hiding this comment

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

Please revert this change. This is the wrong PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oops, sorry.

@ghost ghost added Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Apr 20, 2020
@daxian-dbw
Copy link
Member

Revert "Disable Start-ResGen"
Disable Start-CrossGen

@iSazonov You reverted a commit and then applied the same commit again.

TravisEz13
TravisEz13 reviewed Apr 22, 2020
View reviewed changes
build.psm1 Outdated
@@ -2233,6 +2233,9 @@ function Start-CrossGen {
$CrossgenPath
)

# TODO: remove Start-CrossGen after testing ReadyToRun.
return
Copy link
Member

Choose a reason for hiding this comment

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

This isn't true, and we should still crossgen the un-optimized binaries.

TravisEz13
TravisEz13 requested changes Apr 22, 2020
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.

Please address changes. I made ALL the needed changes the the crossgen function. Please revert back to my commit.

@ghost ghost added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 22, 2020
TravisEz13
TravisEz13 reviewed Apr 22, 2020
View reviewed changes
build.psm1 Outdated Show resolved Hide resolved
@TravisEz13
Copy link
Member

@PoshChan Please remind me in 1 hour

@TravisEz13
Copy link
Member

If we need to disable crossgen on the other assemblies, please submit a separate PR. Combining items in PRs makes it difficult if we need to backport.

@TravisEz13 TravisEz13 removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Apr 22, 2020
@PoshChan
Copy link
Collaborator

@TravisEz13, this is the reminder you requested 1 hour ago

@TravisEz13 TravisEz13 changed the title Turn on ReadyToRun Turn on ReadyToRun Apr 22, 2020
@TravisEz13 TravisEz13 merged commit 5cd89a4 into PowerShell:master Apr 22, 2020
@iSazonov iSazonov deleted the turn-on-readytorun branch April 23, 2020 03:51
@iSazonov
Copy link
Collaborator Author

If we need to disable crossgen on the other assemblies, please submit a separate PR. Combining items in PRs makes it difficult if we need to backport.

I have only question about PSRL and MI. We could make it readytorun in their repos. If yes I can open tracking issues. Thoughts?

@adityapatwardhan adityapatwardhan mentioned this pull request Apr 23, 2020
Merged
14 tasks
@TravisEz13
Copy link
Member

I think it's bad practices in publish the readytorun in the nuget. But I would open a new issue, and just be sure to ask that question.

@TravisEz13
Copy link
Member

TravisEz13 commented Apr 24, 2020 •
edited by unfurl-links bot

"At present, libraries can only be R2R compiled as part of an application, not for delivery as a NuGet package."
https://devblogs.microsoft.com/dotnet/announcing-net-core-3-0-preview-6/

.NET Blog
Today, we are announcing .NET Core 3.0 Preview 6. It includes updates for compiling assemblies for improved startup, optimizing applications for size with linker and EventPipe improvements. We’ve also released new Docker images for Alpine on ARM64. Download .NET Core 3.0 Preview 6 right now on Windows,

@iSazonov
Copy link
Collaborator Author

iSazonov commented Apr 25, 2020 •
edited

In our publish folder we get .Net dll-s in R2R format and trace shows this. If we compiled them locally into this format, then PSRL and MI would also be converted in it. Yes? It makes me think that we get them already in R2R format and I'd expect we could get PSRL and MI in R2R format too.
I can ask in .Net repo but I do still not understand that ask. :-)

@ghost
Copy link

ghost commented May 19, 2020

🎉v7.1.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

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

@daxian-dbw daxian-dbw Awaiting requested review from daxian-dbw daxian-dbw is a code owner

@SteveL-MSFT SteveL-MSFT Awaiting requested review from SteveL-MSFT

@adityapatwardhan adityapatwardhan Awaiting requested review from adityapatwardhan adityapatwardhan is a code owner

@anmenaga anmenaga Awaiting requested review from anmenaga anmenaga is a code owner

Assignees

@TravisEz13 TravisEz13

Labels
CL-BuildPackaging Indicates that a PR should be marked as a build or packaging change in the Change Log
Projects
None yet
Milestone
7.0.x-Servicing-Consider
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@iSazonov @TravisEz13 @daxian-dbw @PoshChan @SteveL-MSFT @adityapatwardhan