1597562 - Better support for Fission content processes in about:memory

Status: RESOLVED FIXED

(Toolkit :: about:memory, task, P3)

Description

[Andrew McCreight [:mccr8]]

kmag suggested the support for Fission could be improved in about:memory. Specifically, we could indicate more specific than "Web Content" for Fission processes. It sounded like maybe he was thinking of putting the URL in there? (Which would need to be anonymized if that was enabled.) I'm not sure how well that will work with the index, given how there's not much room.

Comment 1

[Chris Peterson [:cpeterson]]

Tentatively assigning to the M5 dogfooding milestone because this about:memory information would be helpful for dogfood testers.

Comment 2

[Chris Peterson [:cpeterson]]

Blocks Fission dogfooding (M5).

Comment 3

[Chris Peterson [:cpeterson]]

Moving to Fission Nightly (M6)

TBD: How might we redesign about:memory to scale to many iframe processes?

Comment 4

[Chris Peterson [:cpeterson]]

Yoric, perhaps you can work on this bug to improve about:memory for Fission after about:processes?

This bug is a Fission Nightly blocker (milestone M6c).

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Works for me.

mccr8, why do we need to anonymize?

Comment 6

[Andrew McCreight [:mccr8]]

Memory reports get included in crash reports.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

(In reply to Andrew McCreight [:mccr8] from comment #6)

Memory reports get included in crash reports.

But we can patch just the output of about:memory without patching the memory reports, right? Or are there some hardcoded shenanigans involved?

kmag, to clarify, is the objective to expand the data made available in the actual about:memory page or in the memory crash reports?

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #7)

But we can patch just the output of about:memory without patching the memory reports, right? Or are there some hardcoded shenanigans involved?

Anonymity is a property of an individual memory report. You have to check the aAnonymize flag when generating the report. Here's an example.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

(In reply to Andrew McCreight [:mccr8] from comment #8)

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #7)

But we can patch just the output of about:memory without patching the memory reports, right? Or are there some hardcoded shenanigans involved?

Anonymity is a property of an individual memory report. You have to check the aAnonymize flag when generating the report. Here's an example.

So you suggest I patch CollectReports rather than patching about:memory itself, right?

Comment 10

[Andrew McCreight [:mccr8]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #9)

So you suggest I patch CollectReports rather than patching about:memory itself, right?

I'm not sure what you mean. This extra information should be added to the memory reports. about:memory itself just loads a memory report. I don't think it adds extra information.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Well, that's my entire question :) It should be fairly easy to patch about:memory (the page) to add data only in the UX. If this data is never meant to be added to other kinds of memory reports, it might (or might not) be simpler/better.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1597562 - about:memory now displays process types;r?njn

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4311c3df8803
about:memory now displays process types;r=mccr8

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/010cfd3c3b56
Backed out changeset 4311c3df8803 for causing failures in aboutmemory.

Comment 15

[Alexandru Michis [:malexandru]]

Backed out changeset 4311c3df8803 for causing failures in aboutmemory.

Backout link: https://hg.mozilla.org/integration/autoland/rev/010cfd3c3b562bad64d115d5fe7ba09522d22e65

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&fromchange=8aaca63ec5c6dd73365ba31d1972771cb847d5bc&searchStr=windows%2C7%2Cdebug%2Cmochitests%2Cwithout%2Ce10s%2Ctest-windows7-32%2Fdebug-mochitest-chrome-1proc-3%2Cm-1proc%28c3%29&tochange=010cfd3c3b562bad64d115d5fe7ba09522d22e65

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304384299&repo=autoland&lineNumber=110076

Comment 16

[David Teller [:Yoric] - still alive but not very active]

Andrew, my code seems to add one instance of vsize, heap-allocated, resident... which cause all the tests to fail. Do you have an idea what could be going on?

Comment 17

[Andrew McCreight [:mccr8]]

I don't know anything about these tests. Maybe they are doing some kind of canonicalization that combines together reports from the same process type, and your patch is breaking that?

njn might have some guesses.

Comment 18

[Nicholas Nethercote [inactive]]

Several of the tests collate all the vsize/resident/whatever measurements and check if there are the expected number, which corresponds to the number of processes. If you have changed the number of processes that are alive, or you have somehow broken memory reporting of those measurements for some processes, then you will see failures along those lines.

Having said all that, I see failures like "typeError: can't access property "length", endOfBrowsers is null" which suggests something a bit deeper is going on. I don't know why the very simple patch could cause any of the above, though.

Comment 19

[David Teller [:Yoric] - still alive but not very active]

What do you mean collate? Could it somehow end merging two reports from processes that have the same name (minus pid)? So far, it's the only hypothesis that seems to make sense.

Having said all that, I see failures like "typeError: can't access property "length", endOfBrowsers is null" which suggests something a bit deeper is going on. I don't know why the very simple patch could cause any of the above, though.

Apparently, there's code in the tests that turns one-element arrays into their first/single value. I'm fairly sure that this is what causes the error messages because we end up with two items instead of one.

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Ok, about:memory does seem to merge information from processes that have the exact same name.

https://searchfox.org/mozilla-central/rev/7cadba1d8b8feaec4615f5bb98aac4b8a719793c/toolkit/components/aboutmemory/content/aboutMemory.js#1186

Is this a behavior I need to preserve? If so, I'll need to rework my patch a bit.

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #20)

Is this a behavior I need to preserve? If so, I'll need to rework my patch a bit.

about:memory includes the pid in the process name right now, so it isn't actually combining anything. I'm guessing the merging only happens in some kind of testing mode? Presumably it is doing something in testing mode that excludes the pid, so presumably when you exclude the pid you could also exclude the URL from the process name.

Comment 23

[Nicholas Nethercote [inactive]]

There is code in some of the tests to filter out the PIDs, e.g. here.

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: Bug 1597562 - Updating about:memory tests to handle the fact that more processes don't have an empty process name;r?njn!

The changes in ContentChild.cpp seem to accidentally undo some of the changes that required Bug 1634987. This patch attempts to update the tests to the new reality.

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Things are odd. Several about:memory tests fail on my machine from mozilla-central.

Maybe it's because I'm on macOS Catalina?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09ce524559da
about:memory now displays process types;r=mccr8

Comment 28

[Dorel Luca [:dluca]]

Backed out changeset 09ce524559da (bug 1597562) for failures in awsy/test_base_memory_usage.py. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305916862&repo=autoland&lineNumber=1238

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=HOLAl5FvRJi6nOOxLORywQ-0&revision=09ce524559daedac82944e7e71ea2dfc6c27aa3a

Backout:
https://hg.mozilla.org/integration/autoland/rev/32e2e5441bf670337149667d658945e873fdfcdf

Comment 29

[David Teller [:Yoric] - still alive but not very active]

That's... interesting.

jmaher, do you know how I can launch that test?

Comment 30

[Joel Maher ( :jmaher ) (UTC -8)]

I assume ./mach awsy-test --help will point you in the right direction

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Thanks.
Well, as expected, the tests fail on my machine even pre-patch. I'll wait until I have received my Linux desktop to resume work on this bug.

Comment 32

[Eric Rahm [:erahm]]

David, for this test you can run the follow for a quick sanity check: ./mach awsy-test --per-tab-pause 1 --settle-wait-time 1 testing/awsy/awsy/test_base_memory_usage.py

The failure points to process_perf_data:

[task 2020-06-11T11:16:48.593Z] 11:16:48     INFO - TEST-UNEXPECTED-ERROR | awsy/test_base_memory_usage.py TestMemoryUsage.test_open_tabs | IndexError: list index out of range
...
[task 2020-06-11T11:16:48.594Z] 11:16:48     INFO -   File "/builds/worker/workspace/build/venv/lib/python2.7/site-packages/awsy/process_perf_data.py", line 52, in median
[task 2020-06-11T11:16:48.594Z] 11:16:48     INFO -     return (sorted_[med - 1] + sorted_[med]) / 2

My guess is the list of values is empty leading ot an out of bounds acesss. We limit this value to only process that have the name Web Content. Did you change the name to not contain that string?

Comment 33

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Yoric, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 34

[David Teller [:Yoric] - still alive but not very active]

(In reply to Eric Rahm [:erahm] from comment #32)

My guess is the list of values is empty leading ot an out of bounds acesss. We limit this value to only process that have the name Web Content. Did you change the name to not contain that string?

Yes, under Fission, we're using a more detailed list of types. So, under Fission, Web Content => web, but without Fission, it remains Web Content. Trying to find out where we're using that string.

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d2e5708bb9cf
about:memory now displays process types;r=mccr8,perftest-reviewers,sparky

Comment 36

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/d2e5708bb9cf