Closed Bug 1333197 Opened 3 years ago Closed 3 years ago

DateTimeFormat causes synchronous disk IO for each compartment creation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- unaffected

People

(Reporter: kmag, Assigned: anba)

References

Details

Attachments

(2 files, 1 obsolete file)

This happens because we currently flush the timezone info cache for each compartment creation, and the DateTimeFormat prototype initialization code immediately reinitializes it in order to read the default timezone.

According to talos profiling data, this causes a serious performance hit, especially when we attach content script sandboxes to content pages.
Comment on attachment 8829597 [details]
Bug 1333197: Lazily initialize default timezone in DateTimeFormat prototype.

Forwarding review to anba who's done a lot of work in this area recently.

(I can't do that in mozreview because it tells me that it can't find the user. It tells me the same thing about myself, so it *might* be a problem with mozreview, not me or anba.)
Attachment #8829597 - Flags: review?(till) → review?(andrebargull)
This is basically the same as bug 1331473, which was rejected by Waldo in its current form. Our current plan is to land bug 1332604 to avoid loading any time zone data when initializing Intl.DateTimeFormat.prototype. And bug 1332610, so we no longer create a Intl.DateTimeFormat object when starting Firefox.
(In reply to André Bargull from comment #3)
> This is basically the same as bug 1331473, which was rejected by Waldo in
> its current form. Our current plan is to land bug 1332604 to avoid loading
> any time zone data when initializing Intl.DateTimeFormat.prototype.

Yes, I agree this is a dupe. Will bug 1332604 be upliftable? If not, can we go
with the simpler solution first, for the sake of uplift? I'm worried about the
performance cost of this for extensions that use content scripts.

> And bug 1332610, so we no longer create a Intl.DateTimeFormat object when
> starting Firefox.

That's good to know. That was my initial plan, but I gave up when I realized it was more work than I had time for.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1331473
(In reply to Kris Maglione [:kmag] from comment #4)
> (In reply to André Bargull from comment #3)
> > This is basically the same as bug 1331473, which was rejected by Waldo in
> > its current form. Our current plan is to land bug 1332604 to avoid loading
> > any time zone data when initializing Intl.DateTimeFormat.prototype.
> 
> Yes, I agree this is a dupe. Will bug 1332604 be upliftable? If not, can we
> go
> with the simpler solution first, for the sake of uplift? I'm worried about
> the
> performance cost of this for extensions that use content scripts.

Hmm, I don't think I'd feel comfortable with uplifting bug 1332604, because
1. the current patch is based on bug 1328386, so we'd either need to uplift both and create a standalone version of bug 1332604
2. the specifications for these changes isn't yet finalized (bug 1332604 has a PR on github.com/tc39/ecma402; bug 1328386 only has an issue report, but not yet a PR). 

We could also consider to take this patch (or the one in bug 1331473) for aurora/beta, so the performance is improved for the next two releases, but not for central. In central, we're instead working with the alternative plan to land bug 1332604 and bug 1332610. 

Waldo, how do you feel about this approach?
Flags: needinfo?(jwalden+bmo)
Comment on attachment 8829597 [details]
Bug 1333197: Lazily initialize default timezone in DateTimeFormat prototype.

Just clearing review for now.
Attachment #8829597 - Flags: review?(andrebargull)
I would kind of prefer if we wrote a fresh backport patch, that disentangled itself from bug 1328386.  Shouldn't be all that hard.  But for a few weeks, we could probably also take this amount of laziness if we had to.
Flags: needinfo?(jwalden+bmo)
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #7)
> I would kind of prefer if we wrote a fresh backport patch, that disentangled
> itself from bug 1328386. 

I don't quite get which bug you refer to when you say "a fresh backport patch". Do you mean this one (bug 1333197) or bug 1331473 or a completely different one? (bug 1333197 and bug 1331473 don't depend on bug 1328386.)

> But for a few weeks, we could probably also take this amount of laziness if we had to.

And I'm also lost here, because I don't understand what "this amount of laziness" refers to. :-(
It's been a week, and my memory is surprisingly more hazy than it should be.  :-\  Hmm.

I think my *first* preference was for doing a super-simple backport of making Intl.DateTimeFormat.prototype not be a DateTimeFormat object.  Disentangled from all the symbol fallback nonsense that clearly isn't backportable, just the super-minimal bit of make the literal prototype object plain.

But my *second* preference, if that (probably) wasn't actually tenable because the compatibility change might possibly be risky (notwithstanding that this would be aligning with behavior Chrome has always shipped), was to (old-)spec-violate and just make DateTimeFormat.prototype's initialization of its gunky internals lazy in the way the patch here posed.  "This amount of laziness" wouldn't be my first choice in an ideal world.  But "probably" the first choice isn't actually tenable, or at least it's too risky for a backport.

Does that clarify things?
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #9)
> Does that clarify things?

Yes, thanks a lot! :-)

> But "probably" the first choice isn't actually tenable, or at least it's too risky for a backport.

For a backport I prefer this patch which delays calling DefaultTimeZone() until resolveDateTimeFormatInternals() is called, because it minimizes the user-visible effects. I think I even prefer the patch attached to this bug over the patch at bug 1331473, because it contains fewer changes, which reduces the risks when applying it to Aurora and Beta.
(In reply to Kris Maglione [:kmag] from comment #0)
> This happens because we currently flush the timezone info cache for each
> compartment creation, [...].

Flushing the time zone cache is marked as a hack in JSCompartment::init(), so it probably make sense to add a feature request to use OS-level time zone change observers (NSSystemTimeZoneDidChangeNotification on Mac, WM_TIMECHANGE on Windows, etc.). Is HAL the correct core component for this kind of feature request?
Blocks: 1314354
We have the new intl/locale/OSPreferences which we're using for things like system locales, and regional preferences related to intl (time format, first day of week, etc.)

We could get system timezone there as well.
This bug has more recent info than bug 1331473, so let's re-open this bug and mark 1331473 as a duplicate.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Duplicate of this bug: 1331473
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #12)
> We could get system timezone there as well.

JSAPI already has an embedder hook to reset the time zone, and there's even a call to it from the DOM side (https://dxr.mozilla.org/mozilla-central/source/dom/time/DateCacheCleaner.cpp), but the HAL implementation for Windows/Mac/Linux doesn't seem to implement any time zone notification observers. I think that's the missing piece to remove the JS::ResetTimeZone() call in JSCompartment::init().
Patch to lazily call DefaultTimeZone for Aurora.
Attachment #8829597 - Attachment is obsolete: true
Attachment #8837685 - Flags: review?(jwalden+bmo)
And the same path for Beta.
Attachment #8837689 - Flags: review?(jwalden+bmo)
ni jwalden to progress regression bug 1314354.
Flags: needinfo?(jwalden+bmo)
Attachment #8837685 - Flags: review?(jwalden+bmo) → review+
Attachment #8837689 - Flags: review?(jwalden+bmo) → review+
My understanding is that per bug 1314354 comment 20, Trunk/54 is unaffected for the purposes of this bug. Assuming that's the case, can we please get some approval requests here, André? Thanks!
Assignee: kmaglione+bmo → andrebargull
Status: REOPENED → ASSIGNED
Flags: needinfo?(jwalden+bmo) → needinfo?(andrebargull)
Comment on attachment 8837685 [details] [diff] [review]
bug1333197-aurora.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 837961

[User impact if declined]: Performance regressions per bug 1314354 and bug 1317681

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: No. Nightly uses a different approach (bug 1328386, bug 1332604) to fix the performance regression, but these changes aren't backportable, therefore a separate patch for Aurora/Beta is necessary. The patch introduces a slight ES2017 Intl (ECMA-402) specification violation, which is unlikely to make a difference in practice, because it's only observable when the user changes the OS time zone before the internal state of a previously created Intl.DateTimeFormat object is initialized. Nevertheless we don't want to introduce this specification violation to Nightly, because bug 1328386 and bug 1332604 make the changes from this bug obsolete. 

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No

[Why is the change risky/not risky?]: The patch only delays the call to retrieve the default OS time zone and it's restricted to JavaScript code. 

[String changes made/needed]: None
Flags: needinfo?(andrebargull)
Attachment #8837685 - Flags: approval-mozilla-aurora?
Comment on attachment 8837689 [details] [diff] [review]
bug1333197-beta.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 837961

[User impact if declined]: Performance regressions per bug 1314354 and bug 1317681

[Is this code covered by automated tests?]: Yes

[Has the fix been verified in Nightly?]: No. Nightly uses a different approach (bug 1328386, bug 1332604) to fix the performance regression, but these changes aren't backportable, therefore a separate patch for Aurora/Beta is necessary. The patch introduces a slight ES2017 Intl (ECMA-402) specification violation, which is unlikely to make a difference in practice, because it's only observable when the user changes the OS time zone before the internal state of a previously created Intl.DateTimeFormat object is initialized. Nevertheless we don't want to introduce this specification violation to Nightly, because bug 1328386 and bug 1332604 make the changes from this bug obsolete. 

[Needs manual test from QE? If yes, steps to reproduce]: No

[List of other uplifts needed for the feature/fix]: None

[Is the change risky?]: No

[Why is the change risky/not risky?]: The patch only delays the call to retrieve the default OS time zone and it's restricted to JavaScript code. 

[String changes made/needed]: None
Attachment #8837689 - Flags: approval-mozilla-beta?
Comment on attachment 8837689 [details] [diff] [review]
bug1333197-beta.patch

lazy default timezone lookup to avoid perf regression, beta52+
Attachment #8837689 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8837689 [details] [diff] [review]
bug1333197-beta.patch

Sounds like this should also go to aurora.
Attachment #8837689 - Flags: approval-mozilla-aurora+
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Thanks!
Comment on attachment 8837685 [details] [diff] [review]
bug1333197-aurora.patch

Aurora-specific patch to fix perf issues.
Attachment #8837685 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting qe-verify- based on André`s assessment on manual testing needs (see Comment 21) and the fact that this fix has automated coverage.
Flags: qe-verify-
Filed bug 1343826 to request system time zone change observer support on all platforms.
You need to log in before you can comment on or make changes to this bug.