Figure out if GetInProcessTop usage in DocGroup::ReportPerformanceInfo is OK
Categories
(Core :: Performance, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox85 | --- | fixed |
People
(Reporter: kmag, Assigned: farre)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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 2•4 years ago
|
||
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 ?
Reporter | ||
Comment 3•4 years ago
|
||
(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.
Reporter | ||
Comment 4•4 years ago
|
||
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
.
Updated•4 years ago
|
Comment 5•4 years ago
|
||
Tracking "Figure out GetInProcessTop usage" bugs for Fission M6b.
Comment 6•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
Thanks, Tarek.
Patricia, please find an assignee for this as this is blocking Fission Nightly (M6b).
Comment 9•4 years ago
|
||
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•4 years ago
|
||
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)
Assignee | ||
Comment 11•4 years ago
|
||
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•4 years ago
|
||
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•4 years ago
|
||
needinfo to Harald since Florian is on pto currently. (see comment 10)
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Comment 15•4 years ago
|
||
(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 nowFor 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?
Updated•4 years ago
|
Assignee | ||
Comment 16•4 years ago
|
||
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.
Assignee | ||
Comment 17•4 years ago
|
||
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:
-
CollectMemoryInfo
is called for everyDocGroup
if theDocGroup
contains a top-level window. This is problematic since aBrowsingContextGroup
might contain several top-levelBrowsingContext
s, that are in differentDocGroup
s but they all share the same zone, and this makesCollectMemoryInfo
count used heap double. This situation occurs for cross-originwindow.open
or cross-origin links with a top-level target. -
CollectMemoryInfo
also only computes the size of one top-levelBrowsingContext
-tree, and there can be several same group top-levelBrowsingContext
s that have the sameDocGroup
which makes us miss counting the size of the entire tree of the skipped top-level. This situation occurs for same-originwindow.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.
Assignee | ||
Comment 18•4 years ago
|
||
If you want I can file bugs for these things.
Comment 19•4 years ago
|
||
(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•4 years ago
|
||
Comment 21•4 years ago
|
||
bugherder |
Description
•