1477677 - about:performance should display information from the new performance counters when they are enabled

Status: VERIFIED FIXED

(Toolkit :: Performance Monitoring, enhancement, P1)

Description

[Florian Quèze [:florian]]

The new performance counters are currently pref'ed off (behind the dom.performance.enable_scheduler_timing pref).

When they are enabled, we should display information based on them in about:performance instead of the current view.

This will let us experiment in Nightly with the current performance counters and start preparing a heuristic to decide which tabs are performing poorly.

Comment 1

[Florian Quèze [:florian]]

Attachment: Patch v1

- No tests for now. I think this is fine for something that would land pref'ed off.
- No l10n, but the existing about:performance page isn't localized either. Bug 1241024 covers it.

Here is a screenshot: https://i.imgur.com/3rWGV0Z.png

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8994156 [details] [diff] [review]
Patch v1

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

::: toolkit/components/aboutperformance/content/aboutPerformance.js
@@ +391,5 @@
>     */
>    _firstSeen: new Map(),
>  
> +  async _promiseSnapshot() {
> +    if (!Services.prefs.getBoolPref("dom.performance.enable_scheduler_timing", false)) {

Please make an external function that checks for this pref to use it here and in the other locations where this pref is checked.

This way it will be easier to document that this pref is used to toggle between the old and the new performance counters.

@@ +399,5 @@
> +    let counters = await ChromeUtils.requestPerformanceMetrics();
> +    let tabs = {};
> +    for (let counter of counters) {
> +      let {items, host, windowId, duration, isWorker, isTopLevel} = counter;
> +      if (isWorker && (windowId == 18446744073709552000 || !windowId))

curiosity: the windowId is not incremented by this code, so why is it only that this number must converted to 1? Is it the only one that displays incorrectly? (It's much bigger than Number.MAX_SAFE_INTEGER)

@@ +402,5 @@
> +      let {items, host, windowId, duration, isWorker, isTopLevel} = counter;
> +      if (isWorker && (windowId == 18446744073709552000 || !windowId))
> +        windowId = 1;
> +      let dispatchCount = 0;
> +      for (let {category, count} of items) {

nit: `category` unused? won't eslint complain about this?

@@ +527,5 @@
>      this._alerts = cleanedUpAlerts;
>      return result;
>    },
> +
> +  _trackingState: new Map(),

add a comment here explaining this "tracking" is about tracker URLs.  Tracking is an overloaded word and at first I thought this referred to some per-URL information that about:performance would be tracking.

@@ +575,5 @@
> +          name = "Preloaded: " + name;
> +        }
> +      } else if (id == 1) {
> +        name =
> +          Services.strings.createBundle("chrome://branding/locale/brand.properties")

Should the brandShortName string be a const in the top of the file?

@@ +583,5 @@
> +        let addon = WebExtensionPolicy.getByHostname(host);
> +        name = `${addon.name} (${addon.id})`;
> +        image = "chrome://mozapps/skin/extensions/extensionGeneric-16.svg";
> +      } else if (id == 0 && !tab.isWorker) {
> +        name = "Ghost windows";

nit: isn't "Ghost window" in the singular better?

@@ +947,5 @@
> +    } else {
> +      let counters = State.getCounters();
> +      for (let {name, image, totalDispatches, dispatchesSincePrevious,
> +                totalDuration, durationSincePrevious, children} of counters) {
> +        function dispatchAndDuraction(dispatches, duration) {

typo: duration

I'd use "dispatches" too, otherwise it sounds like this function is meant to dispatch something

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch v2 for landing

(In reply to :Felipe Gomes (needinfo me!) from comment #2)

Thanks for the review!

> @@ +399,5 @@
> > +    let counters = await ChromeUtils.requestPerformanceMetrics();
> > +    let tabs = {};
> > +    for (let counter of counters) {
> > +      let {items, host, windowId, duration, isWorker, isTopLevel} = counter;
> > +      if (isWorker && (windowId == 18446744073709552000 || !windowId))
> 
> curiosity: the windowId is not incremented by this code, so why is it only
> that this number must converted to 1? Is it the only one that displays
> incorrectly? (It's much bigger than Number.MAX_SAFE_INTEGER)

The windowId 1 is the doc group of the Firefox UI. The workers that have a windowId of 0 or max uint64 are typically workers that are part of Firefox; I force their windowId to 1 to have them displayed as children of the Firefox line.

I added a comment in the code to explain this hack.

> @@ +402,5 @@
> > +      let {items, host, windowId, duration, isWorker, isTopLevel} = counter;
> > +      if (isWorker && (windowId == 18446744073709552000 || !windowId))
> > +        windowId = 1;
> > +      let dispatchCount = 0;
> > +      for (let {category, count} of items) {
> 
> nit: `category` unused? won't eslint complain about this?

Yep, and it actually found another coding style nit that I fixed too.

> @@ +575,5 @@
> > +          name = "Preloaded: " + name;
> > +        }
> > +      } else if (id == 1) {
> > +        name =
> > +          Services.strings.createBundle("chrome://branding/locale/brand.properties")
> 
> Should the brandShortName string be a const in the top of the file?

Actually, there's an existing const I can just re-use.

> @@ +583,5 @@
> > +        let addon = WebExtensionPolicy.getByHostname(host);
> > +        name = `${addon.name} (${addon.id})`;
> > +        image = "chrome://mozapps/skin/extensions/extensionGeneric-16.svg";
> > +      } else if (id == 0 && !tab.isWorker) {
> > +        name = "Ghost windows";
> 
> nit: isn't "Ghost window" in the singular better?

I wish we had a single ghost window, but typically there's a whole bunch: when a window becomes a ghost window, its window id becomes 0, so all the ghost windows are mixed together under that header.

Anyway, "Ghost window" is jargony, I hope when we'll be closer to something that can be exposed to users UX will come up with a better wording.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/92ca61f5c4cd
about:performance should display information from the new performance counters when they are enabled, r=felipe.

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/92ca61f5c4cd

Comment 6

[Florian Quèze [:florian]]

For anybody who wants to try this now that it's in Nightly, here are the steps:
1. From about:config, set dom.performance.enable_scheduler_timing to true.
2. Restart the browser.
3. Open about:performance

If you don't restart the browser after step 1, the browser will crash at step 3 (this is bug 1474286).

Comment 7

[Vlad Lucaci, Thunderbird QA (:vlucaci)]

Verified this issue on the latest Nightly 66.0a1 (2019-01-14) and 65.0b10 (64-bit)on Windows 10x64, macOS 10.13 and Ubuntu 16.04x64