Get the default time zone when resolving the DateTimeFormat properties

RESOLVED DUPLICATE of bug 1333197

Status

()

RESOLVED DUPLICATE of bug 1333197
2 years ago
2 years ago

People

(Reporter: anba, Assigned: anba)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox53 affected)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8827421 [details] [diff] [review]
bug1331473.patch
Attachment #8827421 - Flags: review?(jwalden+bmo)
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)
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

2 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

2 years ago
Filed bug 1332610 for LoginManagerContextMenu.jsm
(Assignee)

Comment 6

2 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.
Duplicate of this bug: 1333197

Comment 8

2 years ago
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

2 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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4999d0f84b57
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 11

2 years ago
Reopening and resetting flags, commit message contains wrong bug number.
Status: RESOLVED → REOPENED
status-firefox54: fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla54 → ---
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1333197
(Assignee)

Comment 13

2 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.