1402379 - Clean up the SpiderMonkey Intl build system

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement, P3)

Description

[Zibi Braniecki [:zbraniecki][:gandalf]]

We decided to not remove the ENABLE_INTL_API flag (bug 1402235)

At the same time Intl is going to be an active area over the next year or so
A lot of new APIs will get added in early stages for chrome only usage, and then exposed as they get to Stage 4, 

Because of that I'd like to reduce the amount of build system quirks that make hacking on SpiderMonkey hard for us.

Ideas:

 - Put Intl.cpp/Intl.h/Intl.js in `if ENABLE_INTL_API` and remove all non-ICU code from inside of it
 - Move Intl.* into its own directory and divide into API modules as possible (DateTimeFormat, NumberFormat, Collator, PluralRules etc.) to make it easier to add a new API starting with boilerplate code from an existing one
 - Make it easy to guard APIs in flags "chrome-only", "harmony-intl-pluralrules" etc. for runtime enabling
 - Further generalize the API boilerplate that repeats for every new API (constructor, initializer, resolvedOptions, format, formatToParts will repeat for Intl.UnitFormat, Intl.ListFormat, Intl.RelativeTimeFormat etc.)

This will make it easier for intl contributors to write patches for spidermonkey in the time when Intl module will stay very active.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

:till, :anba, :waldo - thoughts?

Comment 2

[Zibi Braniecki [:zbraniecki][:gandalf]]

