Closed Bug 1309946 Opened 8 years ago Closed 8 years ago

Remove "this addon might be making Firefox run slowly" warning

Categories

(Toolkit :: Performance Monitoring, defect)

Product:
Toolkit
Component:
Performance Monitoring
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact ?
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: Dolske, Assigned: Gijs, NeedInfo)

References

Details

Attachments

(3 files, 1 obsolete file)

Screenshot
41.50 KB, image/png
Details
Bug 1309946 - disable addonwatcher for easy uplift,
59 bytes, text/x-review-board-request
Yoric
: review+
Details
Bug 1309946 - remove all traces of add-on performance monitoring,
59 bytes, text/x-review-board-request
Yoric
: review+
Details
Attached image Screenshot —
I just got a warning about "Multi-process staged rollout might be making Nightly run slowly". Which is a little silly, because it's a (1) a system addon that can't be disabled and (2) I've not observed any performance issue with Nightly. The most narrowly scoped fix here would be to disable this warning for system addons (feel free to spin that off), but I think the scope should be broader... I've seen this warning a number of other times, and it has never seemed useful to me. It's never warned about something that was actually causing a performance problem observable to me, and it's warned about addons that I'm simply not going to disable (like Adblock / uBlock Origin). That's my personal experience, but it makes me question if users are actually finding this useful, or if we're just annoying them with a warning when there isn't a problem or they're not likely to take any action. Is there any data/evidence that shows this warning is effective and useful? Quick straw poll of a few people on #fx-team seems to indicate other people think we should just remove this.
I haven't see any data that this is useful, but I'm going to ni? kev to see if he knows of any insights.
Flags: needinfo?(kev)
I have no problem with the general idea. The slow add-on watcher was initially meant to be used with self-support/shield to help troubleshooting misbehaving instances of Firefox. The current UX was... rushed out in a very imperfect state. Despite improvements, it's still pretty bad in terms of UX. Now, any evidence would be on Telemetry SLOW_ADDON_WARNING_STATES (https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2016-10-11&keys=__none__!__none__!__none__&max_channel_version=nightly%252F52&measure=SLOW_ADDON_WARNING_STATES&min_channel_version=null&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2016-09-19&table=0&trim=1&use_submission_date=0) Since the legend is barely readable, let me copy it here: « The states the Slow Add-on Warning goes through. 0: Displayed the warning. 1: User clicked on 'Disable add-on'. 2: User clicked 'Ignore add-on for now'. 3: User clicked 'Ignore add-on permanently'. 4: User closed notification. Other values are reserved for future uses. »
Based on those I would recommend we remove this.
I don't have time to patch this, but I can review a patch. The offending code lives in nsBrowserGlue.js: mainly in `_init()` and ` _notifySlowAddon`.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
I brought this up in a meeting and I think blassey was hoping someone could look at why this was happening and see if we could get it fixed up.
Flags: needinfo?(blassey.bugs)
Well it shouldn't be occuring for the "Multi-process staged rollout" add-on, so it could be that measurement is wrong.
We believe that the cause in the e10srollout add-on is the newly added code to read telemetry and check if the user is experiencing too many tab spinners. It does some file IO on a worker thread, but it appears that this detection also accidentally counts stuff happening on worker threads. And it does a JSON.parse on the main thread which is not good but necessary to read the data. But with the numbers that we saw about how many people used this warning to disable add-ons, I think removing it will be easier than fixing the two main bugs: "don't count work on other threads" and "don't notify on built-in system add-ons" (as for the e10srollout add-on, I'll have some improvements soon that should reduce the file IO and JSON parsing)
But regardless, I believe that real-time monitoring of slow add-ons is not a good idea. We want long-term numbers and we want to suggest getting rid of add-ons only if we have reasons to believe that they are repeat offenders across sessions. More precisely, we want to suggest removing add-ons only if we have reasons to believe that removing them will actually improve the situation. That's the kind of data-crunching that self-support/shield is meant to handle, not toolkit. The Performance Watcher is designed to provide the raw numbers, but these numbers are not sufficient to take decisions.
> But regardless, I believe that real-time monitoring of slow add-ons is not a good idea. Sorry, I meant real-time *warning* of slow add-ons.
8% of the time the user interacts with the dialog, they disable the flagged add-on. That sounds pretty useful to me. 12% of the time they decide the add-on is worth the slowdown and click ignore permanently. Flagging the system add-on is clearly broken, so let's fix that. This dialog has been useful to me. It kept flagging LastPass until I bit the bullet and disabled it. Now Firefox is much snappier. It also tends to flag mailvelope when I'm on gmail, which indicates to me that its working.
Flags: needinfo?(blassey.bugs)
Regardless of the general use of performance monitoring (and I definitely agree that monitoring is useful), pestering the user, as we currently do, is simply annoying. Brad, I believe that you're in the best position to either: 1/ get someone from UX to look at the current warning and design a better experience; or 2/ get the self-support/shield team to use this data and present it as part of that (hopefully well ux-designed) flow. I personally would prefer 2/ as this would let performance monitoring concentrate on monitoring and providing raw data and let the shield team deal with number-crunching and UX, which are both within their realm. If necessary, I can provide APIs for extracting the AddonWatcher's data across sessions.
Flags: needinfo?(blassey.bugs)
Flags: needinfo?(blassey.bugs) → needinfo?(mgrimes)
Seems like we should move that followup to its own bug, and remove this annoyance in the short term.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #14) > 8% of the time the user interacts with the dialog, they disable the flagged > add-on. That sounds pretty useful to me. Absent some indication that their experience is meaningfully improved and their happiness increases as a result of it, that stat doesn't indicate usefulness on its own.
I agree with dcamp. The data will be useful for building better heuristics for these types of notifications in the future, but that phase of the Shield project is likely Q1 2017 or beyond. I'd say we can remove the warning for now.
Flags: needinfo?(mgrimes)
Assignee: jaws → nobody
Status: ASSIGNED → NEW
Before making a final decision on this, it may be worth doing what Andy suggests in https://bugzilla.mozilla.org/show_bug.cgi?id=1329026#c4, since this could be turned into something useful for WebExtensions if it actually captures problems there much.
With Quantum DOM we have better information about the event queue that could inform about slow addons that actually impact input handling or smoothness (aka when it has perceived performance impact). This addon could also provide more insights if it had visuals as counterpart in about:performance. Since we need messaging for Quantum on improved performance, there is room to improve that page so we can use it to highlight the inner workings of improved performance.
That would be very nice. In this case, you should consider updating PerformanceWatcher.jsm (and underlying C++ implementation) to expose the improved Quantum data.
This came up again in today's Firefox desktop meeting, because people are seeing this for the test pilot add-on. This has now caused confusion or required whitelist additions for the landing of 2 separate system add-ons (bug 1308656, bug 1324062). As a result, there was broad consensus in the meeting that this should be disabled. There also seem to be known performance issues with the code itself (bug 1203544, bug 1136935), in addition to the other issues that were highlighted in this bug. From a cursory glance at the code, it actually seems like: 1) the notification bar UI is disabled everywhere except Nightly ( https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/browser/components/nsBrowserGlue.js#430-432 ) 2) the watching is supposed to be disabled entirely everywhere except Nightly ( https://dxr.mozilla.org/mozilla-release/rev/e81854d6ce91f3174774a50c9c5813c33b9aac58/modules/libpref/init/all.js#5308-5313 ) and especially on Talos ( https://dxr.mozilla.org/mozilla-central/rev/9577ddeaafd85554c2a855f385a87472a089d5c0/testing/talos/talos/config.py#62 ) 3) That pref (browser.addon-watch.interval) hasn't worked since Firefox 45 when bug 1186491 landed. As a result, we're incurring (potentially not-super-high, but definitely existant) performance penalties for *running* the code, but it's providing 0 benefit to users unless they proactively load about:performance themselves, which has no UI entry points apart from about:about (which itself has no UI entry points). That's besides all the doubt we have, from experience on Nightly, that the detection mechanisms are accurate. It's possible that Quantum, oop-webextensions, or other tools will provide better tools here - in which case this code will need rewriting anyway. I intend to: i) disable the watch code on 53 beta ASAP, in the way that the browser.addon-watch.interval pref was supposed to, with a targeted minimal patch; ii) thoroughly remove this code from nightly given that it's effectively not fit-for-purpose right now. If parts of it can be used later, that's why we have version control. No point shipping stuff that's effectively perma-disabled. iii) depending on the patch size of (ii), either uplift that to 54 or use the beta patch against 54, too.
Assignee: nobody → gijskruitbosch+bugs
Blocks: 1186491
Status: NEW → ASSIGNED
You may wish to coordinate / discuss this with Yoric, who appears to be separately addressing some perf issues in this area as part of bug 1342714.
Quick note: browser.addon-watch.interval was based on an early measurement architecture that was very e10s-unfriendly, so the reason it doesn't work anymore is that we switched to something much better in both robustness, battery usage and performance. Once bug 1342714 (which fixes a memory allocation issue) has landed, there should be no visible performance cost to performance monitoring unless about:performance is opened. Now, all the performance monitoring infrastructure was designed for two reasons: a. to provide Telemetry data that would help us find out which add-ons were slowing down Firefox; b. to provide raw data for Self-support/Shield. After 18 months, no manpower has materialized for either a. (Telemetry is available, nobody has ever looked at it as far as I can tell) or b. (I don't know if Self-support/Shield still exists, but I'm pretty sure it has never actually looked at the data we collect for it). As for both about:performance and the warning, they were mostly proofs of concept, waiting for manpower to be turned into something actually useful. A little manpower materialized for about:performance, none for the warning. So, as much as it saddens me, I guess that removing all this code makes sense. However, the timeframe might need to be revised. * The patch for ii) will be relatively small, but it hits in both SpiderMonkey, XPConnect, Toolkit and possibly a little bit of Graphics, so I strongly advise against any uplift. * If you're interested in performance, your patch for i) will have strictly no effect.
Flags: needinfo?(gijskruitbosch+bugs)
(and of course, browser/ and telemetry)
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #24) > Quick note: browser.addon-watch.interval was based on an early measurement > architecture that was very e10s-unfriendly, so the reason it doesn't work > anymore is that we switched to something much better in both robustness, > battery usage and performance. Once bug 1342714 (which fixes a memory > allocation issue) has landed, there should be no visible performance cost to > performance monitoring unless about:performance is opened. Is this bug going to be uplifted? > * If you're interested in performance, your patch for i) will have strictly > no effect. I'm quite skeptical about this claim from looking at the code... we initialize performance monitoring for all add-ons in all processes (including child processes). The effect might be 'negligible' or 'hard to measure' or 'nearly zero', but it's all running code and memory usage. Given we're going after unnecessary JSM imports just for their own sakes (bug 1350472), not running code that's just spinning plates seems a good idea.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8803010 - Attachment is obsolete: true
(In reply to :Gijs from comment #26) > > * If you're interested in performance, your patch for i) will have strictly > > no effect. > > I'm quite skeptical about this claim from looking at the code... we > initialize performance monitoring for all add-ons in all processes > (including child processes). The effect might be 'negligible' or 'hard to > measure' or 'nearly zero', but it's all running code and memory usage. Given > we're going after unnecessary JSM imports just for their own sakes (bug > 1350472), not running code that's just spinning plates seems a good idea. More specifically, I believe that the first patch in the series that I just attached ensures we don't call into PerformanceWatcher.jsm at any point unless someone's flipped the existing pref, thus avoiding loading the process script and all other addPerformanceListener calls *unless* the user opens about:performance, which should avoid the impact of the perf watching code as noticed in bug 1342714 because there'll be no callsites. That patch seems small enough to uplift. The second patch removes all the add-on watching/alert code, but not the web perf bits, so users could still open about:performance and use it to check web page performance.
Comment on attachment 8852418 [details] Bug 1309946 - disable addonwatcher for easy uplift, https://reviewboard.mozilla.org/r/124664/#review127168
Attachment #8852418 - Flags: review?(dteller) → review+
Comment on attachment 8852418 [details] Bug 1309946 - disable addonwatcher for easy uplift, https://reviewboard.mozilla.org/r/124664/#review127170 ::: browser/components/nsBrowserGlue.js:48 (Diff revision 2) > > [ > ["AboutHome", "resource:///modules/AboutHome.jsm"], > ["AboutNewTab", "resource:///modules/AboutNewTab.jsm"], > ["AddonManager", "resource://gre/modules/AddonManager.jsm"], > ["AddonWatcher", "resource://gre/modules/AddonWatcher.jsm"], Don't forget to remove it here, too.
(In reply to :Gijs from comment #31) > (In reply to :Gijs from comment #26) > > > * If you're interested in performance, your patch for i) will have strictly > > > no effect. > > > > I'm quite skeptical about this claim from looking at the code... we > > initialize performance monitoring for all add-ons in all processes > > (including child processes). The effect might be 'negligible' or 'hard to > > measure' or 'nearly zero', but it's all running code and memory usage. Given > > we're going after unnecessary JSM imports just for their own sakes (bug > > 1350472), not running code that's just spinning plates seems a good idea. > > More specifically, I believe that the first patch in the series that I just > attached ensures we don't call into PerformanceWatcher.jsm at any point > unless someone's flipped the existing pref, thus avoiding loading the > process script and all other addPerformanceListener calls *unless* the user > opens about:performance, which should avoid the impact of the perf watching > code as noticed in bug 1342714 because there'll be no callsites. That patch > seems small enough to uplift. Ok, so fair enough, this will remove the loading of a jsm. So you may win a few ms during startup. I don't remember having optimized nsPerformanceService.cpp to entirely deactivate performance monitoring when there are no observers, though, since we pretty much guaranteed that there would always be at least one, so the actual measures still take place in JS code. I could fix that, depending on whether we decide to maintain the performance service or not. > The second patch removes all the add-on watching/alert code, but not the web > perf bits, so users could still open about:performance and use it to check > web page performance. I'll take a look at your patch.
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #33) > Comment on attachment 8852418 [details] > Bug 1309946 - disable addonwatcher for easy uplift, > > https://reviewboard.mozilla.org/r/124664/#review127170 > > ::: browser/components/nsBrowserGlue.js:48 > (Diff revision 2) > > > > [ > > ["AboutHome", "resource:///modules/AboutHome.jsm"], > > ["AboutNewTab", "resource:///modules/AboutNewTab.jsm"], > > ["AddonManager", "resource://gre/modules/AddonManager.jsm"], > > ["AddonWatcher", "resource://gre/modules/AddonWatcher.jsm"], > > Don't forget to remove it here, too. I left this and the .uninit() call in this file because in principle users could change prefs, or add-ons could invoke the module directly. uninit() will be a no-op if nobody started the AddonWatcher.
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #34) > I don't remember having optimized > nsPerformanceService.cpp to entirely deactivate performance monitoring when > there are no observers, though, since we pretty much guaranteed that there > would always be at least one, so the actual measures still take place in JS > code. Assuming we're talking about nsPerformanceStatService: That shouldn't get invoked after the first patch unless about:performance is loaded, AIUI, because nobody imports PerformanceWatcher.jsm, and so nobody imports PerformanceStats.jsm, so nobody getService's the service. So nobody ever tells the stopwatches over in JS land to start measuring stuff (isMonitoringJank and isMonitoringCPOWs will be false). Maybe I've missed something?
Ah, that's possible. Well, it's worth checking.
Mmmh... if this works, this means that the (unconfirmed) performance loss reappears forever the first time we launch about:performance, though. Your second patch should work better, as it effectively blacklists addon compartments for the stopwatch. When a compartment is blacklisted, the stopwatch degenerates to an elaborate noop.
Marking qf so we track and report improvements in the wild
Whiteboard: [qf]
Comment on attachment 8852419 [details] Bug 1309946 - remove all traces of add-on performance monitoring, https://reviewboard.mozilla.org/r/124666/#review127908 I believe that this is good. Could you expand the commit message to explain 1/ why you're doing this; 2/ how this patch ensures that it removes the cost of add-on performance monitoring?
Attachment #8852419 - Flags: review?(dteller) → review+
(In reply to David Teller [:Yoric] (please use "needinfo") from comment #24) > Now, all the performance monitoring infrastructure was designed for two > reasons: > > a. to provide Telemetry data that would help us find out which add-ons were > slowing down Firefox; > b. to provide raw data for Self-support/Shield. > > After 18 months, no manpower has materialized for either a. (Telemetry is > available, nobody has ever looked at it as far as I can tell) or b. (I don't > know if Self-support/Shield still exists, but I'm pretty sure it has never > actually looked at the data we collect for it). If we are going to remove the data collection, I think a heads up to the Shield team is warranted. Matt, did the Shield team ever look at this data or does it plan to?
Flags: needinfo?(mgrimes)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3ec6625be031 disable addonwatcher for easy uplift, r=Yoric https://hg.mozilla.org/integration/autoland/rev/3343fa2a55d7 remove all traces of add-on performance monitoring, r=Yoric
Try showed I missed assertions against addonId in the remaining browser_compartments test (but only in non-e10s, which is why I missed them, I guess?). I fixed those and the other comments about addonId in that file, and while I was there, updated all the remaining SilentAssert calls that lacked assert failure messages to have them (because the "undefined is not equal to ''" is no help when the line number is in a utility function called from all over the place). Otherwise, try was green, so I landed this. Thanks everyone! :-)
https://hg.mozilla.org/mozilla-central/rev/3ec6625be031 https://hg.mozilla.org/mozilla-central/rev/3343fa2a55d7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Ah. You have excellent timing. I was just about to use this infrastructure to gather telemetry on jank caused by extension content scripts...
Depends on: 1352866
Flags: needinfo?(kev)
Performance Impact: --- → ?
Whiteboard: [qf]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: