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

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.

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.

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.

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.

... 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.

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

(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

Comment on attachment 8624682 [details] MozReview Request: Bug 1175098 - PerformanceStats for e10s;r=felipe,mconley https://reviewboard.mozilla.org/r/11725/#review11143 Ship It!

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

This started failing when it landed to fx-team, so backed out in https://hg.mozilla.org/integration/fx-team/rev/3bdf6df8a0f9 https://treeherder.mozilla.org/logviewer.html#?job_id=3658963&repo=fx-team https://treeherder.mozilla.org/logviewer.html#?job_id=3658671&repo=fx-team

Thanks, I'll investigate.

Did this fail only under Linux?

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.

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

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

Backed out for e10s browser_compartments.js failures. https://treeherder.mozilla.org/logviewer.html#?job_id=3734299&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/6a45429486c6

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

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.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ea01f769752 Filed multiple loading as bug 1184115.

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

:/ 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

Bug 1175098 - Fixing double-loading of PerformanceStats content script;r?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).

Thanks, I'll be watching that Try push :)

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!

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.

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.

(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?

https://hg.mozilla.org/mozilla-central/rev/b454bbcabfad https://hg.mozilla.org/mozilla-central/rev/8ef2ef7d8490