make nsIUpdateTimerManager support idle dispatch

ASSIGNED
Assigned to

Status

()

Toolkit
General
P3
normal
ASSIGNED
7 months ago
a month ago

People

(Reporter: Away for a while, Assigned: rhelmer)

Tracking

({perf})

unspecified
Points:
---

Firefox Tracking Flags

(firefox57 fix-optional)

Details

(Whiteboard: [qf:ip60][qf:p1])

(Reporter)

Updated

7 months ago
Whiteboard: [qf]
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?)
Now that we have bug 1353206, is it possible to fix this? (I hear you were discussing options with Ehsan, hence the needinfo :)
Flags: needinfo?(rhelmer)
Whiteboard: [qf] → [qf:p1]
(Assignee)

Comment 3

7 months ago
(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.
Flags: needinfo?(rhelmer)
(Assignee)

Updated

7 months ago
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
(Assignee)

Comment 4

7 months ago
(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...)
Flags: needinfo?(rjesup)
> 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.
Flags: needinfo?(rjesup)
(Assignee)

Comment 6

7 months ago
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.
Component: Add-ons Manager → General
Summary: Add-on manager updater runs off of a timer → make nsIUpdateTimerManager support idle dispatch
(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

6 months ago
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.
(Assignee)

Comment 9

5 months ago
(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.

Updated

4 months ago
Whiteboard: [qf:p1] → [qf:p2]
status-firefox57: --- → fix-optional
Priority: -- → P3
Whiteboard: [qf:p2] → [qf:p1][qf:ip60]

Updated

a month ago
Whiteboard: [qf:p1][qf:ip60] → [qf:ip60][qf:p1]
You need to log in before you can comment on or make changes to this bug.