Try to avoid intl_availableCollations when initializing an Intl.Collator

RESOLVED FIXED in Firefox 55

Status

()

Core
JavaScript: Internationalization API
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: André Bargull, Assigned: André Bargull)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

a month ago
intl_availableCollations showed up in bug 1365361 with a rather high ticks count.  And since it's not too difficult to avoid calling intl_availableCollations for the default collator, we should try to improve this code.
(Assignee)

Comment 1

a month ago
Created attachment 8868650 [details] [diff] [review]
bug1365650.patch

This patch adds a "default" entry to the locale data object for each Intl constructor. This default entry is used when neither a Unicode extension subtag nor an explicit option for an extension key was provided. This allows us to avoid calling some ICU methods (e.g. when using intl_defaultCalendar instead of intl_availableCalendars, we only need to retrieve the default calendar for a locale instead of all supported calendars for a locale). And for Intl.Collator, we can even avoid calling into ICU completely. 

Intl.DateTimeFormat improves from 700 ms to 650 ms in this µ-benchmark:
    var t = Date.now();
    for (var i = 0; i < 2000; ++i) { // Only 2000 iterations because intl_patternForSkeleton is slow.
        new Intl.DateTimeFormat().resolvedOptions();
    }
    print(Date.now() - t);

Intl.NumberFormat improves from 830 ms to 770 ms in
    var t = Date.now();
    for (var i = 0; i < 100000; ++i) {
        new Intl.NumberFormat().resolvedOptions();
    }
    print(Date.now() - t);

And Intl.Collator improves from 1100 ms to 460 ms in
    var t = Date.now();
    for (var i = 0; i < 100000; ++i) {
        new Intl.Collator().resolvedOptions();
    }
    print(Date.now() - t);


I'm not really sure who's going to review Intl patches as long as Waldo isn't there, so feel free to reassign this review if anyone "volunteered" to take Intl reviews!
Attachment #8868650 - Flags: review?(shu)

Comment 2

a month ago
I certainly don't know how to review this. What specs do I need to read here?

Comment 3

a month ago
(In reply to Shu-yu Guo [:shu] from comment #2)
> I certainly don't know how to review this. What specs do I need to read here?

Zibi filled me in on the API surface, and I think I have enough info now to review.

Comment 4

a month ago
Comment on attachment 8868650 [details] [diff] [review]
bug1365650.patch

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

This is more of a rubberstamp than an r+ given that I still lack a lot of knowledge of the Intl API. Most of the patch is just a refactoring: making locale data thunks instead of eagerly computed. That all seems fine to me.

::: js/src/builtin/Intl.cpp
@@ +2798,5 @@
> +
> +    // ICU returns old-style keyword values; map them to BCP 47 equivalents
> +    JSString* jscalendar = JS_NewStringCopyZ(cx, uloc_toUnicodeLocaleType("ca", calendar));
> +    if (!jscalendar)
> +        return false;

anba, since you told me this is the same code that's also in intl_availableCalendars, please refactor it so that it's obvious that it must be the same code.

I trust that this legacy -> BCP47 conversion is correct (in both places), and we'd want them to be synced up.

::: js/src/builtin/Intl.js
@@ +1564,5 @@
>          } else {
>              // Step 21.b.
> +            // In theory the default sensitivity is locale dependent;
> +            // in reality the CLDR/ICU default strength is always tertiary.
> +            s = "variant";

I see that this is manually inlined from collatorSearchLocaleData below.

Could collapse the |if (collatorIsSorting)| since both branches will always set |s = "variant"|.
Attachment #8868650 - Flags: review?(shu) → review+
(Assignee)

Comment 5

a month ago
Created attachment 8870008 [details] [diff] [review]
bug1365650.patch

Updated per review comments, carrying r+ from shu.
Attachment #8868650 - Attachment is obsolete: true
Attachment #8870008 - Flags: review+
(Assignee)

Comment 6

a month ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70c160680e6c3f5f681f53e9d52a74d14dab48ba
Keywords: checkin-needed

Comment 7

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/357a1c414efa
Improve ResolveLocale performance when initializing the default Intl objects. r=shu
Keywords: checkin-needed
Backed out for rooting hazard:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd30cdc0b8c848a1d94bf79822df138363b0f6d2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=357a1c414efa56cd6d03c0019cca5c028a498386&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=100935704&repo=mozilla-inbound

From hazards.txt:
Time: Mon May 22 2017 15:27:26 GMT+0000 (UTC)

Function '_ZL15DefaultCalendarP9JSContextRK16JSAutoByteString$Intl.cpp:JSString* DefaultCalendar(JSContext*, JSAutoByteString*)' has unrooted '<returnvalue>' of type 'JSString*' live across GC call '_ZN15ScopedICUObjectIPvXadL_Z13ucal_close_58EEED1Ev$ScopedICUObject<T, Delete>::~ScopedICUObject() [with T = void*; void (* Delete)(T*) = ucal_close_58]' at js/src/builtin/Intl.cpp:2692
    Intl.cpp:2692: Call(12,13, return := JS_NewStringCopyZ(cx*,__temp_3*))
    Intl.cpp:2692: Call(13,14, closeCalendar.~ScopedICUObject()) [[GC call]]
    Intl.cpp:2693:  [[end of function]]
GC Function: _ZN15ScopedICUObjectIPvXadL_Z13ucal_close_58EEED1Ev$ScopedICUObject<T, Delete>::~ScopedICUObject() [with T = void*; void (* Delete)(T*) = ucal_close_58]
    ScopedICUObject<T, Delete>::~ScopedICUObject() [with T = void*; void (* Delete)(T*) = ucal_close_58]
    ucal_close_58
    void icu_58::BucketList::~BucketList()
    static void icu_58::UMemory::operator delete(void*)
    uprv_free_58
    IndirectCall: cmemory.c:pFree
Flags: needinfo?(andrebargull)
(Assignee)

Comment 9

a month ago
Created attachment 8870430 [details] [diff] [review]
bug1365650.patch

Change DefaultCalendar to use an out-param, so the indirect pointer call from ICU's memory management code doesn't confuse the hazard analysis. Carrying r+ from shu.
Attachment #8870008 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8870430 - Flags: review+
(Assignee)

Comment 10

a month ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c51d847cb0f8e9147025afe6d40a209ae4c9158
Keywords: checkin-needed

Comment 11

a month ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/01ed17608749
Improve ResolveLocale performance when initializing the default Intl objects. r=shu
Keywords: checkin-needed

Comment 12

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01ed17608749
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.