Closed Bug 1175098 Opened 5 years ago Closed 4 years ago

PerformanceStats for e10s

Categories

(Toolkit :: Performance Monitoring, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox42 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(2 files)

We should make PerformanceStats speak e10s.
What is PerformanceStats? Is that about:performance?
It's the API used by about:performance and the AddonWatcher. And possibly some day by add-ons.
Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
Attachment #8624682 - Flags: review?(mconley)
Attachment #8624682 - Flags: review?(dtownsend)
Duplicate of this bug: 1140310
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
Mike, in case you don't feel like understanding every single nook and cranny of PerformanceStats, can you take a look at the definition of object `Process` in PerformanceStats.jsm, line 685 and at ContentPerformanceStats.js? That's where everything really e10s-specific is.
Flags: needinfo?(mconley)
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

https://reviewboard.mozilla.org/r/11725/#review10195

::: toolkit/components/perfmonitoring/ContentPerformanceStats.js:16
(Diff revision 2)
> +const { PerformanceStats } = Cu.import("resource://gre/modules/PerformanceStats.jsm", {});
> +const { Task } = Cu.import("resource://gre/modules/Task.jsm", {});
> +const { Services } = Cu.import("resource://gre/modules/Services.jsm", {});

Nit - can these not be loaded lazily?

::: toolkit/components/perfmonitoring/ContentPerformanceStats.js:26
(Diff revision 2)
> + * monitor is controlled by the activation/deactivation of probes marked as "-client"

-client? Or -content?

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:733
(Diff revision 2)
> +    if (!this.loader || this.loader.childCount == 0) {

Note that in the parent process, the childCount should never be 0 - I believe the in-process child message manager, which we use for non-remote tabs, is still counted as a child here.

I guess it's fine to return undefined in that case, but we've got big problems if the childCount is 0. Maybe assert instead?

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:736
(Diff revision 2)
> +    let TOPIC = "performance-stats-service-" + topic;

You've all-caps'd this like a const. Should this be a const?

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:740
(Diff revision 2)
> +    // Apparently, for reasons I do not understand, `this.loader.childCount`
> +    // returns the number of child processes + 2.

Ah, so like I mentioned above, one of them will be for the in-parent content message manager for non-remote tabs.

The other one is likely to fade in and out of existence, and is probably the background thumbnailing service, which uses remote browsers to download thumbnails.

That one may or may not exist at any time. :(

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:730
(Diff revision 2)
> +   * NOOP if we are in a child process, return `undefined` immediately.

Also returns undefined if there are no child processes.

::: toolkit/components/perfmonitoring/PerformanceStats.jsm:771
(Diff revision 2)
> +    // Processes can die/freeze/..., so don't expect that they will
> +    // always respond.

Well spotted.

::: toolkit/components/perfmonitoring/moz.build:16
(Diff revision 2)
> +    'ContentPerformanceStats.js',

Nit - generally, I believe we name content scripts with a "-content.js" suffix.

My suggestion is "PerformanceStats-content.js" for consistency, but I honestly don't care too too much.

::: toolkit/components/perfmonitoring/ContentPerformanceStats.js:113
(Diff revision 2)
> + * @param {{data: {payload: Array<string>}}} msg The message received. `payload`

I think this @param documentation is slightly misleading - it seems to suggest that the message sender needs to pass an object with format:

{{data: {payload: Array<string>}}}.

When in fact data is taken care of for us. Really, this handles messages with data that looks like:

{payload: Array<string>}, which is probably easier to read anyways.

::: toolkit/components/perfmonitoring/ContentPerformanceStats.js:61
(Diff revision 2)
> + * Handle message `performance-stats-service-acquire`: release a given probe

Nit: -acquire -> -release

::: toolkit/components/perfmonitoring/ContentPerformanceStats.js:79
(Diff revision 2)
> +  probes = probes.filter(x => x != name);

probes is not being defined properly. Name is also undefined here.
https://reviewboard.mozilla.org/r/11725/#review10197

> Ah, so like I mentioned above, one of them will be for the in-parent content message manager for non-remote tabs.
> 
> The other one is likely to fade in and out of existence, and is probably the background thumbnailing service, which uses remote browsers to download thumbnails.
> 
> That one may or may not exist at any time. :(

So what do you suggest I do? Always rely upon a timeout?
https://reviewboard.mozilla.org/r/11725/#review10199

> probes is not being defined properly. Name is also undefined here.

Oops, copy & paste error.
Attachment #8624682 - Flags: review?(mconley)
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
https://reviewboard.mozilla.org/r/11725/#review10201

> So what do you suggest I do? Always rely upon a timeout?

Subtracting one for the in-parent child message manager is fine, but I suggest replacing the documentation saying that's what's going on.

I actually want to correct myself - I can only account for a single extra content process, that being the in-parent child message manager. I have no idea where that second one is coming from. Perhaps billm would know?

I guess I'm just suggesting to update the comment explaining what's going on - because it took some digging for me to figure out why you had two additional children (and even then, I can only account for one of them).
https://reviewboard.mozilla.org/r/11725/#review10207

> Subtracting one for the in-parent child message manager is fine, but I suggest replacing the documentation saying that's what's going on.
> 
> I actually want to correct myself - I can only account for a single extra content process, that being the in-parent child message manager. I have no idea where that second one is coming from. Perhaps billm would know?
> 
> I guess I'm just suggesting to update the comment explaining what's going on - because it took some digging for me to figure out why you had two additional children (and even then, I can only account for one of them).

Well, in my code, the in-parent child message manager responds to the request, and if I wait for `childCount` responses, I always timeout. Strike that: according to my logs, I _almost_ always timeout.

Juuuuust a little bit scary.
https://reviewboard.mozilla.org/r/11725/#review10219

> Well, in my code, the in-parent child message manager responds to the request, and if I wait for `childCount` responses, I always timeout. Strike that: according to my logs, I _almost_ always timeout.
> 
> Juuuuust a little bit scary.

In that case, if you've only got non-e10s tabs open, it's _possible_ that this is the thumbnailing service.

You can try to confirm that by turning background thumbnailing off: Add a new bool pref browser.pagethumbnails.capturing_disable set to true.
Clearing needinfo, since I gave a review.
Flags: needinfo?(mconley)
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

https://reviewboard.mozilla.org/r/11725/#review10225

I'm curious to know if disabling thumbnail capture solves the mystery of the other child. If so, we can update the comment that I highlighted accordingly and probably close the other bug that got opened about it.

Anyhow, beyond this last problem I spotted, the things you've asked me to look at seem sane!

::: toolkit/components/perfmonitoring/PerformanceStats-contents.js:84
(Diff revision 4)
> +    .filter(x => !msg.data.payload.include(x));

Did you mean includes?

According to https://developer.mozilla.org/en-US/docs/Using_JS_in_Mozilla_code, Array.prototype.includes is Nightly only for now. Best to use indexOf for now, I guess.
Attachment #8624682 - Flags: review?(mconley) → review+
...
Ok, I found the culprit: the timeout was too short. Content processes are very busy when they load a page, and they don't answer within .5 seconds. Increasing the timeout to 5 seconds dropped the timeout rate to ~0. In a followup bug, I will try and display something nicer in about:performance in presence of non-responding processes.
Attachment #8624682 - Flags: review?(dtownsend) → review+
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

https://reviewboard.mozilla.org/r/11725/#review10497

If conley is happy then so am I
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
Attachment #8624682 - Flags: review+
(In reply to Dave Townsend [:mossop] (PTO till July 21st) from comment #18)
> Comment on attachment 8624682 [details]
> MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
> 
> https://reviewboard.mozilla.org/r/11725/#review10497
> 
> If conley is happy then so am I

Note that I only seriously reviewed the bits that Yoric requested in comment 7.
I'll ask felipe to review it.
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley
Attachment #8624682 - Flags: review?(felipc)
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

https://reviewboard.mozilla.org/r/11725/#review11143

Ship It!
Attachment #8624682 - Flags: review?(felipc) → review+
Assignee: nobody → dteller
Thanks, I'll investigate.
Flags: needinfo?(dteller)
Did this fail only under Linux?
Flags: needinfo?(wkocher)
I believe so. It does seem to be not-completely-permafail on Linux, so it's possible it just lucked out of failing on the other platforms. I retriggered non-Linux runs on https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=8135d886c5ed&filter-searchStr=e10s%28 to see if it fails at all on those platforms.
Flags: needinfo?(wkocher)
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley
Attachment #8624682 - Attachment description: MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=mossop,mconley → MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley
Attachment #8624682 - Flags: review+
Mmmh... I can't seem to reproduce with my patch, so I'll assume that I didn't have the latest version uploaded.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f16e79d109b1
Keywords: checkin-needed
Mmmh.... nsPerformanceStats.cpp doesn't match between m-c and my tree. Sounds like a rebase error. I wonder how I can fix that.
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley
Attachment #8624682 - Flags: review?(mconley)
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

https://reviewboard.mozilla.org/r/11725/#review11967

Yeah, this is fine - but let's be sure to get a bug on file for the multiple process script loads... that sounds really wrong.
Attachment #8624682 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/653179afb65f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
This is still extremely failure-prone on WinXP. Backed out.
https://treeherder.mozilla.org/logviewer.html#?job_id=3779658&repo=fx-team

https://hg.mozilla.org/mozilla-central/rev/61eccd5a10ed
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla42 → ---
:/
Thanks, I'll investigate.
Comment on attachment 8624682 [details]
MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley

Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley
Attachment #8624682 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3786a4026ff8
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Bug 1175098 - Fixing double-loading of PerformanceStats content script;r?mconley
Attachment #8636442 - Flags: review?(mconley)
Comment on attachment 8636442 [details]
MozReview Request: Bug 1175098 - Fixing double-loading of PerformanceStats content script;r?mconley

Bug 1175098 - Fixing double-loading of PerformanceStats content script;r?mconley
Try run with fixup patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a3901597b2d

If it turns out green, I guess I can self-review, given the triviality of the patch.
Ryan, I have moved the discussion here, because this seems to be the best place to put the fix (and hopefully land it today).
Flags: needinfo?(ryanvm)
Depends on: 1183236
Thanks, I'll be watching that Try push :)
Flags: needinfo?(ryanvm)
Comment on attachment 8636442 [details]
MozReview Request: Bug 1175098 - Fixing double-loading of PerformanceStats content script;r?mconley

The Try run looks very promising for fixing bug 1183236!
Attachment #8636442 - Flags: feedback+
Yoric - you've also asked me to review bug 1184115, which includes a patch that is very similar (but not exactly the same) as the one you've posted here.

Can you please help me reconcile these two? At the very least, you're going to bitrot yourself, but I suspect something's gotten confused here.
Flags: needinfo?(dteller)
Sorry, wrong manipulation. The patch was meant to be posted here, but I gave the wrong bug# in MozReview. So this patch is the right one.
Flags: needinfo?(dteller) → needinfo?(mconley)
(additionally, the patch of bug 1184115 contained a typo)
Note: I'm for shipping this immediately. I won't be available for the next ~6h, though, so if anyone else wants to do this once mconley has confirmed his review, that would be great.
Comment on attachment 8636442 [details]
MozReview Request: Bug 1175098 - Fixing double-loading of PerformanceStats content script;r?mconley

https://reviewboard.mozilla.org/r/13519/#review12311

LGTM
Alright, clearing needinfo.

Can you obsolete the patch in bug 1184115 then as well, please?
Flags: needinfo?(mconley) → needinfo?(dteller)
Flags: needinfo?(dteller)
You need to log in before you can comment on or make changes to this bug.