Kris Maglione [:kmag] Reporter
	Description • 8 years ago

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.

	Till Schneidereit [:till]
	Comment 2 • 8 years ago

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

	André Bargull [:anba] Assignee
	Comment 3 • 8 years ago

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.

	Kris Maglione [:kmag] Reporter
	Comment 4 • 8 years ago

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

	André Bargull [:anba] Assignee
	Comment 5 • 8 years ago

(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?

	André Bargull [:anba] Assignee
	Comment 6 • 8 years ago

Comment on attachment 8829597 [details]
Bug 1333197: Lazily initialize default timezone in DateTimeFormat prototype.

Just clearing review for now.

	Jeff Walden [:Waldo]
	Comment 7 • 8 years ago

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.

	André Bargull [:anba] Assignee
	Comment 8 • 8 years ago

(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. :-(

	Jeff Walden [:Waldo]
	Comment 9 • 8 years ago

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?

	André Bargull [:anba] Assignee
	Comment 10 • 8 years ago

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

	André Bargull [:anba] Assignee
	Comment 11 • 8 years ago

(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?

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 12 • 8 years ago

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.

	André Bargull [:anba] Assignee
	Comment 13 • 8 years ago

This bug has more recent info than bug 1331473, so let's re-open this bug and mark 1331473 as a duplicate.

	André Bargull [:anba] Assignee
	Comment 15 • 8 years ago

(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().

	André Bargull [:anba] Assignee
	Comment 16 • 8 years ago

Patch to lazily call DefaultTimeZone for Aurora.

	André Bargull [:anba] Assignee
	Comment 17 • 8 years ago

And the same path for Beta.

	Astley Chen (inactive)
	Comment 18 • 8 years ago

ni jwalden to progress regression bug 1314354.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 8 years ago

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!

	André Bargull [:anba] Assignee
	Comment 20 • 8 years ago

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

	André Bargull [:anba] Assignee
	Comment 21 • 8 years ago

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

	Julien Cristau [:jcristau]
	Comment 22 • 8 years ago

Comment on attachment 8837689 [details] [diff] [review]
bug1333197-beta.patch

lazy default timezone lookup to avoid perf regression, beta52+

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/094a86d66f76

	Julien Cristau [:jcristau]
	Comment 24 • 8 years ago

Comment on attachment 8837689 [details] [diff] [review]
bug1333197-beta.patch

Sounds like this should also go to aurora.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/70eb26b3bee4

	Kris Maglione [:kmag] Reporter
	Comment 26 • 8 years ago

Thanks!

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-esr52/rev/094a86d66f76

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 8 years ago

Comment on attachment 8837685 [details] [diff] [review]
bug1333197-aurora.patch

Aurora-specific patch to fix perf issues.

	Andrei Vaida [:avaida]
	Comment 29 • 8 years ago

Setting qe-verify- based on André`s assessment on manual testing needs (see Comment 21) and the fact that this fix has automated coverage.

	André Bargull [:anba] Assignee
	Comment 30 • 7 years ago

Filed bug 1343826 to request system time zone change observer support on all platforms.