Closed Bug 1646505 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Performance, defect, P1)

defect

Tracking

()

RESOLVED FIXED
85 Branch
Fission Milestone M6c
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.

Tarek, can you take a look?

Flags: needinfo?(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 ?

Flags: needinfo?(tarek) → needinfo?(kmaglione+bmo)

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

Flags: needinfo?(kmaglione+bmo)

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.

Severity: -- → S3
Priority: -- → P3

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

Fission Milestone: --- → M6b

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?

Flags: needinfo?(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

Flags: needinfo?(tarek)

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

Flags: needinfo?(plawless)

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.

Flags: needinfo?(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)

Flags: needinfo?(tarek)

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?

Flags: needinfo?(nkochar)

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.

Fission Milestone: M6b → M6c
Flags: needinfo?(nkochar)

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

Flags: needinfo?(plawless) → needinfo?(hkirschner)

please see comment 10.

Flags: needinfo?(florian)
Assignee: nobody → afarre
Flags: needinfo?(bugzilla)

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

Flags: needinfo?(florian)
Status: NEW → ASSIGNED
Priority: P3 → P1

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.

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.

Flags: needinfo?(florian)

If you want I can file bugs for these things.

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

Flags: needinfo?(florian)
Pushed by afarre@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/45ecc6edfddd Fix about:performance for Fission. r=tarek
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Regressions: 1701190
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: