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

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
Password Manager
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: anba, Assigned: MattN)

Tracking

Trunk
mozilla54
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox52 fixed, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 months ago
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 hidden (mozreview-request)
A try push is queued with autoland via RB.
Assignee: nobody → MattN+bmo
Status: NEW → ASSIGNED
status-firefox52: --- → affected
https://treeherder.mozilla.org/#/jobs?repo=try&revision=23cdb21e6b838c2278aaddc1ccd1d33937f48dd5
https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=6e5f04db4c4a95ad509224b238fd3ad464281e6b&newProject=try&newRevision=23cdb21e6b838c2278aaddc1ccd1d33937f48dd5&framework=1&showOnlyImportant=0
Can you look at the perfherder results? I see some wins but also some regressions…
Flags: needinfo?(andrebargull)

Comment 6

11 months ago
mozreview-review
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.
Attachment #8829292 - Flags: review?(jhofmann) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

11 months ago
mozreview-review
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.
Attachment #8829465 - Flags: review?(MattN+bmo) → review-
(Assignee)

Comment 9

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Attachment #8829465 - Attachment is obsolete: true
(Reporter)

Comment 11

11 months ago
(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.
Flags: needinfo?(andrebargull)
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.
Flags: needinfo?(MattN+bmo)
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
Flags: needinfo?(MattN+bmo)
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.
Flags: needinfo?(andrebargull)
(Reporter)

Comment 15

10 months ago
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
Flags: needinfo?(andrebargull) → needinfo?(jmaher)
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!
Flags: needinfo?(jmaher)
I thought the point of this bug was to have a perf win to help cancel out bug 1314354 though?
(Reporter)

Comment 18

10 months ago
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

10 months ago
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/autoland/rev/5df20eaa8ef4
Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm. r=johannh

Comment 20

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5df20eaa8ef4
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please request Aurora/Beta approval on this when you get a chance.
Flags: needinfo?(MattN+bmo)
Flags: needinfo?(MattN+bmo) → needinfo?(andrebargull)
(Reporter)

Comment 22

10 months ago
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.)
Flags: needinfo?(andrebargull)
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
Attachment #8829292 - Flags: approval-mozilla-beta?
Attachment #8829292 - Flags: approval-mozilla-aurora?
Comment on attachment 8829292 [details]
Bug 1332610 - Don't eagerly create Intl.DateTimeFormat in LoginManagerContextMenu.jsm.

perf regression fix, aurora53+, beta52+
Attachment #8829292 - Flags: approval-mozilla-beta?
Attachment #8829292 - Flags: approval-mozilla-beta+
Attachment #8829292 - Flags: approval-mozilla-aurora?
Attachment #8829292 - Flags: approval-mozilla-aurora+

Comment 25

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/00ad109ba6a6
status-firefox53: affected → fixed

Comment 26

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/5b4f6203e10a
status-firefox52: affected → fixed

Comment 27

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/5b4f6203e10a
status-firefox-esr52: --- → fixed
Setting qe-verify- based on Ryan's assessment on manual testing needs (Comment 23) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.