1373408 - make nsIUpdateTimerManager support idle dispatch

Status: RESOLVED FIXED

(Toolkit :: General, enhancement, P3)

Description

[(no longer active)]

https://searchfox.org/mozilla-central/rev/20d16dadd336e0c6b97e3e19dc4ff907744b5734/toolkit/mozapps/extensions/addonManager.js#135

It needs to be ported to use idle dispatch instead.

See this profile, for example: <https://perf-html.io/public/69f88e5a6c6ef339dfaa0442932e075f690f1bea/calltree/?implementation=js&range=1.4816_2.4332~1.7767_2.1714&thread=0>

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Copied from the bug where I noticed this:
> Also interesting is the amount of time on MainThread all the cert checks take.  Thank you, NSS.
> thoughts?  [snip]  And should we file a bug on NSS and update manager/etc to see if we can move some of this stuff off mainthread?  (or is there one?)

Comment 2

[Andrew Overholt [:overholt]]

Now that we have bug 1353206, is it possible to fix this? (I hear you were discussing options with Ehsan, hence the needinfo :)

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Andrew Overholt [:overholt] from comment #2)
> Now that we have bug 1353206, is it possible to fix this? (I hear you were
> discussing options with Ehsan, hence the needinfo :)

I think so... the addons (which incl. blocklist, gmp, addons) and also the app update timer use nsIUpdateTimerManager, so it probably makes the most sense to have this use requestIdleCallback somehow.

I have ideas for moving update timers out of the main process but the above is the most expedient.

Comment 4

[Robert Helmer [:rhelmer]]

(In reply to Randell Jesup [:jesup] from comment #1)
> Copied from the bug where I noticed this:
> > Also interesting is the amount of time on MainThread all the cert checks take.  Thank you, NSS.
> > thoughts?  [snip]  And should we file a bug on NSS and update manager/etc to see if we can move some of this stuff off mainthread?  (or is there one?)

I don't think there is a bug for this (yet!). For add-on updates, the addons manager code does its own signature verification from JS in XPIInstall.jsm for XPI (JAR) files and directories using nsIX509CertDB.

From a quick look at the app update code I *think* the signature verification of the content itself (the MAR) is done by the standalone updater binary and not from JS, so I wouldn't expect that issue there.

Was this with a clean profile or is it one you have add-ons installed in?

What I suspect you're seeing isn't specific to updates, but the signature verification of existing add-ons that happens shortly after startup (which happens this way to avoid impacting startup time...)

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

> Was this with a clean profile or is it one you have add-ons installed in?

These are clean profiles from QFlow profiling, or should be
 
> What I suspect you're seeing isn't specific to updates, but the signature
> verification of existing add-ons that happens shortly after startup (which
> happens this way to avoid impacting startup time...)

Very likely yes, and profiling procedure is typically "start the browser.  Do X.  Shut it down, Start it again, start profiling, do X" so that would trigger this.

Comment 6

[Robert Helmer [:rhelmer]]

The profile in this bug seems to be all about blocklist check... waiting until the user is idle makes sense in any case, but just looking at the code in this stack we can make some improvements here. That can happen in a separate bug though.

Adjusting the summary slightly so it's clearer what needs to change here - technically a bunch of things run off the add-ons timer, but it's not the only timer, so I think we should implement this in the underlying update timer manager.

Comment 7

[Kris Maglione [:kmag]]

(In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from comment #0)
> See this profile, for example:
> <https://perf-html.io/public/69f88e5a6c6ef339dfaa0442932e075f690f1bea/
> calltree/?implementation=js&range=1.4816_2.4332~1.7767_2.1714&thread=0>

Hm. So leaving aside the idle dispatch question, there are a few things going on in this profile that need to be fixed:

1) The blocklist status check is obscenely expensive, and we do it every time we access the blocklistState getter, which we do *twice* in the escapeAddonURI function for each add-on. That adds up to 85ms per update check. We should only ever need to check the blocklist when an add-on is installed or the blocklist is updated, and store the status in the DB.

2) We call string.replace 12 times in that function, which adds up to another 18ms. 10 of those calls could be replaced with a single `url.replace(/%([A-Z_]+)%/g, (m0, m1) => replacements[m1] || m0)` call. And the 11th could be folded into that for the custom category substitutions.

3) The isCompatible getter likewise doesn't need to compute compatibility for every call. It only need to be computed on add-on or app update. But at least this one is only 3ms.

Comment 8

[Jean Gong :jgong]

My email update from rhelmer. He will try to get this bug fixed within Fx57 if it's a straightforward fix. We'll reassessment later in fx75 in early Sept.

Comment 9

[Robert Helmer [:rhelmer]]

(In reply to Kris Maglione [:kmag] from comment #7)
> (In reply to :Ehsan Akhgari (needinfo please, extremely long backlog) from
> comment #0)
> > See this profile, for example:
> > <https://perf-html.io/public/69f88e5a6c6ef339dfaa0442932e075f690f1bea/
> > calltree/?implementation=js&range=1.4816_2.4332~1.7767_2.1714&thread=0>
> 
> Hm. So leaving aside the idle dispatch question, there are a few things
> going on in this profile that need to be fixed:
> 
> 1) The blocklist status check is obscenely expensive, and we do it every
> time we access the blocklistState getter, which we do *twice* in the
> escapeAddonURI function for each add-on. That adds up to 85ms per update
> check. We should only ever need to check the blocklist when an add-on is
> installed or the blocklist is updated, and store the status in the DB.

This should be fixed now by bug 1377538.
 
> 2) We call string.replace 12 times in that function, which adds up to
> another 18ms. 10 of those calls could be replaced with a single
> `url.replace(/%([A-Z_]+)%/g, (m0, m1) => replacements[m1] || m0)` call. And
> the 11th could be folded into that for the custom category substitutions.

From looking at the code this appears to still be there - I'll file a separate bug for this.

> 3) The isCompatible getter likewise doesn't need to compute compatibility
> for every call. It only need to be computed on add-on or app update. But at
> least this one is only 3ms.

This needs a separate bug as well.

Comment 10

[:Gijs (he/him)]

Tentatively marking p2 - I guess it could be p1 but it'd need an active owner. :rhelmer, are you still working on this?

Comment 11

[Robert Helmer [:rhelmer]]

(In reply to :Gijs (he/him) from comment #10)
> Tentatively marking p2 - I guess it could be p1 but it'd need an active
> owner. :rhelmer, are you still working on this?

Keeps dropping off my radar, if you can't find someone let me know and I'll take it back, going to unassign for now to give someone else a chance.

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bug 1373408 - Make nsUpdateTimerManager notify callbacks on idle.

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

Comment 13

[Robert Helmer [:rhelmer]]

Comment on attachment 8991704 [details]
Bug 1373408 - Make nsUpdateTimerManager notify callbacks on idle.

https://reviewboard.mozilla.org/r/256648/#review263520

Well this change seems fine :) I expected this would break tests though, do we actually have tests for nsUpdateTimerManager?

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Thanks for the fast review!

We do have some tests at toolkit/components/timermanager/tests/unit/consumerNotifications.js. It wouldn't surprise me, however, if any tests that rely on nsUpdateTimerManager are waiting until the notification is called. So now they're waiting a little longer, but it's okay - it eventually fires. That's my guess anyhow - no major surprises on my try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b9dbb09a2a6097e4678f03a5a9783814f9ff1591 except for the Windows build failures (but those are unrelated).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b754717fc772
Make nsUpdateTimerManager notify callbacks on idle. r=rhelmer

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b754717fc772