(asking, because I'd like to pick up Intl.RelativeTimeFormat, Intl.ListFormat, Intl.Locale and a couple others that will land behind some flag first)

Comment 3

[Jeff Walden [:Waldo]]

Seems reasonable enough.

Comment 4

[André Bargull [:anba]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #0)
>  - Put Intl.cpp/Intl.h/Intl.js in `if ENABLE_INTL_API` and remove all
> non-ICU code from inside of it

Yes, please. Nowadays the stubs themselves are prone to bit rot.

>  - Move Intl.* into its own directory and divide into API modules as
> possible (DateTimeFormat, NumberFormat, Collator, PluralRules etc.) to make
> it easier to add a new API starting with boilerplate code from an existing
> one
>  - Make it easy to guard APIs in flags "chrome-only",
> "harmony-intl-pluralrules" etc. for runtime enabling

Sounds good.

>  - Further generalize the API boilerplate that repeats for every new API
> (constructor, initializer, resolvedOptions, format, formatToParts will
> repeat for Intl.UnitFormat, Intl.ListFormat, Intl.RelativeTimeFormat etc.)

As long as this doesn't make it harder to compare the implementation with specific spec steps.

Comment 5

[André Bargull [:anba]]

Attachment: bug1402379-remove-icu-stubs.patch

From http://logs.glob.uno/?c=mozilla%23jsapi&s=12+Oct+2017&e=12+Oct+2017#c864730
> 19:27	gandalf	Waldo: ok. I guess it's a tradeoff of productivity and maintainability. I'm happy to write code for our Intl API, but have to admit that maintaining the --with-icu=false part is a real PITA that takes disproportional amount of time for me :(


@gandalf:
Do you just need this patch to get rid of the ICU stubs or did I misinterpret your comment?

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

That's a huge part of the hassle! Yes! Thank you!

My gut feeling is that there are a few more items like generlizing "IntlFormatter" codebase that is repetitive between all formatters - DateTimeFormat/NumberFormat/UnitFormat/RelativeTimeFormat/ListFormat - and maybe split that into file per API, but those are definitely less problematic than having to handle the stubs and test my patches against no-intl builds.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 8918259 [details] [diff] [review]
bug1402379-remove-icu-stubs.patch

Review of attachment 8918259 [details] [diff] [review]:
-----------------------------------------------------------------

Mm.  As long as we're not actually going to really test the no-Intl case, I suppose it doesn't make much difference whether the #ifdefs are narrowly scoped or much more broadly defined.  So this is fine by me.

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

One other option is to do what Node and V8 are doing - always do Intl, but only package en-US when `--with-intl-api=false`.

I was at Unicode Conference today where IBM and Microsoft were talking about how they can improve the build system to make it easier to select data you want to bundle, and they asked me why we remove the API, instead of just the data.

:till, :anba, :waldo - does it sound like something we might want to do?

Comment 9

[André Bargull [:anba]]

I thought bug 1402235 was closed because it is difficult to build ICU in certain configurations resp. because of the increased code size when ICU is included. And even if we only bundle en-US, we'll only decrease the data size of the icudtXXl.dat file, but won't reduce the code size. Or do I misunderstand something here?

Can we move this bug to "JavaScript: Internationalization API", so it's easier to track for SpiderMonkey people?

Comment 10

[Zibi Braniecki [:zbraniecki][:gandalf]]

> And even if we only bundle en-US, we'll only decrease the data size of the icudtXXl.dat file, but won't reduce the code size. Or do I misunderstand something here?

My understanding was that the data size is all that matters. I don't have numbers but the way people talked about ICU payload at Unicode Conference made it seems as if the code size is barely noticable, and all that matters is how much data we carry (both, what tables and what locales).

So my thinking was that if we could replace "ENABLE_INTL_API=no" with "WITHOUT_INTL_DATA=yes" and only ship en-US data, we'd shave almost all of the cost, but keep the code able to run in all configurations.

Makoto, Jonathan - do you know how much an en-US only ICU costs us vs. "per-locale" size?

Comment 11

[Makoto Kato [:m_kato]]

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> > And even if we only bundle en-US, we'll only decrease the data size of the icudtXXl.dat file, but won't reduce the code size. Or do I misunderstand something here?
> 
> My understanding was that the data size is all that matters. I don't have
> numbers but the way people talked about ICU payload at Unicode Conference
> made it seems as if the code size is barely noticable, and all that matters
> is how much data we carry (both, what tables and what locales).
> 
> So my thinking was that if we could replace "ENABLE_INTL_API=no" with
> "WITHOUT_INTL_DATA=yes" and only ship en-US data, we'd shave almost all of
> the cost, but keep the code able to run in all configurations.
> 
> Makoto, Jonathan - do you know how much an en-US only ICU costs us vs.
> "per-locale" size?

When I tested this for Android last year, en-US (root + en) only dat was around 3-4MB.  Additional, icu code size was also big...  Although Android case was that it used szip or lzma compression, it required 1.6MB+ for compressed data and compressed code with en only.

Comment 12

[Jonathan Kew [:jfkthame]]

I don't have any more detail to add to that. It's true that the data size (for a full set of locales) greatly outweighs the code size, so stripping it down to en-US only would reduce it substantially; but whether the "code size is barely noticeable" depends on one's expectations. Do we know how big a standalone spidermonkey build (without intl api) currently is? What percentage size increase there would be considered acceptable?

Comment 13

[Zibi Braniecki [:zbraniecki][:gandalf]]

Steven, can you share your experience with slicing ICU/CLDR? How much overhead would you expect to have coming from data vs. code? How much payload is the static cost of just includign any ICU/CLDR and how much is per-locale?
What would you expect the cost to be for ICU/CLDR with en-US vs. ICU/CLDR with 80 locales?

Comment 14

[Steven R. Loomis]

off the top of my head for node considering data, but the full data file is about 25MiB (now about 26). Using 'English only' by default gets us to about 1+MiB (now about 2.7MiB - need to see what crept in to increase the size). English only means root + English. 

See:  https://ssl.icu-project.org/trac/ticket/10922 

About Node's build system, see comment at : https://ssl.icu-project.org/trac/ticket/10919#comment:5


also node.js builds statically. so, anything not used is dead stripped.  I have an exclusion list that excludes as many source files as I possibly can.  (see 2nd link)

Comment 15

[André Bargull [:anba]]

Attachment: Bug 1402379 - Part 1: Remove unused and unmaintainted jshint directives. r=jwalden!

Comment 16

[André Bargull [:anba]]

Attachment: Bug 1402379 - Part 2: Don't build Intl sources when ENABLE_INTL_API is not set. r=jwalden!

Depends on D41767

Comment 17

[André Bargull [:anba]]

Attachment: Bug 1402379 - Part 3: Remove ICUStubs.h file. r=jwalden!

Depends on D41769

Comment 18

[André Bargull [:anba]]

Attachment: Bug 1402379 - Part 4: Remove no longer needed #ifdef in Intl code. r=jwalden!

Depends on D41770

Comment 19

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=43a5b57a8ce958951bf8e6b9d63823c1c07512b2

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df0970454a3a
Part 1: Remove unused and unmaintainted jshint directives. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/daed59dc376d
Part 2: Don't build Intl sources when ENABLE_INTL_API is not set. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/f10098386f9e
Part 3: Remove ICUStubs.h file. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/43ecbbc83d6c
Part 4: Remove no longer needed #ifdef in Intl code. r=jwalden

Comment 21

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/df0970454a3a
https://hg.mozilla.org/mozilla-central/rev/daed59dc376d
https://hg.mozilla.org/mozilla-central/rev/f10098386f9e
https://hg.mozilla.org/mozilla-central/rev/43ecbbc83d6c