Closed Bug 1477677 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Performance Monitoring, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
mozilla63
Tracking Status
firefox63 --- fixed
firefox65 --- verified
firefox66 --- verified

People

(Reporter: florian, Assigned: florian)

References

Details

(Whiteboard: [perf-tools])

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
- 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
Attachment #8994156 - Flags: review?(felipc)
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
Attachment #8994156 - Flags: review?(felipc) → review+
(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.
Attachment #8994156 - Attachment is obsolete: true
Pushed by florian@queze.net:
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.
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [perf-tools]
https://hg.mozilla.org/mozilla-central/rev/92ca61f5c4cd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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).

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

Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: