Disable embedded package data for Android #53
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
finagolfin
reviewed
Mar 31, 2025
|
This is going to be a problem for getting consistent cross-platform results in the tests for Foundation. I'm not sure what the right answer is, yet. |
|
Oh - perhaps I misunderstood where we ended up here. We would still ship the data from this project - but as a file loaded at runtime instead of using one provided by the system? |
Yep, that's the idea. 😺 He simply tested against the system macOS data because it was the easiest available. |
|
Would it be possible to run some of the benchmarks from swift-foundation against this, to see if it makes a difference? We could do that on a non-Android platform as a proxy, if it's easier. |
Yes, that is the idea. A |
I set up a comparison workflow for this at https://github.com/swift-everywhere/swift-foundation-icu-demo/actions. You can see a result run both with and without externalizing the ICU data file at: https://github.com/swift-everywhere/swift-foundation-icu-demo/actions/runs/15025509227/job/42225156287. A summary of the mean times when running on macos 15 are as follows. External ICU data seems to average around 5% slower.
|
|
Thanks for that data @marcprux. @iCharlesHu - any feedback here? I know you thought a lot about how best to package this data when we set up the project. |
|
@iCharlesHu I'm also wondering where we can get the |
|
@iCharlesHu, now that 6.1.1 is about to be released, I'm thinking about switching my Android SDK bundle over to using this more conventional dat file loading instead. Marc tells me the header from this repo is generated from an Apple-modified ICU data file though: is there a public copy of that original dat file you used that I can download and ship with Android Foundation instead? It would be good if this pull checked that original ICU dat file back into this repo, for those of us who need to save space and would rather use this pull over the current shared library approach, which repeats the icudata build for every shipped architecture and takes up a lot more space on constrained mobile app deployments. |
|
It is derived from https://github.com/apple-oss-distributions/ICU. |
|
I would like to keep all of our platforms as consistent as possible. The more android (or any platform) specific behaviors we have, the more likely we are to introduce regressions that we don't detect before we ship. |
|
So the idea behind this pull is to use exactly the same icudata as in the header files that this repo currently builds into a shared library for each platform architecture, but simply ship the icudata as a single text file that is loaded at runtime instead. Are you worried that the icudata will differ or that something will go wrong in that different initialization process? I plan to get this repo set up in an Android CI that regularly runs all the Foundation tests in an Android emulator by this fall. I currently run the Foundation tests natively on my Android phone once a month, only 5-6 tests failing lately, last checked with the Apr. 12 trunk source snapshot. Obviously, the Foundation tests don't check all this ICU data, but it's an indicator. |
Yes, or something else we can't predict now. I'd just like to give @iCharlesHu a chance to chime in here and then we can make a decision about the best way to do this. |
I had seen that repo, but it doesn't contain the ICU makefiles errorsThe other option could be to craft a simple CLI that includes This could also be interesting because we could generate both a fat |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Embedded ICU data is problematic for Android apps:
aarch64-linux-android/lib_FoundationICU.so40M,arm-linux-androideabi/lib_FoundationICU.so37M,x86_64-linux-android/lib_FoundationICU.so39M)These problems result in extremely large Android apps when they depend on
FoundationInternationalization. E.g., see https://github.com/skiptools/skipapp-hiya/releases/tag/0.1.0As discussed at https://forums.swift.org/t/android-app-size-and-lib-foundationicu-so/78399, one possible solution would be to externalize the data and load the data from the file, which could then be shipped compressed as a single asset in the Android app. The app startup process would be responsible for extracting the data from the assets and writing it to a cached location, and then setting the
ICU_DATA_DIR_PREFIXto the data's root folder.This draft PR enables using an external data file on Android by default, and some testing against the macOS-provided
/usr/share/icu/icudt74l.datfile shows that it works the same as if the data is embedded.