1151750 - Make it easier to find out why the AddonWatcher displays an add-on as slowing down Firefox

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

For the moment, the only way to find out why the AddonWatcher has flagged an add-on as slow is to look at Telemetry. The self-support UX will certainly eventually provide something intended for end-users, but we need something for add-on developers.

We should add the information in about:performance.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recapitulating alerts in about:performance

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Screen Shot 2015-04-07 at 14.24.24.png

For reference, this is what it looks like.

Comment 3

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Comment on attachment 8589061 [details] [diff] [review]
Recapitulating alerts in about:performance

Review of attachment 8589061 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +122,5 @@
> +      "Alerts",
> +      "Jank level alerts",
> +      "(highest jank)",
> +      "Cross-Process alerts",
> +      "(highest CPOW)"

Not necessary for landing but seems like you could construct this from the MEASURES constant above.

@@ +153,5 @@
> +      // Display the name of the add-on
> +      let elName = document.createElement("td");
> +      elAddon.appendChild(elName);
> +      AddonManager.getAddonByID(addonId, a => {
> +        elName.textContent = a?a.name:addonId

Nit: spaces around operators

@@ +155,5 @@
> +      elAddon.appendChild(elName);
> +      AddonManager.getAddonByID(addonId, a => {
> +        elName.textContent = a?a.name:addonId
> +      });
> +      

Nit: whitespace

::: toolkit/modules/AddonWatcher.jsm
@@ +183,1 @@
>          // Report mibehaviors to the user.

Typo: misbehaviors

@@ +185,3 @@
>          let reason = null;
>  
> +        for (let k of FILTERS) {

s/k/filter/

@@ +187,5 @@
> +        for (let k of FILTERS) {
> +          let peak = stats.peaks[k] || 0;
> +          stats.peaks[k] = Math.max(diff[k], peak);
> +
> +          if (limits[k] < 0 || diff[k] <= limits[k]) {

limits[k] <= 0 please or setting the prefs to 0 won't disable checking.

@@ +193,2 @@
>            }
> +          

Nit: whitespace

Comment 4

[David Teller [:Yoric] - still alive but not very active]

(In reply to Dave Townsend [:mossop] from comment #3)
> @@ +187,5 @@
> > +        for (let k of FILTERS) {
> > +          let peak = stats.peaks[k] || 0;
> > +          stats.peaks[k] = Math.max(diff[k], peak);
> > +
> > +          if (limits[k] < 0 || diff[k] <= limits[k]) {
> 
> limits[k] <= 0 please or setting the prefs to 0 won't disable checking.

Well, so far, we used -1 for disabling. Do you want me to change this?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recapitulating alerts in about:performance, v2

Applied feedback.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3d558695b31

Comment 6

[Dave Townsend [:mossop] (PTO until Aug 21st)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #4)
> (In reply to Dave Townsend [:mossop] from comment #3)
> > @@ +187,5 @@
> > > +        for (let k of FILTERS) {
> > > +          let peak = stats.peaks[k] || 0;
> > > +          stats.peaks[k] = Math.max(diff[k], peak);
> > > +
> > > +          if (limits[k] < 0 || diff[k] <= limits[k]) {
> > 
> > limits[k] <= 0 please or setting the prefs to 0 won't disable checking.
> 
> Well, so far, we used -1 for disabling. Do you want me to change this?

The previous code ignored limits <= 0 and I don't see a reason to change that.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

My bad.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recapitulating alerts in about:performance, v3

Applied final feedback.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3d558695b31

Comment 10

[David Teller [:Yoric] - still alive but not very active]

So, on Try, the MacOS X 10.8 build timeouts silently during linking, for no reason that I can understand. Since I have strictly no issue locally on MacOS X and other platforms pass all tests, I'll suspect an unrelated issue (disk full?) and mark this checkin-needed.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/1786e8cb842e

Comment 12

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=2635525&repo=fx-team

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Oh, wrong patch :/

Comment 14

[Ryan VanderMeulen [:RyanVM]]

This wasn't at fault. Re-landed.
https://hg.mozilla.org/integration/fx-team/rev/886f4e8d4bc0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Except this is a real failure from this patch. Backed out again.
https://hg.mozilla.org/integration/fx-team/rev/b87c799bfb90

https://treeherder.mozilla.org/logviewer.html#?job_id=2641082&repo=fx-team

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Yes, as I wrote, wrong patch. My apologies.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: bz://1151750/Yoric

/r/7069 - Bug 1151750 - about:performance now recapitulates alerts;r=mossop
/r/7071 - Bug 1152759 - Regroup Performance Monitoring modules/components;r=yoric
/r/7073 - API WIP

Pull down these commits:

hg pull -r 516cbbd3805a58ffcc21feaf47c9ed66dfd29595 https://reviewboard-hg.mozilla.org/gecko/

Comment 18

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Comment on attachment 8592864 [details]
MozReview Request: bz://1151750/Yoric

Assuming this is all the same as the patch in bug 1154239

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Attachment: Recapitulating alerts in about:performance, v4

Rebased.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76d251f8f314

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f35b2802480c

Comment 21

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

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