1262102 - Fix --with-intl-api on Android

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

bug 1239083 makes it so ICU wants to load its data from a separate file. The patch adds code in xpcom/system/XPCOMInit.cpp to call `u_setDataDirectory`, which ICU uses to locate the file. Unfortunately this won't work on Android because the file would be in the APK, not on disk.

We could either read the data out of the APK and call `udata_setCommonData` to point ICU at the in-data memory blob, or we could just link libxul against the ICU-data-as-an-object that we are generating to link the JS shell with anyway.

Comment 1

[Jeff Walden [:Waldo]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #0)
> We could either read the data out of the APK and call `udata_setCommonData`
> to point ICU at the in-data memory blob, or we could just link libxul
> against the ICU-data-as-an-object that we are generating to link the JS
> shell with anyway.

Seems like, generally, u_setDataDirectory is a little limiting, because it necessarily requires going to disk and the file system to load this big file when needed.  It'd be better if it were part of the library/executable itself so that the OS would be responsible for making its paging-in fast and all.  Right?

(Note that I have no idea how nicely this plays with OS X's universal binaries, and whether the common data could be conveniently stored such that both architectures' code could use the same common data.)

Comment 2

[Mike Hommey [:glandium]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> (Note that I have no idea how nicely this plays with OS X's universal
> binaries, and whether the common data could be conveniently stored such that
> both architectures' code could use the same common data.)

universal "fat" binaries are stupid. You can't share data between both ends.

Comment 3

[Makoto Kato [:m_kato]]

Chromium implementation loads assets/icudtl.dat via AssetManager...

Comment 4

[(not currently active) Ted Mielczarek]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Seems like, generally, u_setDataDirectory is a little limiting, because it
> necessarily requires going to disk and the file system to load this big file
> when needed.  It'd be better if it were part of the library/executable
> itself so that the OS would be responsible for making its paging-in fast and
> all.  Right?

Sorta, but we were just linking the ICU data into libxul, which we preload on startup anyway because we learned that OS shared library loading patterns are suboptimal.

> (Note that I have no idea how nicely this plays with OS X's universal
> binaries, and whether the common data could be conveniently stored such that
> both architectures' code could use the same common data.)

This was one of the motivating factors to move to splitting the data out to a separate file (bug 926980). Unfortunately I don't think there's a reasonable way to do that on OS X.

One thing we could do, that I was thinking about due to the startup file I/O regression in bug 1262492, is to mmap the data file on startup, and pass that data pointer to ICU instead of just telling it to read the data file. That way presumably it'd only page the data in as-needed. Hopefully that should make fixing this straightforward as well, as we'd just need to map the data out of the APK and pass that to ICU. (I'm pretty sure we compress the APK, so I don't think we can directly mmap the file out, but we do have our fancy custom linker so maybe we could use that?)

Comment 5

[Makoto Kato [:m_kato]]

Attachment: WIP: Dynamically extract ICU data file from APK

WIP.  For startup and memory usage, use szip and dynamically loading icu data file if needed

Comment 6

[Axel Hecht [:Pike]]

Wondering, can we use an OBB? https://developer.android.com/google/play/expansion-files.html.

Might also help with getting permission to enable ICU in bug 1215247, as we'd rarely update the ICU data.

Comment 7

[Makoto Kato [:m_kato]]

(In reply to Axel Hecht [:Pike] from comment #6)
> Wondering, can we use an OBB?
> https://developer.android.com/google/play/expansion-files.html.
> 
> Might also help with getting permission to enable ICU in bug 1215247, as
> we'd rarely update the ICU data.

ICU uses on gfx and layout, too.  So this ICU data file will be required on every startup.  So OBB isn't good for our usages.

Comment 8

[Makoto Kato [:m_kato]]

Attachment: Bug 1262102 - Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android.

Add compressed icudt<version>l.dat to assert/<arch>/.

Review commit: https://reviewboard.mozilla.org/r/63906/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63906/

Comment 9

[Makoto Kato [:m_kato]]

Attachment: Bug 1262102 - Part 2. Set GRE_HOME to remote binary directory.

Now, GRE_HOME on remote xpcshell-test is /data/data/org.mozilla.fennec, not binary directory.  Since we don't copy binaries and libraries into /data/data/org.mozilla.fennec, GRE_HOME should be remote binary directory.

Review commit: https://reviewboard.mozilla.org/r/63908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63908/

Comment 10

[Makoto Kato [:m_kato]]

Attachment: Bug 1262102 - Part 3. Extract ICU data file on unit tests.

When runnig unit test such as xpcshell-test, it doesn't use APK file.  So ICU data file should be extracted on GRE path.

Review commit: https://reviewboard.mozilla.org/r/63910/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63910/

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Bug 1262102 - Part 4. Load ICU data from APK on demand.

Add new class to load compressed asset that is ICU data file.

But, when running on unit test such as xpcshell, it will run on local executable binary instead of Android app.  If no APK, use GRE path for ICU data directory.

Review commit: https://reviewboard.mozilla.org/r/63912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63912/

Comment 12

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8770476 [details]
Bug 1262102 - Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android.

https://reviewboard.mozilla.org/r/63906/#review61032

I'm fine with this.  (Pity about the `Makefile.in` guck.)

It's not clear that this will work for folks who build with Gradle; see https://dxr.mozilla.org/mozilla-central/source/mobile/android/app/build.gradle#252.  Could you either test yourself (with `mach gradle build` and inspecting/running the resulting APK), or have somebody on the Fennec team (mcomella, sebastian) test for you?  Don't want to break the front-end team's workflow if we can help it.

::: python/mozbuild/mozbuild/action/package_fennec_apk.py:67
(Diff revision 1)
>  
>      for assets_dir in assets_dirs:
>          finder = FileFinder(assets_dir, find_executables=False)
>          for p, f in finder.find('**'):
>              compress = None  # Take default from Jarrer.
> -            if p.endswith('.so'):
> +            if p.endswith('.so') or re.match(r".*/icudt\d+l\.dat$", p):

nit: single quotes for consistency.

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

reassigned to gbrown as he is more fluent with our current Android automation.

Comment 14

[Geoff Brown [:gbrown]]

Comment on attachment 8770478 [details]
Bug 1262102 - Part 3. Extract ICU data file on unit tests.

https://reviewboard.mozilla.org/r/63910/#review61378

Comment 15

[Geoff Brown [:gbrown]]

Comment on attachment 8770477 [details]
Bug 1262102 - Part 2. Set GRE_HOME to remote binary directory.

https://reviewboard.mozilla.org/r/63908/#review61382

Comment 16

[Mike Hommey [:glandium]]

I'll refrain from reviewing this for now, because we may end up not loading ICU data from a data file on other platforms, which would fix this bug as a side effect.

Comment 17

[Axel Hecht [:Pike]]

Do you have a bug number for that?

Comment 18

[Makoto Kato [:m_kato]]

If ICU data file isn't loaded correctly, JS_InitWithFailureDiagnostic returns error, then process will terminate by  NS_RUNTIMEABORT.

glandium, can we use old way (not define MOZ_ICU_DATA_ARCHIVE, and ICU data file is into libxul.so) for ICU on android build?  If OK, I will update fix.

Comment 19

[(not currently active) Ted Mielczarek]

Yes, you'd just need to link the `icudata` library instead of the `icustubdata` library:
https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/config/external/icu/moz.build#17

I think all you need to do is change the conditionals to set `MOZ_ICU_DATA_ARCHIVE` and things should already work (I wrote that so that standalone JS builds wouldn't have to deal with the data file):
https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/build/autoconf/icu.m4#84

njn floated this idea in bug 1262731 as well, because apparently something is causing the ICU data file to be unavailable on Windows.

Comment 20

[Makoto Kato [:m_kato]]

Comment on attachment 8770476 [details]
Bug 1262102 - Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/63906/diff/1-2/

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 8770476 [details]
Bug 1262102 - Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android.

https://reviewboard.mozilla.org/r/63906/#review64862

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a467d989a3b9
Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android. r=ted

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a467d989a3b9