Last Comment Bug 853301 - Enable ECMAScript Internationalization API for desktop Firefox
: Enable ECMAScript Internationalization API for desktop Firefox
Status: RESOLVED FIXED
[DocArea=JS][qa-]
: dev-doc-complete, feature
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- enhancement with 3 votes (vote)
: mozilla27
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
: 890763 (view as bug list)
Depends on: 724531 724533 769871 769872 837957 845190 857450 864519 869659 890127 890238 899635 899948 900286 919872 924839 926980 928017 958108
Blocks: es-intl 866344 992651 899722 899890
  Show dependency treegraph
 
Reported: 2013-03-20 21:11 PDT by Norbert Lindenberg
Modified: 2016-04-07 05:47 PDT (History)
47 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled
disabled
fixed
fixed
29+


Attachments
Enable ECMAScript Internationalization API for desktop Firefox (1.44 KB, patch)
2013-03-26 22:04 PDT, Norbert Lindenberg
gps: review+
brendan: review+
mh+mozilla: review-
asa: superreview+
Details | Diff | Splinter Review
Make the Intl API opt-out, rather than opt-in (1.66 KB, patch)
2013-07-01 17:02 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
Enable Intl in desktop builds (1.12 KB, patch)
2013-07-03 18:19 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Implement --enable-intl-api=disable|build|implement (default implement) (10.86 KB, patch)
2013-08-06 15:14 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
till: review+
mh+mozilla: feedback+
Details | Diff | Splinter Review
Convert to a MOZ_ARG_WITH_STRING (11.00 KB, patch)
2013-08-08 20:52 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
mh+mozilla: review+
Details | Diff | Splinter Review
Mac OS X 10.9 build error log (262.56 KB, text/plain)
2013-08-15 11:54 PDT, Guillaume Abadie
no flags Details
callgrind of Intl-enabled shell doing |./js -e Intl| (1.35 MB, application/octet-stream)
2013-09-18 14:59 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details
Annotated callgrind of Intl-enabled debug shell doing |js -e ''| (30.97 KB, text/plain)
2013-09-18 16:05 PDT, Till Schneidereit [:till]
no flags Details
Annotated callgrind of Intl-enabled debug shell doing |js -e ''| (134.43 KB, text/plain)
2013-09-18 17:19 PDT, Till Schneidereit [:till]
no flags Details
Make available-locale computation lazier, draft patch (5.29 KB, patch)
2013-09-18 17:40 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
no flags Details | Diff | Splinter Review
Annotated callgrind of Intl-enabled lazily-parsing-self-hosted-code shell doing |js -e ''| (152.95 KB, text/plain)
2013-09-19 07:31 PDT, Till Schneidereit [:till]
no flags Details
Annotated callgrind of Intl-enabled lazily-parsing-self-hosted-code-lazy-available-locale shell doing |js -e ''| (154.26 KB, text/plain)
2013-09-19 07:40 PDT, Till Schneidereit [:till]
no flags Details
Make available-locale computation lazier (5.50 KB, patch)
2013-09-20 13:46 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
till: review+
Details | Diff | Splinter Review
Adjust test_interfaces.html to account for Intl on desktop (2.81 KB, patch)
2013-10-03 06:27 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
bugs: review+
Details | Diff | Splinter Review
Turn on the Intl API in desktop builds (1.19 KB, patch)
2013-10-03 06:28 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
ted: review+
Details | Diff | Splinter Review

Description Norbert Lindenberg 2013-03-20 21:11:31 PDT
Enable the implementation of the ECMAScript Internationalization API, which has so far been landed disabled, for desktop Firefox.
Comment 1 Norbert Lindenberg 2013-03-26 22:04:46 PDT
Created attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

A small patch to turn on 6000 lines of already landed SpiderMonkey code and uncountable lines of ICU code and CLDR data. Depends on the two patches under review in bug 724533.

Try build at:
https://tbpl.mozilla.org/?tree=Try&rev=af3dddfe0f62
Comment 2 Norbert Lindenberg 2013-03-26 22:06:05 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

Product management doesn't seem to have a dedicated review flag, so I'm using superreview to request permission to enable the ECMAScript Internationalization API for desktop Firefox.

There has been quite some discussion of the impact of this API and the underlying ICU library especially on download size. Current try builds indicate that this change will increase download sizes as follows:

Platform                          Intl     no Intl     Δ        Δ%
Linux, 32 bit (tar.bz2):         31.0 MB   28.0 MB   3.0 MB   10.8 %
Linux, 64 bit (tar.bz2):         34.2 MB   31.2 MB   3.0 MB    9.7 %
MacOS X, 32+64 bit (dmg):        60.7 MB   54.7 MB   5.9 MB   10.8 %
Windows, 32 bit (installer.exe): 22.4 MB   20.5 MB   1.9 MB    9.2 %

There are a number of known issues in the current implementation, all listed as blockers of bug 837963. If product management believes that any of them must be fixed before the API can be enabled, please add them as blockers of this bug. We're currently investigating bug 837941 and bug 822080 (blocker of bug 853704) with the goal of getting them fixed before the Firefox 22 train leaves.
Comment 3 Asa Dotzler [:asa] 2013-03-27 18:30:31 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

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

This "superreview" is to document Product's acceptance of this feature.
Comment 4 Norbert Lindenberg 2013-03-29 13:49:48 PDT
Approved by Brendan via IRC #jsapi 2013-03-29.
Comment 5 Gregory Szorc [:gps] 2013-03-29 16:53:35 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

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

The build system review is being performed in bug 724533. I consider r+ of those patches sufficient sign-off from the Build Config Module that ICU can be enabled whenever someone sees fit.

That someone is not a build peer but likely a super reviewer or someone else familiar with all the technical implications and risks attached to enabling ICU.
Comment 6 Gregory Szorc [:gps] 2013-03-29 16:56:22 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

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

Opps - forgot to steal review. rs=gps.

I insist on super review from a technically minded person being attained before this lands.
Comment 7 Brendan Eich [:brendan] 2013-03-29 20:02:02 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

Taras wrote "f+. It's being folded into an existing library, there shouldn't be a measurable startup hit(due to readahead). Dmandelin ran some tests to confirm. I'm not aware of any other perf concerns."

sr=me.

/be
Comment 8 Ryan VanderMeulen [:RyanVM] 2013-04-13 09:28:32 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

https://hg.mozilla.org/integration/mozilla-inbound/rev/390be7c3dd3a
Comment 9 Phil Ringnalda (:philor) 2013-04-13 13:39:21 PDT
Comment on attachment 729984 [details] [diff] [review]
Enable ECMAScript Internationalization API for desktop Firefox

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a18b89e5b3e6 - not only was bug 845190 blocking landing since these patches made it permaorange on Windows, but also the Windows b2g desktop build became permared with the unclear failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=21778030&tree=Mozilla-Inbound
Comment 10 Phil Ringnalda (:philor) 2013-04-13 15:11:14 PDT
Looks like the b2g Windows desktop bustage was something else unrelated.
Comment 11 Jeff Walden [:Waldo] (remove +bmo to email) 2013-05-06 21:17:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e40466e428dc
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-05-07 19:38:41 PDT
https://hg.mozilla.org/mozilla-central/rev/e40466e428dc
Comment 13 Mike Hommey [:glandium] 2013-05-08 00:46:50 PDT
This prevents the switch to 64 bits builders for 32-bits linux, which is increasingly important to do because we may hit the 4GB memory wall for linking soon (people are hitting it locally already), because of bug 869659 comment 2.

