1370194 - Directly use arrays in some Intl js-functions

Status: RESOLVED FIXED

(Core :: JavaScript: Internationalization API, enhancement)

Description

[André Bargull [:anba]]

Directly using arrays avoids extra allocations when we later convert the List objects to Array objects. And using Arrays may help inlining functions like Array.prototype.push which is currently invoked on List objects.

Comment 1

[Till Schneidereit [:till]]

Doesn't this risk observably changing the order of operations? In particular, using Array#push on an Array should be observable using element setters on Array.prototype.

Comment 2

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #1)
> Doesn't this risk observably changing the order of operations? In
> particular, using Array#push on an Array should be observable using element
> setters on Array.prototype.

Ups, what I meant to say is that we're currently using Array.p.push on List objects, which is not an Array, so it can't be inlined. So what I did is to replace the List with a normal Array and instead of Array.p.push, the code now uses _DefineDataProperty to avoid the prototype issue with inherited setters.

Comment 3

[André Bargull [:anba]]

Attachment: bug1370194.patch

Most language tags don't contain any extension (e.g. "-u-co-phonebk") or private use subtags ("-x-custom-tag"), so I modified CanonicalizeLanguageTag to directly return the normalized language tag in that case. Similar CanonicalizeLocaleList should most of the time be called with a single string argument, so we can skip the extra array allocations and directly call IsStructurallyValidLanguageTag and CanonicalizeLanguageTag.

Furthermore I've replaced some List objects with normal Array objects, so we can skip the List to Array conversion in LookupSupportedLocales and Intl_getCanonicalLocales


This µ-benchmark improves from 165ms to 120ms:

    var t = Date.now();
    for (var i = 0; i < 100000; ++i) {
        Intl.getCanonicalLocales("de-DE")
    }
    print(Date.now() - t);


This one improves from 450ms to 330ms:

    var t = Date.now();
    for (var i = 0; i < 100000; ++i) {
        Intl.Collator.supportedLocalesOf("de-DE")
    }
    print(Date.now() - t);

Comment 4

[Till Schneidereit [:till]]

Comment on attachment 8874396 [details] [diff] [review]
bug1370194.patch

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

This isn't only faster but also cleaner. In a way that seems more important, as I have a hard time believing that Intl.getCanonicalLocales and Intl.Collator.supportedLocalesOf are really hot in real-world code. Because of that, r=me with or without the comment below addressed - it really doesn't matter.

::: js/src/builtin/Intl.js
@@ +1201,5 @@
>  
>          // Step 4.c-d.
>          var availableLocale = BestAvailableLocale(availableLocales, noExtensionsLocale);
>          if (availableLocale !== undefined)
> +            _DefineDataProperty(subset, subset.length, locale);

It probably doesn't matter, but getting subset.length once before the loop and then incrementing it locally would probably make this ever so slightly faster, still.

Comment 5

[André Bargull [:anba]]

(In reply to Till Schneidereit [:till] from comment #4)
> This isn't only faster but also cleaner. In a way that seems more important,
> as I have a hard time believing that Intl.getCanonicalLocales and
> Intl.Collator.supportedLocalesOf are really hot in real-world code. Because
> of that, r=me with or without the comment below addressed - it really
> doesn't matter.

I think I stick with the current patch, because most code in Intl.js currently doesn't care about multiple accesses to the .length property. (The next big performance improvements in this code probably require rewriting the BCP47 language tag parsing, so we don't need to execute RegExps and, when moving to native code, we don't need to have per-compartment clones of the data in IntlData.js. But that's work for another day! :-)

Comment 6

[André Bargull [:anba]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d912b21f2ae6
Prefer to use Array objects when performing array operations in Intl self-hosted code. r=till

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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