1646505 - Figure out if GetInProcessTop usage in DocGroup::ReportPerformanceInfo is OK

Status: RESOLVED FIXED

(Core :: Performance, defect, P1)

Description

[Kris Maglione [:kmag]]

It generally isn't Fission-compatible, but the performance code may have other ways to deal with that that I don't know about.

Comment 1

[Markus Stange [:mstange]]

Tarek, can you take a look?

Comment 2

[Tarek Ziadé (:tarek)]

The code is trying to crawl back to the top window to grab its id in order to call PerformanceInfo against it (which is basically returning the counters data that are stored in that window)

So I guess my question would be: is there a fission-compatible way to do this ?

Comment 3

[Kris Maglione [:kmag]]

(In reply to Tarek Ziadé (:tarek) from comment #2)

The code is trying to crawl back to the top window to grab its id in order to call PerformanceInfo against it (which is basically returning the counters data that are stored in that window)

So I guess my question would be: is there a fission-compatible way to do this ?

Yes. It can walk the WindowContext tree, which will give you the inner and outer window IDs of all of your ancestors, regardless of which process they're in.

Comment 4

[Kris Maglione [:kmag]]

That said, I doubt that PerformanceInfo will work when given a window ID that lives in a different process, in which case some new performance info infrastructure will probably need to be added to WindowContext.

Comment 5

[Chris Peterson [:cpeterson]]

Tracking "Figure out GetInProcessTop usage" bugs for Fission M6b.

Comment 6

[Neha Kochar [:neha]]

Tarek, where is this used? Trying to figure out what would be broken if this is not fixed for Fission - we need this info for our ongoing milestone M6b.
Also, were you able to re-review this in light of comment 4?

Comment 7

[Tarek Ziadé (:tarek)]

the API is used for about:performance, and we would need to adapt it in light of https://bugzilla.mozilla.org/show_bug.cgi?id=1646505#c4

Comment 8

[Neha Kochar [:neha]]

Thanks, Tarek.
Patricia, please find an assignee for this as this is blocking Fission Nightly (M6b).

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

So I think more changes are needed than just adapting an alternate way to get the top window id. In fission, iframes typically live in other processes -- which show up as top-level entities in about:performance. I.e. I see 'adservice.google.com', 'apis.google.com', 's7.addthis.com', 'widgets.outbrain.com', etc all as processes in about:performance at the moment.

What's the UI design for showing the relationships between frames and their parents that's planned? You can't simply move them under their parent, since (for example) apis.google.com could appear in iframes of a dozen different tabs/top-level pages.

Right now, as best I can tell, it's happening to work (though perhaps by accident or luck).

If this is (more or less) working, I would move it to m6c, which is stuff we really want for fission Nightly, but we can get along for a while in Nightly without.

Comment 10

[Tarek Ziadé (:tarek)]

yeah I don't think it's a big deal if it's misplaced in about:performance for now

For the next steps, I'll defer to Florian, for reworking stuff from an UI pov

(he's off until August 10th though)

Comment 11

[Andreas Farre [:farre]]

So I'm a big fan of about:performance, and I think it offers a great and orthogonal perspective to what we have in plan for with about:processes, but I agree with Tarek that this looking a bit funky with fission is ok for now.

So my take is that this shouldn't block M6. What do you think Neha?

Comment 12

[Neha Kochar [:neha]]

Thanks for the analysis. I agree we don’t need to block Nightly on it, but will be desired to get fixed for Nightly so moving to M6c for now.

Comment 13

[Patricia Lawless]

needinfo to Harald since Florian is on pto currently. (see comment 10)

Comment 14

[Neha Kochar [:neha]]

please see comment 10.

Comment 15

[Florian Quèze [:florian]]

(In reply to Randell Jesup [:jesup] (needinfo me) from comment #9)

So I think more changes are needed than just adapting an alternate way to get the top window id. In fission, iframes typically live in other processes -- which show up as top-level entities in about:performance. I.e. I see 'adservice.google.com', 'apis.google.com', 's7.addthis.com', 'widgets.outbrain.com', etc all as processes in about:performance at the moment.

What's the UI design for showing the relationships between frames and their parents that's planned? You can't simply move them under their parent, since (for example) apis.google.com could appear in iframes of a dozen different tabs/top-level pages.

I'm really confused by this comment. I wonder if you wrote "about:performance" but meant "about:processes". about:performance doesn't show any process information, and (without fission) already showed subframes of different origins as child nodes of the top level tab.

(In reply to Tarek Ziadé (:tarek) from comment #10)

yeah I don't think it's a big deal if it's misplaced in about:performance for now

For the next steps, I'll defer to Florian, for reworking stuff from an UI pov

Can you clarify what needs reworking from a UI point of view?

Comment 16

[Andreas Farre [:farre]]

Attachment: Bug 1646505 - Fix about:performance for Fission. r=florian

With fission it's not possible to use
nsPIDOMWindowOuter::GetInProcessTop and expect to find the top-level
window, since it might be in another process. Instead we use
BrowsingContext::Top. Another issue is that it's not possible to
traverse children by recursively calling GetTabSizes, instead we need
to use BrowsingContext traversal mechanics.

Memory reporting is a bit tricky, since now doc gropus belonging to a
cross process top level browsing context needs to report it's memory
usage and can't skip it, so this is also handled.

Comment 17

[Andreas Farre [:farre]]

Florian, I've been able to fix so that about:performance works better with fission, but I've found some other issues that have made me reconsider my comment 11.

The problems I've found are the following:

1. CollectMemoryInfo is called for every DocGroup if the DocGroup contains a top-level window. This is problematic since a BrowsingContextGroup might contain several top-level BrowsingContexts, that are in different DocGroups but they all share the same zone, and this makes CollectMemoryInfo count used heap double. This situation occurs for cross-origin window.open or cross-origin links with a top-level target.

2. CollectMemoryInfo also only computes the size of one top-level BrowsingContext-tree, and there can be several same group top-level BrowsingContexts that have the same DocGroup which makes us miss counting the size of the entire tree of the skipped top-level. This situation occurs for same-origin window.open or same-origin links with a top-level target.

The latter is also a problem for execution time reporting, where two tabs share performance counters.

These things are also something that the UI isn't aware of and need to handle somehow if we want something that is better than an approximation.

The patch I've attached doesn't really fix any of this, but makes the FIssion case less bad.

Comment 18

[Andreas Farre [:farre]]

If you want I can file bugs for these things.

Comment 19

[Florian Quèze [:florian]]

(In reply to Andreas Farre [:farre] from comment #17)

Thanks for looking at this bug!

Florian, I've been able to fix so that about:performance works better with fission, but I've found some other issues that have made me reconsider my comment 11.

The problems I've found are the following:

I'm afraid I'll need to read some documentation about DocGroup, BrowsingContext and BrowsingContextGroup before being able to understand the problems you are describing.

The latter is also a problem for execution time reporting, where two tabs share performance counters.

The data shown in about:performance without Fission was afaik quite good for JavaScript execution (but mistakenly reported low energy use for a tab doing a webrtc video call), but the memory values have always been significantly under reported, to the point that I'm not sure if they are really useful. Sometimes they help find a leaky tab. In lots of cases they don't. How much would it help if we decided to drop support for the memory column?

These things are also something that the UI isn't aware of and need to handle somehow if we want something that is better than an approximation.

I don't understand the problems enough to understand how much platform and front-end work there would be to make the display correct, but if you are confident you can fix the platform side, I can find time in December to fix the front-end.

The patch I've attached doesn't really fix any of this, but makes the FIssion case less bad.

I've not worked on the platform part of about:performance, Tarek did, so the review should be directed to him.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45ecc6edfddd
Fix about:performance for Fission. r=tarek

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/45ecc6edfddd