Closed
Bug 1331473
Opened 8 years ago
Closed 8 years ago
Get the default time zone when resolving the DateTimeFormat properties
Categories
(Core :: JavaScript: Internationalization API, defect)
Core
JavaScript: Internationalization API
Tracking
()
RESOLVED
DUPLICATE
of bug 1333197
Tracking | Status | |
---|---|---|
firefox53 | --- | affected |
People
(Reporter: anba, Assigned: anba)
References
Details
Attachments
(1 file)
1.85 KB,
patch
|
Details | Diff | Splinter Review |
This should help for bug 1314354 and improves the following µ-benchmark by 10-15%:
---
var t = Date.now();
for (var i = 0; i < 100000; ++i) new Intl.DateTimeFormat();
print(Date.now() - t);
---
Open question: Is this spec-compliant? Consider the case when the time zone is changed between calls to InitializeDateTimeFormat and resolveDateTimeFormatInternals. 6.4.3 DefaultTimeZone() [1] is specified to return "the host environment’s current time zone", but strictly speaking we're already not spec-compliant because of the deferred initialization of the Intl object and its constructors. So adding another case where we defer calling DefaultTimeZone() may be acceptable.
The alternative is too save the unchecked result of intl_defaultTimeZone() and later compute the actual time zone from the original return value of intl_defaultTimeZone().
This should help bug 1314354, because the Intl object is already initialized during start-up (by
LoginManagerContextMenu.jsm [2]), which means Intl.DateTimeFormat.prototype gets initialized, which in turn leads to executing js::SharedIntlData::ensureTimeZones. And js::SharedIntlData::ensureTimeZones requires us to load the time zone names from ICU.
[1] https://tc39.github.io/ecma402/#sec-defaulttimezone
[2] https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/toolkit/components/passwordmgr/LoginManagerContextMenu.jsm#22-23
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8827421 -
Flags: review?(jwalden+bmo)
Comment 2•8 years ago
|
||
Comment on attachment 8827421 [details] [diff] [review]
bug1331473.patch
Review of attachment 8827421 [details] [diff] [review]:
-----------------------------------------------------------------
I think I prefer the alternative described in comment 0.
As to the prototypes being lazy-created, I wonder if they could be made into plain objects now. Probably worth a try fixing that and updating the spec, too.
Attachment #8827421 -
Flags: review?(jwalden+bmo)
Comment 3•8 years ago
|
||
LoginManagerContextMenu.jsm could probably stand to lazily create its Intl.DateTimeFormat object, too. No good reason it has to occur during startup.
Assignee | ||
Comment 4•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> LoginManagerContextMenu.jsm could probably stand to lazily create its
> Intl.DateTimeFormat object, too. No good reason it has to occur during
> startup.
I think fixing the startup performance of Intl should be our job, but OTOH creating a single Intl.DateTimeFormat object in LoginManagerContextMenu.jsm also means they keep using the old time zone even when the user has selected a new time zone.
Assignee | ||
Comment 5•8 years ago
|
||
Filed bug 1332610 for LoginManagerContextMenu.jsm
Assignee | ||
Comment 6•8 years ago
|
||
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> I think I prefer the alternative described in comment 0.
To make this work, I needed to call both intl_defaultTimeZone() and intl_defaultTimeZoneOffset() in InitializeDateTimeFormat() (otherwise it's not possible to reconstruct the default time zone). And having two JS->C++ calls instead of one obviously leads to a performance degradation for the test case in comment #0. To reduce the performance loss I had to modify ICU to speed-up ucal_getDefaultTimeZone and I had to create a new ICU method to retrieve the default time zone offset (so we don't need to create a UCalendar object in intl_defaultTimeZoneOffset). And at that point I stopped, because I think these changes exceed the complexity budget we should spent here.
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4999d0f84b57
Part 2: Remove the undefined check in Intl object finalizers which was only needed for Intl prototypes. r=Waldo
Assignee | ||
Comment 9•8 years ago
|
||
(In reply to Pulsebot from comment #8)
> Pushed by cbook@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4999d0f84b57
> Part 2: Remove the undefined check in Intl object finalizers which was only
> needed for Intl prototypes. r=Waldo
Commit message fail by me. :-/
Comment 10•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 11•8 years ago
|
||
Reopening and resetting flags, commit message contains wrong bug number.
Status: RESOLVED → REOPENED
status-firefox54:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
Updated•8 years ago
|
Status: REOPENED → NEW
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → DUPLICATE
Assignee | ||
Comment 13•8 years ago
|
||
(In reply to André Bargull from comment #6)
> (In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #2)
> > I think I prefer the alternative described in comment 0.
>
> To make this work, I needed to call both intl_defaultTimeZone() and
> intl_defaultTimeZoneOffset() in InitializeDateTimeFormat() (otherwise it's
> not possible to reconstruct the default time zone).
And the call to intl_defaultTimeZone() is too expensive according to bug 1333197, comment #0, so we need to avoid calling it, which is another reason the alternative idea doesn't work out.
You need to log in
before you can comment on or make changes to this bug.
Description
•