1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[André Bargull [:anba]]

LoginManagerContextMenu.jsm creates an Intl.DateTimeFormat object when the module is instantiated [1]. Since LoginManagerContextMenu.jsm is already loaded when starting Firefox, this causes us to initialize the whole JavaScript Intl object and its constructor properties during start-up. This in turn leads to initializing the time zone data from ICU (bug 837961, bug 1303091), which is a bit time-consuming and therefore causes noticeable performance regressions in multiple Talos benchmarks (bug 1314354).

Do you see any problems with changing LoginManagerContextMenu.jsm to create the Intl.DateTimeFormat object only when needed? 

[1] https://dxr.mozilla.org/mozilla-central/rev/aa3e49299a3aa5cb0db570532e3df9e75d30c2d1/toolkit/components/passwordmgr/LoginManagerContextMenu.jsm#22-23

Comment 1

[Matthew N. [:MattN]]

Attachment: Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

Review commit: https://reviewboard.mozilla.org/r/106402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106402/

Comment 2

[Matthew N. [:MattN]]

A try push is queued with autoland via RB.

Comment 3

[Matthew N. [:MattN]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=23cdb21e6b838c2278aaddc1ccd1d33937f48dd5

Comment 4

[Matthew N. [:MattN]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=6e5f04db4c4a95ad509224b238fd3ad464281e6b&newProject=try&newRevision=23cdb21e6b838c2278aaddc1ccd1d33937f48dd5&framework=1&showOnlyImportant=0

Comment 5

[Matthew N. [:MattN]]

Can you look at the perfherder results? I see some wins but also some regressions…

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8829292 [details]
Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

https://reviewboard.mozilla.org/r/106402/#review107432

r=me on the code, but I have troubles getting a clear signal out of that perfherder resultset, too.

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:194
(Diff revision 1)
>    return Services.strings.
>           createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
>  });
> +
> +XPCOMUtils.defineLazyGetter(LoginManagerContextMenu, "dateAndTimeFormatter", function() {
> +  return this.dateAndTimeFormatter = new Intl.DateTimeFormat(undefined, {

This works but I haven't seen the "return this.... = ..." style done in lazyGetters so far. Is there a specific reason you're not just returning the new reference directly?

::: toolkit/components/passwordmgr/LoginManagerContextMenu.jsm:197
(Diff revision 1)
> +
> +XPCOMUtils.defineLazyGetter(LoginManagerContextMenu, "dateAndTimeFormatter", function() {
> +  return this.dateAndTimeFormatter = new Intl.DateTimeFormat(undefined, {
> +    day: "numeric",
> +    month: "short",
> +    year: "numeric"

Nit: You could add a trailing comma here.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Bug 1332610 - Create Intl.DateTimeFormat in LoginManagerContextMenu.jsm lazily.

Review commit: https://reviewboard.mozilla.org/r/106556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/106556/

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8829465 [details]
Bug 1332610 - Create Intl.DateTimeFormat in LoginManagerContextMenu.jsm lazily.

https://reviewboard.mozilla.org/r/106556/#review107620

This approach is inconsistent with the lazy `_stringBundle` getter so it's why I went with a different approach.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8829292 [details]
Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

https://reviewboard.mozilla.org/r/106402/#review107432

> This works but I haven't seen the "return this.... = ..." style done in lazyGetters so far. Is there a specific reason you're not just returning the new reference directly?

Ugh, yeah, sorry, that's a cut+paste mistake from initially doing the getter+delete approach. I don't think it would have affected the performance though unfortunately. My manual testing showed that what I had actually worked.

> Nit: You could add a trailing comma here.

True, it didn't have it before since it was on one line and I didn't think to check that.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8829292 [details]
Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/106402/diff/1-2/

Comment 11

[André Bargull [:anba]]

(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #5)
> Can you look at the perfherder results? I see some wins but also some
> regressions…

I think this could be just the effect of delaying the time zone data initialization. We probably need to remeasure the performance numbers after bug 1332604 has landed.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Are we good to Try remeasuring this now? Looks like bug 1332604 has landed. This bug is blocking an open Talos regression bug going back to Fx52, so it would be good to keep it moving along.

Comment 13

[Matthew N. [:MattN]]

Not sure why you're asking me as I'm not driving this investigation and I'm very busy… Anyone is free to measure my patch on Try whenever they want.

Here's a new try push on inbound: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8c8d77bf8160c0fdc57f9ae5a32d0f7e5274f559
Here's the base: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7538950ebcc99c5ee1435d3aebc239ebf84f1ced

Comparison: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=7538950ebcc99c5ee1435d3aebc239ebf84f1ced&newProject=try&newRevision=8c8d77bf8160c0fdc57f9ae5a32d0f7e5274f559&framework=1&showOnlyImportant=0

Comment 14

[Matthew N. [:MattN]]

André, can you look at the perfherder results and decide whether to land. I have no problem landing it even if it doesn't cause perf wins.

Comment 15

[André Bargull [:anba]]

Forwarding the NI to :jmaher because I'm not sure how to evaluate the perfherder numbers. For example the comparison page claims an 8% regression on tabpaint opt e10s, windows7-32, but when comparing it with the results from the last two weeks [1], I don't think there's any regression for tabpaint.

[1] https://treeherder.mozilla.org/perf.html#/graphs?series=%5Btry,3fa816ffa2efd0725909627d61f6a2a8cad2be5f,1,1%5D&series=%5Bmozilla-inbound,3fa816ffa2efd0725909627d61f6a2a8cad2be5f,0,1%5D&highlightedRevisions=7538950ebcc9&highlightedRevisions=8c8d77bf8160

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

I don't see this causing a real perf regression, just the baseline is hitting the low end of the normal range and the try push hitting the high end of the normal range.

Thanks for checking!

Comment 17

[Matthew N. [:MattN]]

I thought the point of this bug was to have a perf win to help cancel out bug 1314354 though?

Comment 18

[André Bargull [:anba]]

So, I think this change is good to go. Ignoring the tabpaint numbers, the comparison page shows 1-2% improvements for other tests. And combined with the other performance patches, we should see an improvement similar to the results in bug 1314354, comment 14.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5df20eaa8ef4
Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm. r=johannh

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5df20eaa8ef4

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 22

[André Bargull [:anba]]

It probably doesn't hurt to backport this change to Aurora/Beta, but I can't give an estimation whether or not we'll see the same Talos changes in Aurora/Beta, because other infrastructural changes to the Intl API aren't present in Aurora/Beta. (I assume the NI was redirected to me to evaluate how useful it is to backport the patch.)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8829292 [details]
Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

Approval Request Comment
[Feature/Bug causing the regression]: bug 837961
[User impact if declined]: perf regression
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: comment 18 suggests a moderate Talos win
[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?]: just delays initializing JavaScript Intl until it's needed
[String changes made/needed]: none

Comment 24

[Julien Cristau [:jcristau]]

Comment on attachment 8829292 [details]
Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

perf regression fix, aurora53+, beta52+

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/00ad109ba6a6

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5b4f6203e10a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/5b4f6203e10a

Comment 28

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Ryan's assessment on manual testing needs (Comment 23) and the fact that this fix has automated coverage.