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

Enable symbol stripping for crossgen2 #82698

Merged
merged 1 commit into from
Mar 2, 2023
Merged

Enable symbol stripping for crossgen2 #82698

merged 1 commit into from
Mar 2, 2023

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 27, 2023

crossgen2 in nuget package has a 62M binary on linux-arm64. The size of stripped binary drops to 16M.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 27, 2023
@MichalStrehovsky
Copy link
Member

Do we index the symbols somewhere?

@am11
Copy link
Member Author

am11 commented Feb 27, 2023

Do we index the symbols somewhere?

No.

On Windows, we abandon .pdb and create two packages; Microsoft.NETCore.App.Crossgen2.win-{arch}.nupkg and Microsoft.NETCore.App.Crossgen2.win-{arch}.symbols.nupkg, with same contents. This is due to a bug in AddCrossgen2SymbolFilesToPackage target; it runs before the target which computes the actual artifacts.. After that, we abandon .symbols.nupkg before pushing to dotnet-public and nuget.org feeds. I am not sure if we should fix the broken target or delete it.

Similarly, on Unix we create two packages with same content, both with fat binaries (with symbols). The current state of this PR brings Unix to same plan as Windows.

@MichalStrehovsky
Copy link
Member

I am not sure if we should fix the broken target or delete it.

Thanks! Adding @dotnet/crossgen-contrib for thoughts. It feels like the Windows behavior is a bug.

@am11 am11 marked this pull request as ready for review February 27, 2023 11:41
@am11 am11 force-pushed the patch-14 branch 2 times, most recently from 412a1e9 to a65a755 Compare February 27, 2023 18:32
@am11 am11 requested review from jkotas and agocke March 1, 2023 00:46
jkotas
jkotas reviewed Mar 1, 2023
View reviewed changes
src/installer/pkg/sfx/Microsoft.NETCore.App/Microsoft.NETCore.App.Crossgen2.sfxproj
@@ -22,6 +22,8 @@
<NativeAotSupported Condition="'$(CrossBuild)' == 'true' and '$(TargetOS)' != '$(HostOS)'">false</NativeAotSupported>
<!-- Can't use NativeAOT in source build yet https://github.com/dotnet/runtime/issues/66859 -->
<NativeAotSupported Condition="'$(DotNetBuildFromSource)' == 'true'">false</NativeAotSupported>
<ObjCopyName Condition="'$(ContinuousIntegrationBuild)' == 'true' and '$(CrossBuild)' == 'true' and '$(RuntimeIdentifier)' == 'linux-musl-arm64'">llvm-objcopy-15</ObjCopyName>
Copy link
Member

Choose a reason for hiding this comment

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

@sbomer has been working on updating build containers for .NET 8 (we cannot use CentOS7 anymore since it is going to be EOL soon).

The updated containers should have recent LLVM version that will make these workarounds unnecessary once we switch to them.

Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't use CentOS container. It uses Ubuntu 22.04 with Alpine Linux 3.13 rootfs.

Copy link
Member

Choose a reason for hiding this comment

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

We are updating all build containers, not just the ones that use CentOS 7.

Copy link
Member Author

@am11 am11 Mar 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is already at the latest Ubuntu 22.04. The binutils objcopy in host doesn't support cross-build architecture, which container update will not fix. This workaround is necessary because we don't find the toolchain specific objcopy tool in NativeAOT. This is not a problem with coreclr build, because we use cmake find mechanism to locate toolchain-specific objcopy and other tools.

jkotas
jkotas approved these changes Mar 1, 2023
View reviewed changes
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.

Looks good if it is something we want to fix quickly. A better fix will come with updated build containers and this workaround will have to be reverted.

@jkotas
Copy link
Member

jkotas commented Mar 1, 2023

@sbomer Is it ok with you to merge this change?

sbomer
sbomer approved these changes Mar 2, 2023
View reviewed changes
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Yes, LGTM for now. Thanks for the heads up!

@jkotas jkotas merged commit 6a218ab into dotnet:main Mar 2, 2023
am11 added a commit to am11/runtime that referenced this pull request Mar 2, 2023
@am11
Revert "Enable symbol stripping for crossgen2 (dotnet#82698)"
60c8369 
This reverts commit 6a218ab.
jkotas pushed a commit that referenced this pull request Mar 3, 2023
@am11
Revert "Enable symbol stripping for crossgen2 (#82698)" (#82881)
13853e5 
This reverts commit 6a218ab.
am11 added a commit to am11/runtime that referenced this pull request Mar 7, 2023
@am11
Revert "Revert "Enable symbol stripping for crossgen2 (dotnet#82698)" (…
31fbec1 
…dotnet#82881)"

This reverts commit 13853e5.
jkotas pushed a commit that referenced this pull request Mar 8, 2023
@am11
Rename cgroup.cpp to cgroupcpu.cpp (#83029)
0e1ede5 
* Revert "Revert "Enable symbol stripping for crossgen2 (#82698)" (#82881)"

This reverts commit 13853e5.

* Rename cgroup.cpp to cgroupcpu.cpp
@ghost ghost locked as resolved and limited conversation to collaborators Apr 1, 2023
@am11 am11 deleted the patch-14 branch June 14, 2024 04:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@trylek trylek trylek left review comments

@jkotas jkotas jkotas approved these changes

@sbomer sbomer sbomer approved these changes

@agocke agocke Awaiting requested review from agocke

Assignees
No one assigned
Labels
area-crossgen2-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@am11 @MichalStrehovsky @jkotas @sbomer @trylek