912863 - Measure RSS in a better way

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Nicholas Nethercote [inactive]]

Bug 910517 alters how memory reporters work, which breaks Talos's RSS collection.  Turns out there's a much better way to get RSS from the nsIMemoryReporterManager anyway, and that won't be broken by the changes in bug 910517.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Improve how Talos measures RSS.

The change is simple.  The new code only gives the RSS for the main process,
but then, that's also true of the old code, despite the presence of the
procName.split() call -- child processes only report memory consumption if
about:memory sends them some magic notifications, and that's not the case here.

This bug is blocking bug 910517, which itself is blocking a bunch of subsequent
work on memory reporters, so I'd really like for it to be deployed ASAP.
What's the process for updating Talos?

Comment 2

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

Comment on attachment 799994 [details] [diff] [review]
Improve how Talos measures RSS.

Review of attachment 799994 [details] [diff] [review]:
-----------------------------------------------------------------

this looks good.  I will run this on try to confirm it works the same.  

Which branches will this affect, just m-c and siblings?

Comment 3

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

try server run is looking good:
https://tbpl.mozilla.org/?tree=Try&rev=d3d59caa0fa5

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Joel Maher (:jmaher) from comment #2)
> Which branches will this affect, just m-c and siblings?

Yeah, there's no need to backport any of njn's memory reporter stuff.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 799994 [details] [diff] [review]
Improve how Talos measures RSS.

Thanks for the quick review.  How do I go about landing this?  Can I just push to the talos repo?  Is there some kind of deployment step?

Comment 6

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

Please push to the talos repo, then you can update talos.json to point to the new revision in the talos repo, please see bug 900913 as an example.  I will be on PTO tomorrow, so if you want this landed asap, ask jhammel for a review or armenzg.

Comment 7

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/build/talos/rev/ca2229a32cb6

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: Update Talos version.

Comment 9

[Jeff Hammel]

Comment on attachment 800434 [details] [diff] [review]
Update Talos version.

lgtm

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1da522b9214b

Ok, overall that was much less painful than I feared :)

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1da522b9214b

Comment 12

[Nicholas Nethercote [inactive]]

This patch worked great for the desktop test machines.  But the Android machines (specifically "Android 2.2 Opt" and "Android 4.0 Opt") are apparently deployed separately, and didn't get the update, because they broke when I re-landed bug 910517.

A gold star to anyone who can tell me how to update Talos for Android...

Comment 13

[Nicholas Nethercote [inactive]]

Callek, philor thought you might know the answer to comment 12...