Closed Bug 1262102 Opened 6 years ago Closed 5 years ago

Fix --with-intl-api on Android

Categories

(Firefox Build System :: General, defect)

Unspecified
Android
defect
Not set
normal

Tracking

(firefox48 affected, firefox50 fixed)

RESOLVED FIXED
mozilla50
Tracking Status
firefox48 --- affected
firefox50 --- fixed

People

(Reporter: ted, Assigned: m_kato)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Blocks: 1215247
(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.)
(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.
Chromium implementation loads assets/icudtl.dat via AssetManager...
(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?)
WIP.  For startup and memory usage, use szip and dynamically loading icu data file if needed
Assignee: nobody → m_kato
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.
(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.
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/
Attachment #8770476 - Flags: review?(nalexander)
Attachment #8770477 - Flags: review?(jmaher)
Attachment #8770478 - Flags: review?(jmaher)
Attachment #8770479 - Flags: review?(mh+mozilla)
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/
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/
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/
Attachment #8770476 - Flags: review?(nalexander) → review+
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.
Attachment #8770477 - Flags: review?(jmaher) → review?(gbrown)
Attachment #8770478 - Flags: review?(jmaher) → review?(gbrown)
reassigned to gbrown as he is more fluent with our current Android automation.
Comment on attachment 8770478 [details]
Bug 1262102 - Part 3. Extract ICU data file on unit tests.

https://reviewboard.mozilla.org/r/63910/#review61378
Attachment #8770478 - Flags: review?(gbrown) → review+
Comment on attachment 8770477 [details]
Bug 1262102 - Part 2. Set GRE_HOME to remote binary directory.

https://reviewboard.mozilla.org/r/63908/#review61382
Attachment #8770477 - Flags: review?(gbrown) → review+
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.
Do you have a bug number for that?
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.
Flags: needinfo?(mh+mozilla)
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 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/
Attachment #8770476 - Attachment description: Bug 1262102 - Part 1. Add compressed ICU data file to APK. → Bug 1262102 - Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android.
Attachment #8770476 - Flags: review+ → review?(ted)
Attachment #8770477 - Attachment is obsolete: true
Attachment #8770478 - Attachment is obsolete: true
Attachment #8770479 - Attachment is obsolete: true
Attachment #8770479 - Flags: review?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Comment on attachment 8770476 [details]
Bug 1262102 - Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android.

https://reviewboard.mozilla.org/r/63906/#review64862
Attachment #8770476 - Flags: review?(ted) → review+
Pushed by m_kato@ga2.so-net.ne.jp:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a467d989a3b9
Don't use MOZ_ICU_DATA_ARCHIVE=1 on Android. r=ted
https://hg.mozilla.org/mozilla-central/rev/a467d989a3b9
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.