Closed Bug 1370194 Opened 3 years ago Closed 3 years ago

Directly use arrays in some Intl js-functions

Categories

(Core :: JavaScript: Internationalization API, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file)

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.
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.
(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.
Attached patch bug1370194.patchSplinter Review
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);
Attachment #8874396 - Flags: review?(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.
Attachment #8874396 - Flags: review?(till) → review+
(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! :-)
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d912b21f2ae6
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.