I am tempted to say ICU should be disabled until bug 869659 is addressed.
Comment 14 Ryan VanderMeulen [:RyanVM] 2013-05-08 05:27:58 PDT
Backed out.
https://hg.mozilla.org/integration/mozilla-inbound/rev/97e73b69c279
Comment 15 Norbert Lindenberg 2013-05-08 05:53:45 PDT
Is there a bug for the switch proposed in comment 13? It seems there should be, and any work needed to enable that switch should be filed as blockers for that bug. I don't quite see how the desire or need for such a switch justifies undoing work already done.
Comment 16 Mike Hommey [:glandium] 2013-05-08 05:59:35 PDT
(In reply to Norbert Lindenberg from comment #15)
> Is there a bug for the switch proposed in comment 13?

bug 857697. All the work for that bug was done before this landed, and this broke it.
Comment 17 Mark Banner (:standard8) 2013-05-08 07:51:56 PDT
The patch as-is actually enables ECMAScript for xulrunner and b2g builds, which is contrary to the title of this bug. Is it actually intended? (I'm happy either way, but would like clarity).

If it is intended, why not just change --enable-intl-api to --disable-intl-api in configure, and default it on there?

That way all builds will use it - including developer builds which often don't use the common mozconfigs.
Comment 18 Mark Banner (:standard8) 2013-05-08 07:55:46 PDT
(In reply to Mark Banner (:standard8) from comment #17)
> The patch as-is actually enables ECMAScript for xulrunner and b2g builds,
> which is contrary to the title of this bug. Is it actually intended? (I'm
> happy either way, but would like clarity).

I was slightly wrong about the b2g builds. The patch as-is enables ECMAScript intl for Windows b2g, but not any other b2g platforms... which seems like a potential miss-match for developer use.
Comment 19 Mike Hommey [:glandium] 2013-05-08 08:15:49 PDT
(In reply to Mark Banner (:standard8) from comment #17)
> If it is intended, why not just change --enable-intl-api to
> --disable-intl-api in configure, and default it on there?

Indeed, that's how it should be done.
Comment 20 Boris Zbarsky [:bz] 2013-05-08 14:32:47 PDT
The backout for this seems to have reduced trace-malloc leaks by double-digit percentages, just like the landing increased them by that amount.  Is there a bug tracking that issue?
Comment 21 Jeff Walden [:Waldo] (remove +bmo to email) 2013-05-08 14:45:49 PDT
Wasn't aware it was an issue.  I'll look into that too, I guess.
Comment 22 Boris Zbarsky [:bz] 2013-05-08 14:49:27 PDT
There was also a small (2-3%) hit on tracemalloc allocs, but that's not entirely unexpected for something like this that pulls in a big library.  :(
Comment 23 Matt Brubeck (:mbrubeck) 2013-05-08 15:21:38 PDT
Additionally there was a 2-3% regression in Ts Paint, at least on Windows and Linux.  Maybe related to the code size increase (comment 2)?
Comment 24 Norbert Lindenberg 2013-05-08 20:54:14 PDT
(In reply to Mark Banner (:standard8) from comment #18)
> (In reply to Mark Banner (:standard8) from comment #17)
> > The patch as-is actually enables ECMAScript for xulrunner and b2g builds,
> > which is contrary to the title of this bug. Is it actually intended? (I'm
> > happy either way, but would like clarity).
> 
> I was slightly wrong about the b2g builds. The patch as-is enables
> ECMAScript intl for Windows b2g, but not any other b2g platforms... which
> seems like a potential miss-match for developer use.

To intent was to not enable for Firefox OS yet - there's a separate bug 866301 for that. It seems the divergent results are caused by b2g/config/mozconfigs/nightly, which includes an OS mozconfig, while the Linux and Mac OS peers don't do that. Any idea why they operate differently?

Enabling the API for XULRunner wasn't carefully considered, but given that it's supposed to be "as rich as Firefox", is probably a good thing.
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-05-08 21:23:15 PDT
https://hg.mozilla.org/mozilla-central/rev/97e73b69c279
Comment 26 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-01 17:02:28 PDT
Created attachment 769947 [details] [diff] [review]
Make the Intl API opt-out, rather than opt-in

This follows the previously suggested tracks of making lack of anything mean enabling Intl, and the presence of --disable-intl-api meaning turn it off.  It currently has a universal turnoff in $topsrcdir/configure.in so that desktop builds are unaffected.  As the cross-compile situation in bug 869659 gets sorted out, we can tweak the condition to pass along --disable-intl-api in fewer and fewer situations.

https://tbpl.mozilla.org/?tree=Try&rev=d1a3fb975f42

I find myself wondering why our configuration macros don't make both --enable-* and --disable-* work, and maybe --enable-*=[yes/no] as well.  But that's clearly a separate issue.  :-)
Comment 27 Mike Hommey [:glandium] 2013-07-01 17:45:45 PDT
Comment on attachment 769947 [details] [diff] [review]
Make the Intl API opt-out, rather than opt-in

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

::: configure.in
@@ +9474,4 @@
>  dist=$MOZ_BUILD_ROOT/dist
>  ac_configure_args="$_SUBDIR_CONFIG_ARGS"
>  ac_configure_args="$ac_configure_args --enable-threadsafe"
> +if test "A" == "A"; then # "$MOZ_BUILDAPP" != "browser"

== is a bashism. Use =.
Please add a comment that this is a placeholder test for when there will be one.
Comment 28 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-02 09:39:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdfb39891a7
Comment 29 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-03 18:19:33 PDT
Created attachment 771123 [details] [diff] [review]
Enable Intl in desktop builds

https://tbpl.mozilla.org/?tree=Try&rev=532f0b692d91

This patch appears to enable Intl in desktop builds just fine, now that the 64/32 cross-compile situation has been ironed out.  Which, now that the last few patches in bug 496923 have landed today, means that we're down to (as far as I can tell) just the regression concerns from comment 20, comment 22, and comment 23.  I'm going to try spot-running the Intl tests under valgrind and see what sort of memory issues, if any, it reports.  Hopefully that can resolve some of those issues, or at least illuminate them.

njn also wants to hook the ICU stuff up to use custom memory allocation functions, a la u_setMemoryFunctions.

http://www.icu-project.org/apiref/icu4c/uclean_8h.html#a5dea766c5e726833400dc0ee24a2bc6a

This would at least let us get ICU allocations into a separate bucket as far as about:memory goes.  My skimming of ICU docs suggests there's no way to attribute memory use any more precisely than that, along the lines of the sizeOfExcludingThis() intrusive reporting interfaces about:memory would prefer.  If we *really* care, at some point we could implement something like that ourselves in ICU (we'd definitely have to do that upstream -- no way are we taking wide-scale intrusive changes like that even as post-update-applied patches), but I suspect we don't care *quite* that much.

There are also a few small tweaks that can be made to the self-hosting Intl code, to not have to clone self-hosting data into every compartment that needs Intl functionality (even indirectly via toLocaleString calls that don't explicitly implicate Intl itself).  I'm not sure how much that would affect memory -- I suspect it'd be mostly in the noise overall.  But it's probably worth doing to not drop an easy win on the floor.  No bug on file yet, but the patching should be simple enough that I can file after I write some patches.
Comment 30 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-03 18:27:05 PDT
Looking slightly more at ICU docs and what's available, it's a fair guess that we should be calling u_init(UErrorCode*) and u_cleanup() to ensure the memory-reporting stuff that tinderbox is likely looking at is sane.  :-)  I'll investigate that and file another bug for it.
Comment 31 Norbert Lindenberg 2013-07-15 22:16:27 PDT
*** Bug 890763 has been marked as a duplicate of this bug. ***
Comment 32 Matt Brubeck (:mbrubeck) 2013-07-30 10:06:06 PDT
Similar to the previous landing, this regressed Ts by ~25ms, tpaint by about 10ms, and increased various memory counters.  Possible clue: "Tp5 No Network Row Major MozAfterPaint (Main Startup File IO Bytes)" regressed by 34% (about 10MB).

https://groups.google.com/d/topic/mozilla.dev.tree-management/o0klAW2kxj8/discussion

If this patch stays in, we'll need follow-up bugs to fix the regressions.  They should be nominated for tracking-firefox25.
Comment 33 Ryan VanderMeulen [:RyanVM] 2013-07-30 10:24:56 PDT
https://hg.mozilla.org/mozilla-central/rev/a297a9937018
Comment 34 Jean-Yves Perrier [:teoli] 2013-07-30 11:10:31 PDT
Is the Target Milestone "Firefox 23" correct? Shouldn't it be Firefox 25
Comment 35 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-07-31 15:03:21 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #32)
> Similar to the previous landing, this regressed Ts by ~25ms, tpaint by about
> 10ms, and increased various memory counters.  Possible clue: "Tp5 No Network
> Row Major MozAfterPaint (Main Startup File IO Bytes)" regressed by 34%
> (about 10MB).

What's the plan to address this? This needs to come out if we don't have a reasonable expectation of fixing this in the near term.
Comment 36 Till Schneidereit [:till] 2013-07-31 15:15:17 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #35)
> What's the plan to address this? This needs to come out if we don't have a
> reasonable expectation of fixing this in the near term.

I haven't measured yet, but at least Ts could be caused by compiling the self-hosted code for Intl. We have plans for doing that lazily, but didn't yet work on it.

I'll do some measurements and report back.
Comment 37 Jeff Walden [:Waldo] (remove +bmo to email) 2013-07-31 15:16:35 PDT
The plan is to investigate and fix the marionette intermittent failure, then circle back and investigate these concerns.  I haven't filed bugs for them yet; anyone who cares should feel free to do so, assign to me.

"Backout" is a one-liner, easily done.  I'm fine doing that Monday if it comes to it, but I think we should run with this a little longer before throwing in the towel.
Comment 38 Justin Dolske [:Dolske] 2013-08-01 10:42:39 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #37)

> "Backout" is a one-liner, easily done.  I'm fine doing that Monday if it
> comes to it, but I think we should run with this a little longer before
> throwing in the towel.

Two points:

1) This has become a pain for Australis, because we're doing lots of investigation for our own perf regression, and comparing the combinations of m-c vs UX vs with-ICU-regression vs without-ICU-regression is a mess. Our tpaint regression is close to test noise, and so being able to look at trends is especially important.

2) Monday is the FF26 merge day, so this should be out before then to avoid getting in relmgmt's hair.

Especially if a backout is a one-liner, I'd suggest just doing it now.
Comment 39 bhavana bajaj [:bajaj] 2013-08-02 13:18:08 PDT
[release-notes] Add as a separate note in addition to the other ES6 note for this feature
Comment 40 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-04 16:54:33 PDT
I'll back this out later tonight after dinner.
Comment 42 Gregory Szorc [:gps] 2013-08-04 23:43:33 PDT
Can you not use NIGHTLY_BUILD (https://wiki.mozilla.org/Platform/Channel-specific_build_defines) for enabling/disabling this feature (ideally via <app>/confvars.sh instead of modifying configure.in)?
Comment 43 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-05 19:09:09 PDT
We could.  That wouldn't address the Australis concerns in comment 38, tho, so it seems probably not the right way to go the next time around here.

I'd like to get some sort of vague comparison of baseline numbers (nothing enabled) against numbers with ICU and everything compiled, but with Intl specifically *not* added to globals.  That will disentangle differences purely due to the library size increase, from differences deriving from the Intl code actually doing anything.  More on that tomorrow.
Comment 44 Ryan VanderMeulen [:RyanVM] 2013-08-05 20:32:22 PDT
https://hg.mozilla.org/mozilla-central/rev/77f89c68ba08
Comment 45 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-06 15:14:24 PDT
Created attachment 786538 [details] [diff] [review]
Implement --enable-intl-api=disable|build|implement (default implement)

disable:
* does what it says
build:
* builds ICU, builds Intl code, doesn't do anything which would affect visible JS behavior -- no Intl global, existing toLocaleString methods unaffected; also doesn't init/cleanup ICU as it should never be used
implement:
* builds ICU, builds Intl code, inits/cleanups ICU, JS semantics include Intl, toLocaleString changes, etc.

Shell builds with the three different options pass all of jstests.py.  Try builds before/after this are in progress, to permit comparison of perf numbers:

https://tbpl.mozilla.org/?tree=Try&rev=f494a68ea0aa (before)
https://tbpl.mozilla.org/?tree=Try&rev=442efbbb135b (after)

The build change bits are, I think, relatively straightforward.  I totally don't care about the string values, or the #define names, but please don't bikeshed 'em too much.

The non-build bits mostly involved auditing all instances of ENABLE_INTL_API and converting the relevant ones to EXPOSE_INTL_API.  After a false start or two I think I got it all right, and jstests.py backs me up on that, but double-checking everything there is definitely desirable.
Comment 46 Till Schneidereit [:till] 2013-08-06 16:48:35 PDT
Comment on attachment 786538 [details] [diff] [review]
Implement --enable-intl-api=disable|build|implement (default implement)

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

Mmh, I don't quite understand why you didn't just let ENABLE_INTL_API keep its meaning and introduced something like BUILD_ICU. In that case, none of the source changes would be necessary, and it doesn't look like the configure and makefile changes would be too difficult.

The changes as they are look fine, though, so r=me in reliance on your judgement on this question.
Comment 47 Till Schneidereit [:till] 2013-08-06 18:27:10 PDT
(In reply to Till Schneidereit [:till] from comment #46)
> Mmh, I don't quite understand why you didn't just let ENABLE_INTL_API keep
> its meaning and introduced something like BUILD_ICU. In that case, none of
> the source changes would be necessary, and it doesn't look like the
> configure and makefile changes would be too difficult.

Meh, I just realized that it's not entirely clear what I mean here. Keeping the meaning of ENABLE_INTL_API and introducing BUILD_ICU would only require source code changes in Intl.cpp, IINM. And I'm not even entirely sure those should be made: we want ICU to be fully integrated but just not called by anything, after all.
Comment 48 Mike Hommey [:glandium] 2013-08-08 02:31:03 PDT
Comment on attachment 786538 [details] [diff] [review]
Implement --enable-intl-api=disable|build|implement (default implement)

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

I don't understand what the goal is here. If the code is built but not used, it's going to be stripped off by the linker. Actually, now that I look, it's not going to be, which is probably part of the problem for the footprint increase: all the ICU symbols are exported, which means a bigger symbols table, more expensive function calls, less LTO...
Comment 49 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-08 16:26:44 PDT
(In reply to Matt Brubeck (:mbrubeck) from comment #32)
> Possible clue: "Tp5 No Network
> Row Major MozAfterPaint (Main Startup File IO Bytes)" regressed by 34%
> (about 10MB).

This is either a red herring, or something we deliberately have to eat, and have chosen to eat, to do anything in this bug at all.  Here are perf comparisons for the latest patch, that adds ICU to the build, adjusts code compiled by the JS engine accordingly, but doesn't actually add the Intl property to the global object, or change how the existing toLocaleString methods work, or affect JS engine behavior at all (except in code/symbol layout).  This shows roughly the difference between not having ICU at all, and compiling ICU into the JS engine but never using it:

http://perf.snarkfest.net/compare-talos/index.html?oldRevs=f494a68ea0aa&newRev=442efbbb135b&submit=true

That shows the same increase in this stat, I think because (as the name itself seems to point out) this is library/binary size loaded by the OS at startup.  Unless someone happens to have a 10MB library removal lying around somewhere, I don't see how this numeric increase can be eliminated.

> Similar to the previous landing, this regressed Ts by ~25ms, tpaint by about
> 10ms, and increased various memory counters.

tpaint in that comparison looks unchanged, I think.  I'm still trying to understand the Ts numbers.  I don't know the meaning of the apparent memory counters well enough to say what's happened to them, just yet.
Comment 50 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-08 16:34:10 PDT
(In reply to Mike Hommey [:glandium] from comment #48)
> I don't understand what the goal is here.

The goal is to produce a build with all the ICU bits in it, compiled and linked in, but with no actual functionality change, and without any new code executed at runtime (except that in the OS to load libraries, etc. under the hood).  This disentangles codesize/library size/etc. effects from those incurred by actually exposing the Intl API, and by any code in the browser or in webpages that puts it to use.

> If the code is built but not used,
> it's going to be stripped off by the linker. Actually, now that I look, it's
> not going to be, which is probably part of the problem for the footprint
> increase: all the ICU symbols are exported, which means a bigger symbols
> table, more expensive function calls, less LTO...

Why would increasing the symbol table size make non-ICU function calls more expensive?  (Note that in the long run, we probably will start using ICU in places beyond the JS engine, so we will want these symbols -- or at least a subset of them -- exported.)

Note that we don't want to touch ICU, and it seems doubtful that we could make per-symbol export decisions without touching it.  So exporting all the symbols seems likely to be representative of what we ultimately ship.
Comment 51 Mike Hommey [:glandium] 2013-08-08 16:54:49 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #50)
> (In reply to Mike Hommey [:glandium] from comment #48)
> > I don't understand what the goal is here.
> 
> The goal is to produce a build with all the ICU bits in it, compiled and
> linked in, but with no actual functionality change, and without any new code
> executed at runtime (except that in the OS to load libraries, etc. under the
> hood).  This disentangles codesize/library size/etc. effects from those
> incurred by actually exposing the Intl API, and by any code in the browser
> or in webpages that puts it to use.
> 
> > If the code is built but not used,
> > it's going to be stripped off by the linker. Actually, now that I look, it's
> > not going to be, which is probably part of the problem for the footprint
> > increase: all the ICU symbols are exported, which means a bigger symbols
> > table, more expensive function calls, less LTO...
> 
> Why would increasing the symbol table size make non-ICU function calls more
> expensive?  (Note that in the long run, we probably will start using ICU in
> places beyond the JS engine, so we will want these symbols -- or at least a
> subset of them -- exported.)

Calls to exported symbols go through the PLT instead of going directly. It also means that the symbols are exported for other libraries to use, and that if a system library uses ICU, it might end up using our symbols, which may or may not be a problem.

> Note that we don't want to touch ICU, and it seems doubtful that we could
> make per-symbol export decisions without touching it.  So exporting all the
> symbols seems likely to be representative of what we ultimately ship.

We should just hide all the ICU symbols. There's no point in exporting them.
Comment 52 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-08 17:06:48 PDT
(In reply to Mike Hommey [:glandium] from comment #51)
> Calls to exported symbols go through the PLT instead of going directly.

But only calls to the ICU symbols get slowed down, right?  I interpreted your comment as saying that calls to other symbols would get slowed down, too.

> It also means that the symbols are exported for other libraries to use, and
> that if a system library uses ICU, it might end up using our symbols, which
> may or may not be a problem.

Hm, yeah, we probably have to do something about that eventually.  It sounds like something we can defer til after this is on in desktop builds, tho, unless I'm missing something.

> We should just hide all the ICU symbols. There's no point in exporting them.

That's only true as long as it's only JS using them, tho, right?  Once libxul starts wanting to use them, we have to export at least the used symbols for Windows, don't we?  (We can cross that bridge when we get to it, perhaps, but if possible I'd rather not rely on a temporary hack that leaves someone else with an impossible problem later.)
Comment 53 Mike Hommey [:glandium] 2013-08-08 17:12:12 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #52)
> > We should just hide all the ICU symbols. There's no point in exporting them.
> 
> That's only true as long as it's only JS using them, tho, right?  Once
> libxul starts wanting to use them, we have to export at least the used
> symbols for Windows, don't we?  (We can cross that bridge when we get to it,
> perhaps, but if possible I'd rather not rely on a temporary hack that leaves
> someone else with an impossible problem later.)

It should be as simple as forcing the ICU build system to use the VISIBILITY_FLAGS, and later, when needing to export the symbols on windows, add a DEFFILE exporting those symbols. (I don't think there's a point trying to export the symbols on other platforms, people like me who build js as a shared library on linux should use system icu instead)
Comment 54 Mike Hommey [:glandium] 2013-08-08 17:13:27 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #50)
> (In reply to Mike Hommey [:glandium] from comment #48)
> > I don't understand what the goal is here.
> 
> The goal is to produce a build with all the ICU bits in it, compiled and
> linked in, but with no actual functionality change, and without any new code
> executed at runtime (except that in the OS to load libraries, etc. under the
> hood).  This disentangles codesize/library size/etc. effects from those
> incurred by actually exposing the Intl API, and by any code in the browser
> or in webpages that puts it to use.

Why would that need to land?
Comment 55 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-08 17:27:13 PDT
Two reasons:

1) I'm stupid and don't understand Talos numbers nearly as well as other people do, and actually landing this will get better, more knowledgeable evaluations of perf impact than if I do comparisons and interpret them.
2) As long as ICU/Intl stuff is disabled at compile time in desktop builds, there's a chance of it bitrotting.  Enabling it partially means that those portions of the overall work won't bitrot on desktop.
Comment 56 Mike Hommey [:glandium] 2013-08-08 18:17:05 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #55)
> Two reasons:
> 
> 1) I'm stupid and don't understand Talos numbers nearly as well as other
> people do, and actually landing this will get better, more knowledgeable
> evaluations of perf impact than if I do comparisons and interpret them.

Nothing you/they can't do with try pushes, right?

> 2) As long as ICU/Intl stuff is disabled at compile time in desktop builds,
> there's a chance of it bitrotting.  Enabling it partially means that those
> portions of the overall work won't bitrot on desktop.

But your patch doesn't enable it partially on desktop.
Comment 57 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-08 18:38:03 PDT
(In reply to Mike Hommey [:glandium] from comment #56)
> Nothing you/they can't do with try pushes, right?

To some extent.  But also: 1) I'd rather not have such a patch constantly at the bottom of my patch queue, if I can help it.  2) Landing this makes the patch more readily available for others to help with debugging size/perf differences in the Intl functionality.

> But your patch doesn't enable it partially on desktop.

How does

+if test "$MOZ_BUILD_APP" = "browser"; then
+    # In desktop builds the Internationalization API is disabled, but all code
+    # for it is still built.  Bug 853301 will switch this to "implement".
+    ac_configure_args="$ac_configure_args --enable-intl-api=build"
+else

not partially enable it on desktop?
Comment 58 Norbert Lindenberg 2013-08-08 18:50:13 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #49)

> This is either a red herring, or something we deliberately have to eat, and
> have chosen to eat, to do anything in this bug at all.

I don't pretend to understand what all the Ts and Tp numbers mean, but maybe somebody more knowledgeable can cross-reference them to the measurements that Dave Mandelin did in February, comparing a build with ICU and the internationalization API against one without on Windows 7. I do see the 10MB increase in DLL size and the associated read time being discussed. Asa and Taras were aware of Dave's findings when they OKed the feature.

Dave's findings:

> 1. Sizes
> 
>                      trunk             +ICU
> zip file            25.7 MB          28.6 MB    = + 3 MB for ICU
> installed           44.9 MB          54.8 MB    = +10 MB for ICU
> 
> 2. Warm startup
> 
> I did 3 trials. Times are in ms.
> 
>  trunk:  1031, 769, 843
>  +ICU:   1449, 759, 778
> 
> I call this one "no difference".
> 
> 3. Cold startup
> 
> Three trials again, times in ms.
> 
>  trunk:  3885, 3798, 3376
>  +ICU:   4393, 4303, 3156
> 
> This is hard to get anything from: +ICU was half a second slower on two trials, but then it was fastest of all the trials. Cold startup times in general seem pretty unreliable, because other system activity can use the disk and disrupt things, and also because Windows has features that optimize disk access, so different trials are not necessarily even doing the same access patterns.
> 
> To try to understand this better, I looked at what's different about the builds and how that shows up in a fileio profile. The difference is that on trunk, mozjs.dll (the JS engine) is 2.8 MB, while with +ICU, mozjs.dll is 12.7 MB because it includes ICU code and data (mostly data, I believe). In a fileio profile, mozjs.dll is loaded by the System process (Process ID 4), with a size of 12.7 MB (the whole thing gets loaded) and a time taken of 15 ms. For comparison, in the same fileio profile, xul.dll loaded 18.0 MB in 23 ms, which is comparable.
> 
> That would suggest the effect on cold startup of including ICU is not more than 15 ms. There is one anomaly that makes me question that conclusion, which is that the fileio numbers suggest a read data rate of 1000 MB/s, and I thought disks were typically not more than 100 MB/s. Taras and Vladan might know what this means. My guess would be that something was prefetching mozjs.dll (although I tried to turn the prefetching optimizations off) and the fileio profile shows the time it takes for the file call to complete not counting the prefetching time.
> 
> Based on that, I'd guess that if prefetching optimizations are active and effective, the effect on startup time might be about 15 ms. But if the optimizations are turned off, don't work, or are not available (I believe some of them are in XP but not all of them), then the effect on startup time is probably about 150 ms.


Taras commented:

> Adding data to existing files is nearly free due to optimization Mike and me did where we preload most of our dlls. So as long as new files are not added, you are adding data to a file that's being read sequentially at the maximum speed of the disk. That overhead would be small enough to be lost in the noise of random io we do elsewhere. dlls are read at >10-60mb/s vs roughly 100x faster than random IO which is to blame for most of our startup time.
Comment 59 Mike Hommey [:glandium] 2013-08-08 19:40:17 PDT
Comment on attachment 786538 [details] [diff] [review]
Implement --enable-intl-api=disable|build|implement (default implement)

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

::: configure.in
@@ +9418,5 @@
>  ac_configure_args="$ac_configure_args --enable-threadsafe"
> +if test "$MOZ_BUILD_APP" = "browser"; then
> +    # In desktop builds the Internationalization API is disabled, but all code
> +    # for it is still built.  Bug 853301 will switch this to "implement".
> +    ac_configure_args="$ac_configure_args --enable-intl-api=build"

Ah, somehow i thought the disable below was for the browser.
Please limit this to $NIGHTLY_BUILD = 1

::: js/src/configure.in
@@ +4253,5 @@
> +                          Determine the status of the ECMAScript Internationalization API.
> +                          Values are disable (don't build it), build (build the
> +                          support code for it, but disable it in JS), and
> +                          implement (build it and enable it in JS).],
> +                      [_ENABLE_INTL_API=$enableval])

--enable=disable is confusing. It would be better to switch to a MOZ_ARG_WITH_STRING, in which case you can do --with-intl-api, --without-intl-api and --with-intl-api=build-only.
Comment 60 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-08 20:52:11 PDT
Created attachment 787940 [details] [diff] [review]
Convert to a MOZ_ARG_WITH_STRING

Incremental diff here, for anyone who wants to see it, before it gets GC'd by pastebin (despite being retained -- ahem -- forever):

http://pastebin.mozilla.org/2813900

No changes to JS bits, just to build bits, so till's previous review should still be good.
Comment 61 Mike Hommey [:glandium] 2013-08-08 22:23:11 PDT
Comment on attachment 787940 [details] [diff] [review]
Convert to a MOZ_ARG_WITH_STRING

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

::: configure.in
@@ +9403,5 @@
> +if test "$NIGHTLY_BUILD" = "1" -a "$MOZ_BUILD_APP" = "browser"; then
> +    # In desktop nightlies the Internationalization API is disabled, but all
> +    # code for it is still built.  Bug 853301 will remove this so that it's
> +    # built and the API is enabled.
> +    ac_configure_args="$ac_configure_args --with-intl-api=build"

You know what? Considering ICU is a real drag for build time, especially because it's not parallelized, I think it would be better set in the browser/ mozconfigs, and left --without in configure.in.
Comment 62 Ted Mielczarek [:ted.mielczarek] 2013-08-09 09:56:00 PDT
Having a big feature like this be built in automation but not on developer builds seems like a big footgun for developers. I'd be happier to just have a post on dev.platform telling developers how to disable ICU for their local builds, caveat emptor.
Comment 63 Mike Hommey [:glandium] 2013-08-09 16:36:35 PDT
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #62)
> Having a big feature like this be built in automation but not on developer
> builds seems like a big footgun for developers. I'd be happier to just have
> a post on dev.platform telling developers how to disable ICU for their local
> builds, caveat emptor.

Do you think there's really a risk of a random developer breaking ICU considering how it's hooked up?
Comment 64 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-12 14:19:01 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/680a80d15f3e

I'd say the risk is small, ish, but not zero.  In any case, the lost parallel time from ICU building non-parallel seems like a fairly small loss, all things considered -- especially if we're going to be eating that when this is fully, fully enabled.
Comment 65 Carsten Book [:Tomcat] - PTO-back Sept 4th 2013-08-13 05:06:52 PDT
https://hg.mozilla.org/mozilla-central/rev/680a80d15f3e
Comment 66 Guillaume Abadie 2013-08-15 09:00:22 PDT
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #64)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/680a80d15f3e
> 
> I'd say the risk is small, ish, but not zero.  In any case, the lost
> parallel time from ICU building non-parallel seems like a fairly small loss,
> all things considered -- especially if we're going to be eating that when
> this is fully, fully enabled.
I can't build on Mac OS X 10.9 anymore with this change set. (found by bisection) Where I'm not familiar with configure.in, is there a .mozconfig pref to work around?
Comment 67 Mike Hommey [:glandium] 2013-08-15 09:03:48 PDT
(In reply to Guillaume Abadie from comment #66)
> I can't build on Mac OS X 10.9 anymore with this change set. (found by
> bisection) Where I'm not familiar with configure.in, is there a .mozconfig
> pref to work around?

ac_add_options --without-intl-api.
Comment 68 Guillaume Abadie 2013-08-15 11:03:34 PDT
(In reply to Mike Hommey [:glandium] from comment #67)
> (In reply to Guillaume Abadie from comment #66)
> > I can't build on Mac OS X 10.9 anymore with this change set. (found by
> > bisection) Where I'm not familiar with configure.in, is there a .mozconfig
> > pref to work around?
> 
> ac_add_options --without-intl-api.

That doesn't work either.
Comment 69 Jeff Walden [:Waldo] (remove +bmo to email) 2013-08-15 11:28:56 PDT
(In reply to Guillaume Abadie from comment #66)
> I can't build on Mac OS X 10.9 anymore with this change set. (found by
> bisection) Where I'm not familiar with configure.in, is there a .mozconfig
> pref to work around?

Could you post the relevant compilation failure log bits?
Comment 70 Guillaume Abadie 2013-08-15 11:54:50 PDT
Created attachment 790881 [details]
Mac OS X 10.9 build error log

Here is the log compiling on:
changeset:   142346:680a80d15f3e
user:        Jeff Walden <jwalden@mit.edu>
date:        Thu Aug 08 15:17:08 2013 -0700
Comment 71 Jeff Walden [:Waldo] (remove +bmo to email) 2013-09-18 14:59:45 PDT
Created attachment 806917 [details]
callgrind of Intl-enabled shell doing |./js -e Intl|

According to this data, 57% of time running |./js -e Intl| is in JSRuntime::initSelfHosting.  Of that time, most of it (52% of total [not of that 57%]) is in JS::Evaluate of the self-hosted code.  Slightly less than half of JS::Evaluate time is in frontend::CompileScript, and slightly more than half is in js::Execute.

Breakpoint 4, JS::Evaluate (cx=cx@entry=0x14005e0, obj=obj@entry=(JSObject * const) 0x7ffff1803060 [object self-hosting-global] delegate, options=..., 
    bytes=bytes@entry=0x1408570 "\n\n\n\n\nvar std_isFinite = isFinite;\nvar std_isNaN = isNaN;\nvar std_Array_indexOf = ArrayIndexOf;\nvar std_Array_join = Array.prototype.join;\nvar std_Array_push = Array.prototype.push;\nvar std_Array_shift"..., length=length@entry=112766, rval=rval@entry=0x7fffffffdbc0) at /home/jwalden/moz/no-intl/js/src/jsapi.cpp:4919
4919	{

The self-hosted code comes to 112766 bytes right now, which is kind of huge.  I haven't looked at the overall string yet to see exactly what's in it (although I vaguely gather it's the concatenation of self-hosted JS listed in js/src/buildfile).  Given most of it (I believe) is just declaring functions, it's surprising to me that we'd spend much time at all in executing anything.

Anyway, more data to put out there, no real progress made yet on this.  :-|
Comment 72 Till Schneidereit [:till] 2013-09-18 16:05:04 PDT
Created attachment 806966 [details]
Annotated callgrind of Intl-enabled debug shell doing |js -e ''|

This is taken with today's valgrind svn because 3.8.1 doesn't work on OS X 10.8.

The annotation was done with --inclusive=yes.

If I'm reading the profile correctly, it mostly corroborates Waldo's findings: ~80% of the entire run time (which essentially means startup time) is spent under initSelfHosting. Of that, slightly more than 50% are spent under CompileScript, and slightly less under Execute. While that's reversed compared to Waldo's profile, it doesn't seem to matter much.

I'm not sure this profile is to be trusted otherwise, however. More time being spent under frontend::EmitTree than under CompileScript doesn't seem right. And it's not just slightly more, either: It's 1.5x as much.

Moving on, though: if any of this is correct, we spent almost 40% of total run time under DefineNativeProperty. If true, we might want to look at optimizing that.
Comment 73 Till Schneidereit [:till] 2013-09-18 17:19:37 PDT
Created attachment 806994 [details]
Annotated callgrind of Intl-enabled debug shell doing |js -e ''|

Uh, so, completely ignore that last comment. Here's a new, actually (or, hopefully, but probably) correct and informative profile.

This one contains one-level-deep callee info for all functions, so we can see where under a function time is spent. I removed all callee info from system calls, because it didn't seem that interesting.

In the following, all percentages are fractions of the total of the caller, not the entire run time.

So, more than 60% of time is spent in the linker, with less than 40% spent under js.cpp:main, with, again, ~84% of that under JSRuntime::initSelfHosting.

92% of that time is spent under JS::Evaluate. Of that, 71% is spent under CompileScript, and ~29% under JS::Execute. 

Some additional notes:

We spend almost as much time under js::DefineNativeProperty as we do under JS::Execute.

Almost all timer under JS::Execute is spent under three functions:
- js::intl_Collator_availableLocales
- js::intl_DateTimeFormat_availableLocales
- js::intl_NumberFormat_availableLocales

These, in turn, actually spend all their time under intl_availableLocales. From a quick glance, it looks like we should be able to call this function once and reuse the result. That would shave off roughly 60% of the time under JS::Execute.
Comment 74 Jeff Walden [:Waldo] (remove +bmo to email) 2013-09-18 17:40:19 PDT
Created attachment 807000 [details] [diff] [review]
Make available-locale computation lazier, draft patch

This seems to take Intl stuff out of the callgrind profile a bit better.  Not sure it's totally correct or totally complete, but I wanted to post it now so others can see it, at least.
Comment 75 Norbert Lindenberg 2013-09-18 22:55:41 PDT
(In reply to Till Schneidereit [:till] from comment #73)
> Created attachment 806994 [details]
> Annotated callgrind of Intl-enabled debug shell doing |js -e ''|
> 
> Uh, so, completely ignore that last comment. Here's a new, actually (or,
> hopefully, but probably) correct and informative profile.
> 
> This one contains one-level-deep callee info for all functions, so we can
> see where under a function time is spent. I removed all callee info from
> system calls, because it didn't seem that interesting.
> 
> In the following, all percentages are fractions of the total of the caller,
> not the entire run time.
> 
> So, more than 60% of time is spent in the linker, with less than 40% spent
> under js.cpp:main, with, again, ~84% of that under
> JSRuntime::initSelfHosting.
> 
> 92% of that time is spent under JS::Evaluate. Of that, 71% is spent under
> CompileScript, and ~29% under JS::Execute. 
> 
> Some additional notes:
> 
> We spend almost as much time under js::DefineNativeProperty as we do under
> JS::Execute.
> 
> Almost all timer under JS::Execute is spent under three functions:
> - js::intl_Collator_availableLocales
> - js::intl_DateTimeFormat_availableLocales
> - js::intl_NumberFormat_availableLocales
> 
> These, in turn, actually spend all their time under intl_availableLocales.
> From a quick glance, it looks like we should be able to call this function
> once and reuse the result. That would shave off roughly 60% of the time
> under JS::Execute.

intl_availableLocales receives different ICU functions as parameters in the three calls, and the actual locale list returned for Collator is significantly shorter than the other two.
Comment 76 Jeff Walden [:Waldo] (remove +bmo to email) 2013-09-18 23:47:28 PDT
Good to know.  I expressed skepticism on IRC that we could (correctly) just do a single call, precisely because of the different-function-pointers thing suggesting wildly different results between them.  But I didn't bother translating it into a bug comment when attachment 807000 [details] [diff] [review] was easy to whip up for testing.

Odds are I'll kick off some try runs without/with that patch tonight, to see what the results are in the morning.  But it's also getting late, and I have a chore or two to do still at home, so no promises.
Comment 77 Florian Bender 2013-09-19 00:30:34 PDT
Are self-hosted scripts not lazy-parsed?
Comment 78 Jeff Walden [:Waldo] (remove +bmo to email) 2013-09-19 00:48:43 PDT
They are not.  Bug 900292 would change that, but the numbers there tentatively suggest it doesn't make much difference either way (presumably most stuff would get compiled anyway eventually, and it only takes once for something to be compiled).  The perf issues here are all per-window anyway and would be unaffected by lazy parsing.
Comment 79 Till Schneidereit [:till] 2013-09-19 07:31:52 PDT
Created attachment 807202 [details]
Annotated callgrind of Intl-enabled lazily-parsing-self-hosted-code shell doing |js -e ''|

More details to follow with the next attachment, but here are some highlights:

- time spent under js.cpp:main reduced by 13%
- time spent under CompileScript reduced by 40%
- time spent under Execute increased by 30%
Comment 80 Till Schneidereit [:till] 2013-09-19 07:40:14 PDT
Created attachment 807203 [details]
Annotated callgrind of Intl-enabled lazily-parsing-self-hosted-code-lazy-available-locale shell doing |js -e ''|

This is with lazy-parsing enabled and Waldo's patch for making locale resolution lazy applied.

Highlights:
- time spent under js.cpp:main reduced by 30%
- time spent under CompileScript reduced by 44%
- time spent under Execute reduced by 31%
- almost all time spent under Execute is actually spent under intrinsic_SetScriptHints
- almost all of that, in turn, is spent under createScriptForLazilyInterpretedFunction

I'll look into why we're de-lazifying scripts for SetScriptHints and see if we can get around that next. However, even without changing that, we're in pretty good shape here.

Also, SetScriptHints doesn't matter too much, as it's only used in PJS, which is disabled in Beta and Release, IINM.
Comment 81 Jeff Walden [:Waldo] (remove +bmo to email) 2013-09-20 13:46:23 PDT
Created attachment 807947 [details] [diff] [review]
Make available-locale computation lazier

If http://perf.snarkfest.net/compare-talos/index.html?oldRevs=fc961c6d9ced&newRev=e46c340ac68d&submit=true is to be believed, there's still a bit of a Tpaint issue with this change.  :-\  Bleh.  We might as well get it in, tho, so it's not a back-of-the-mind issue.
Comment 82 Till Schneidereit [:till] 2013-09-20 14:26:18 PDT
Comment on attachment 807947 [details] [diff] [review]
Make available-locale computation lazier

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

yup
Comment 83 Jeff Walden [:Waldo] (remove +bmo to email) 2013-09-20 15:09:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da8cfe2c47f
Comment 84 Wes Kocher (:KWierso) 2013-09-20 19:37:09 PDT
https://hg.mozilla.org/mozilla-central/rev/8da8cfe2c47f
Comment 85 Till Schneidereit [:till] 2013-09-21 07:58:51 PDT
I did some more measurements and found out what causes the tpaint regression: for new windows (and tabs), we're not lazily initializing the standard classes. That causes Intl and Intl.Collator & co to be initialized, including running the initialization JS. For a browser run to an empty window, then opening a second empty window and closing the browser down, that causes 100 million instructions to be executed (out of ~4 billion).

I'm working on consistently doing lazy standard class initialization in bug 919095. Not adding that as a dependency here, though, as Waldo's working on a patch that makes initialization of the Intl object itself more lazy. Given that eagerly initializing standard classes without Intl enabled is only about 0.15% of all instructions in the above-described scenario, we might actually rip out support for lazy standard class initialization.
Comment 86 Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-03 06:27:32 PDT
Created attachment 813511 [details] [diff] [review]
Adjust test_interfaces.html to account for Intl on desktop
Comment 87 Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-03 06:28:59 PDT
Created attachment 813512 [details] [diff] [review]
Turn on the Intl API in desktop builds

It looks like lazy initialization in bug 919872 got rid of the previous regressions, so I think we these two patches we can turn on Intl now.
Comment 88 Mike Hommey [:glandium] 2013-10-06 07:13:37 PDT
Can we keep it disabled for local builds?
Comment 89 Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-08 03:55:40 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6dc96f18391

I don't think it's practical to disable this in local builds.  If the browser itself doesn't start using it in short order, I'll be very surprised.  (I'm also aware of layout bits that want to be able to depend on ICU being around, as well.)  Plus, to be honest, how does the same argument not apply to WebRTC, or any other big-ish spec things we're adding support for?  It's a bit of a losing battle to get into supporting removing specified, standardized web features just because they don't build quickly enough, or nicely enough.
Comment 90 Mike Hommey [:glandium] 2013-10-08 16:11:20 PDT
I've actually made the argument for webrtc on dev-platform. But the thing is: ICU builds at -j1. It's slower to build than anything else.
Comment 91 Mike Hommey [:glandium] 2013-10-08 16:12:45 PDT
So here's my point: ICU shouldn't be enabled on local builds or for anything in gecko until its build system is fixed by someone.
Comment 92 :Ehsan Akhgari 2013-10-08 16:15:43 PDT
(In reply to comment #90)
> I've actually made the argument for webrtc on dev-platform. But the thing is:
> ICU builds at -j1. It's slower to build than anything else.

What happens if we just simply change it to use -jN N being whatever the rest of our build uses?
Comment 93 Mike Hommey [:glandium] 2013-10-08 16:43:42 PDT
(In reply to :Ehsan Akhgari (needinfo? me!) from comment #92)
> (In reply to comment #90)
> > I've actually made the argument for webrtc on dev-platform. But the thing is:
> > ICU builds at -j1. It's slower to build than anything else.
> 
> What happens if we just simply change it to use -jN N being whatever the
> rest of our build uses?

It fails randomly with segmentation faults (which, by the way, for some reason happens on mac builders (which gets me wondering if their make is not broken))
Comment 94 :Ehsan Akhgari 2013-10-08 17:48:20 PDT
(In reply to comment #93)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #92)
> > (In reply to comment #90)
> > > I've actually made the argument for webrtc on dev-platform. But the thing is:
> > > ICU builds at -j1. It's slower to build than anything else.
> > 
> > What happens if we just simply change it to use -jN N being whatever the
> > rest of our build uses?
> 
> It fails randomly with segmentation faults (which, by the way, for some reason
> happens on mac builders (which gets me wondering if their make is not broken))

Can we use pymake on mac to build ICU?  (I know I know, this is me suggesting to use pymake ;-)
Comment 95 Carsten Book [:Tomcat] - PTO-back Sept 4th 2013-10-09 01:28:49 PDT
https://hg.mozilla.org/mozilla-central/rev/b6dc96f18391
Comment 96 Steven R. Loomis 2013-10-29 11:57:48 PDT
(In reply to Mike Hommey [:glandium] from comment #93)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #92)
> > (In reply to comment #90)
> > > I've actually made the argument for webrtc on dev-platform. But the thing is:
> > > ICU builds at -j1. It's slower to build than anything else.
> > 
> > What happens if we just simply change it to use -jN N being whatever the
> > rest of our build uses?
> 
> It fails randomly with segmentation faults (which, by the way, for some
> reason happens on mac builders (which gets me wondering if their make is not
> broken))

Have you filed an upstream on this? I build ICU -j8 all the time, never saw random segfaults.  (Or is this the "without collation rules" issue mentioned in http://bugs.icu-project.org/trac/ticket/10043 somehow?)

Not trying to reopen the ticket, but let me know what's going on here.
Comment 97 Jeff Walden [:Waldo] (remove +bmo to email) 2013-10-29 13:48:33 PDT
I suspect the issue is a multi-threading race issue in the relevant code whose binary is being executed.  I've been meaning to investigate it myself, but I've had more basic ICU-related priorities to deal with before getting to that, thus far.  (Plus other unrelated issues of greater importance.)
Comment 98 Mike Hommey [:glandium] 2013-10-29 15:01:11 PDT
(In reply to Steven R. Loomis from comment #96)
> Have you filed an upstream on this? I build ICU -j8 all the time, never saw
> random segfaults.  (Or is this the "without collation rules" issue mentioned
> in http://bugs.icu-project.org/trac/ticket/10043 somehow?)
> 
> Not trying to reopen the ticket, but let me know what's going on here.

Seems plausible to be the same thing.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #97)
> I suspect the issue is a multi-threading race issue in the relevant code
> whose binary is being executed.  I've been meaning to investigate it myself,
> but I've had more basic ICU-related priorities to deal with before getting
> to that, thus far.  (Plus other unrelated issues of greater importance.)

It's not. It's a pure parallel make issue, where it's running commands against data that's not there yet.
Comment 99 Chris Peterson [:cpeterson] 2014-02-04 13:45:06 PST
FYI for those writing release notes and dev docs: this feature landed disabled in Firefox 27, but was not actually enabled until Firefox 28.
Comment 100 Jeff Walden [:Waldo] (remove +bmo to email) 2014-02-04 18:01:35 PST
Hmm, not entirely quite.  It was in what used to be Aurora, but on uplift to Beta it should have been disabled.  The current Aurora, Firefox 29, is the first version of Firefox that should hit release that should have it enabled.
Comment 101 Florian Scholz [:fscholz] (MDN) 2014-02-05 12:03:43 PST
(In reply to Chris Peterson (:cpeterson) from comment #99)
> FYI for those writing release notes and dev docs: this feature landed
> disabled in Firefox 27, but was not actually enabled until Firefox 28.

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #100)
> Hmm, not entirely quite.  It was in what used to be Aurora, but on uplift to
> Beta it should have been disabled.  The current Aurora, Firefox 29, is the
> first version of Firefox that should hit release that should have it enabled.

Thanks Chris and Jeff. I have added it to the developer release notes for Firefox 29 now.
https://developer.mozilla.org/en-US/Firefox/Releases/29#JavaScript

The reference docs also got cleaned up and now mention desktop Firefox 29.
Dumping a list here to know what to review/rewrite if anything changes or gets backed out:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/prototype
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/compare
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/resolvedOptions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Collator/supportedLocalesOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/prototype
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/format
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/resolvedOptions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/DateTimeFormat/supportedLocalesOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/prototype
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/format
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/resolvedOptions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/NumberFormat/supportedLocalesOf

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/toLocaleString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleDateString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toLocaleTimeString

Feel free to review these docs and please let us know if the version changes (again).
(reopen the doc request with dev-doc-complete -> dev-doc-needed).
Comment 102 Jean-Yves Perrier [:teoli] 2014-02-05 13:42:53 PST
Given comment 100, I think we should relnote this for Aurora 29. Ccing Sylvestre as the relnote flag can be unoticed given the target milestone.
Comment 103 Ioana (away) 2014-03-07 06:22:05 PST
Is there any need for manual QA for this feature?
Comment 104 Jeff Walden [:Waldo] (remove +bmo to email) 2014-03-11 16:16:27 PDT
No, automated tests and incidental web usage should be adequate here, I think.
Comment 105 ijdt.editor 2015-02-27 09:36:16 PST
Hello,

I am trying to compile Firefox 36 on OpenIndiana (the open fork of Solaris). I have installed the various things needed (e.g., pulse-audio, Python 2.7.X, etc.) and when I try to configure, the
configuration process stops with a message that says something like "ECMAScript Internationalization API is not defined for this platform". Maybe it is not defined somewhere in Firefox configuration files, but I am sure that this thing works under Solaris. For example, I am compiling TeXLive for Solaris with no problem and this includes icu and other things. Note that I am using GCC 4.8.2 not Sun Studio. Does anybody have any suggestion to help me on this? Thanks in advance for any suggestion and/or help.
Comment 106 Steven R. Loomis 2015-02-27 10:08:05 PST
(In reply to ijdt.editor from comment #105)
> Hello,
> 
> I am trying to compile Firefox 36 on OpenIndiana (the open fork of Solaris).
> I have installed the various things needed (e.g., pulse-audio, Python 2.7.X,
> etc.) and when I try to configure, the
> configuration process stops with a message that says something like
> "ECMAScript Internationalization API is not defined for this platform".
> Maybe it is not defined somewhere in Firefox configuration files, but I am
> sure that this thing works under Solaris. For example, I am compiling
> TeXLive for Solaris with no problem and this includes icu and other things.
> Note that I am using GCC 4.8.2 not Sun Studio. Does anybody have any
> suggestion to help me on this? Thanks in advance for any suggestion and/or
> help.

I'm guessing this is not the right place for compilation help, though I'm not sure where is. 
I'm not a moz expert just an icu dev.

In any event, the file to modify looks like this https://hg.mozilla.org/mozilla-central/file/c7968255c1ea/build/autoconf/icu.m4#l89

(MOz people- this file looks way too complex.  And ICU supports a bunch of platforms not listed. Not sure what's happening here).

ANywyas, ijdt, ICU works fine on OpenIndiana despite what the file says. Hack around with your local copy of build/autoconf/icu.m4 is my advice.
Comment 107 ijdt.editor 2015-03-03 10:03:06 PST
I guess it is because some configure files are wrong. So once needs to fix them. The following files 

./configure
./js/src/configure
./build/autoconf/icu.m4

need to be patched as follows

--- configure.orig	2015-03-03 18:04:49.759472921 +0200
+++ configure	2015-03-03 17:54:10.477556967 +0200
@@ -28550,7 +28550,7 @@
                     MOZ_ICU_DBG_SUFFIX=d
                 fi
                 ;;
-            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD)
+            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD|SunOS)
                 ICU_LIB_NAMES="icui18n icuuc icudata"
                 ;;
             *)

--- js/src/configure.orig	2015-03-03 18:06:28.765386878 +0200
+++ js/src/configure	2015-03-03 18:06:58.332710458 +0200
@@ -15409,7 +15409,7 @@
                     MOZ_ICU_DBG_SUFFIX=d
                 fi
                 ;;
-            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD)
+            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD|SunOS)
                 ICU_LIB_NAMES="icui18n icuuc icudata"
                 ;;
             *)

--- build/autoconf/icu.m4.orig	2015-03-03 18:03:50.231266800 +0200
+++ build/autoconf/icu.m4	2015-02-28 17:30:32.423919341 +0200
@@ -89,7 +89,7 @@
                     MOZ_ICU_DBG_SUFFIX=d
                 fi
                 ;;
-            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD)
+            Darwin|Linux|DragonFly|FreeBSD|NetBSD|OpenBSD|GNU/kFreeBSD|SunOS)
                 ICU_LIB_NAMES="icui18n icuuc icudata"
                 ;;
             *)

Now configure does not stop with this error message. Please include these patches into the source tree.
Comment 108 Magnus Melin 2015-03-03 11:38:19 PST
This is a closed bug. Please attach the diff as an attachment to a new bug. (Click "Clone this bug" below)
Then ask for review - https://developer.mozilla.org/en/docs/Getting_your_patch_in_the_tree#Getting_the_patch_reviewed

Note You need to log in before you can comment on or make changes to this bug.