Skip to content

[coreclr][android] Don't build/ship unused GC variants for Android #114448

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

Merged
merged 4 commits into from
Apr 16, 2025

Conversation

ivanpovazan
Copy link
Member

@ivanpovazan ivanpovazan commented Apr 9, 2025

Description

This PR disables building unused GC variants for Android on CoreCLR, reducing the size of built apps and the runtime package.

There are two parts to it, currently:

  • we ship both: libclrgc.so ~972 KB and libclrgcexp.so ~1025 KB as part of CoreCLR runtime pack for Android, which aren't used

  • we build server GC into runtime lib which is unlikely to be used on Android taking up ~300 KB in libcoreclr.so:

    .so size (MB) Before After diff
    libcoreclr.so 6,66 6,32 -0,34

Size savings

On a dotnet new android app this change reduces the size of the .apk by ~1MB

Copy link
Contributor

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

@ivanpovazan
Copy link
Member Author

@Maoni0 @janvorli @BrzVlad let me know what do you think about feasibility of the changes.

@janvorli
Copy link
Member

janvorli commented Apr 9, 2025

I think these changes make sense. On mobile devices, we would not want to use server GC and standalone GC, so it is just a useless added baggage.

@ivanpovazan ivanpovazan marked this pull request as ready for review April 10, 2025 12:47
@Copilot Copilot AI review requested due to automatic review settings April 10, 2025 12:47
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 4 changed files in this pull request and generated no comments.

Files not reviewed (2)
  • src/coreclr/CMakeLists.txt: Language not supported
  • src/coreclr/clrdefinitions.cmake: Language not supported

@steveisok steveisok self-requested a review April 10, 2025 12:56
Copy link
Member

@steveisok steveisok left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@Maoni0
Copy link
Member

Maoni0 commented Apr 15, 2025

(I'm back from vacation)

we ship both: libclrgc.so ~972 KB and libclrgcexp.so ~1025 KB as part of CoreCLR runtime pack for Android, which aren't used

I can see libclrgc.so used occasionally. this contains the old GC implementation that uses segments instead of regions. there were a couple of times when we did ask the customer to use libclrgc.so. it's only for problematic situations, same idea with shipping 2 bridge implementations.

a standalone GC is also very convenient for debugging, eg, if you want to give the customer a GC impl with some additional instrumentation to help track down an issue, a standalone GC just means they need to load it without having to change anything else. if we want this as a viable way for debugging, then at the minimum, we should make sure libcoreclr.so can load a standalone GC. however, I'm unclear what's possible/not possible for debugging on android so someone else who knows please comment.

as far as Server GC goes, again, I have no idea what sort of heap sizes we are seeing on android and if Server GC would be desirable. it's fine to not ship it for now though. just wanted to point out there might be a possibility later we'd want to ship it but we can enable it then.

@BrzVlad
Copy link
Member

BrzVlad commented Apr 15, 2025

Mono GC doesn't have great handling of large heap scenarios and I haven't heard it being a problem. We prioritize low pause times and my understanding is that Workstation GC would always be the desirable choice for this purpose.

My understanding is that we're going for static linking on mobile. Also, if the user would need to use some custom runtime binaries, the application would need to be rebuilt and redeployed anyway, so I don't see the standalone GC really of particular help here.

@steveisok
Copy link
Member

My understanding is that we're going for static linking on mobile. Also, if the user would need to use some custom runtime binaries, the application would need to be rebuilt and redeployed anyway, so I don't see the standalone GC really of particular help here.

+1 to what Vlad said. It's best to think the main mobile and wasm targets as exclusively self contained. It's easier in any debugging / support scenario to just rebuild / redeploy.

@jkotas
Copy link
Member

jkotas commented Apr 15, 2025

static linking

Native AOT allows static linking of different GCs. If we see a need to make the GC implementation configurable for mobile, we can use the same pattern. (It does not need to be done as part of this PR. You may want to track it in a list of potential future improvements.)

GC implementation that uses segments

We may want to check whether the segment GC is better fit for typical mobile app. The region GC reserves very large block of address space at startup that may have undesirable perf side-effects on mobile. (Against, it does not need to be done as part this PR.)

@Maoni0
Copy link
Member

Maoni0 commented Apr 15, 2025

if you have a large enough heap and allocates frequently, Server GC would be beneficial for pause times. but that doesn't need to be done with this PR - I just wanted to point it out as a potential future consideration.

makes sense to not care about the standalone GC for mobile. thanks for the info.

as far as which GC should be used, it's better to simply reserve less (if it's even a problem) for mobile instead of using the segments GC - we prefer to focus our efforts in fixing problems for regions instead of segments.

@ivanpovazan
Copy link
Member Author

Thank you all for your feedback.

Regarding:

Native AOT allows static linking of different GCs. If we see a need to make the GC implementation configurable for mobile, we can use the same pattern. (It does not need to be done as part of this PR. You may want to track it in a list of potential future improvements.)

I opened: #114737 for tracking, please feel free to include more information there (/cc: @vitek-karas @kotlarmilos)

Regarding:

We may want to check whether the segment GC is better fit for typical mobile app. The region GC reserves very large block of address space at startup that may have undesirable perf side-effects on mobile. (Against, it does not need to be done as part this PR.)

if you have a large enough heap and allocates frequently, Server GC would be beneficial for pause times. but that doesn't need to be done with this PR - I just wanted to point it out as a potential future consideration.

I opened: #114738 for tracking, please feel free to include more information there (/cc: @BrzVlad @Maoni0)

@ivanpovazan ivanpovazan merged commit 65cda8b into dotnet:main Apr 16, 2025
158 of 161 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators May 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants