912165 - Remove the Linux-only maps reporters

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

On Linux, we read /proc/self/smaps and present the info in about:memory (in
verbose mode).  This appears to be busted, at least on my machine, where I
currently get these rubbish results:

> Resident Set Size (RSS) Breakdown
> 
> 73,728 B (100.0%) -- rss
> └──73,728 B (100.0%) ── other-files/firefox/[r-xp]
> 
> Proportional Set Size (PSS) Breakdown
> 
> 73,728 B (100.0%) -- pss
> └──73,728 B (100.0%) ── other-files/firefox/[r-xp]
> 
> Virtual Size Breakdown
> 
> 110,592 B (100.0%) -- size
> └──110,592 B (100.0%) ── other-files/firefox/[r-xp]
> 
> Swap Breakdown
> 
> 0 B (100.0%) -- swap
> └──0 B (100.0%) ── total

Other than "swap", the numbers are ridiculous, and there are meant to be
hundreds of entries

I have no idea what went wrong.  It's possible the smaps file format changed
slightly, silently breaking the parsing.

I wonder how long it's been like this.  We don't directly test these reporters
(mostly because Linux-only code in a mochitest feels weird, I guess), and it
appears nobody has noticed.

Furthermore, while these reports are interesting, I don't recall any occasions
when we actually acted on them.  (jlebar might, but he probably won't read
this.)  And the implementation is non-trivial;  the part where we read
/proc/self/smaps isn't too bad (though apparently fragile!) but aboutMemory.js
has quite a bit of special-casing to handle these reports -- they get put in
their own trees, and we only show them in verbose mode or if we're reading from
file.

So I think they should just be removed.

Comment 1

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Comparing the 3.2 kernel on my laptop, where this works, with the 3.11 kernel on my virtual ARM dev board, I see this line in the latter, at the end of each map entry:

VmFlags: rd wr mr mw me gd ac 

This line will fail to match the "%1024[a-zA-Z_]: %llu kB\n" in MapsReporter::ParseMapBody.  It should be relatively simple to fix the parser to skip lines that aren't a number of kB.

Comment 2

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

Also this is Linux kernel commit 834f82e2aa9a8ede94b17b656329f850c1471514, and `git tag --contains` suggests it's in Linux >= 3.8.

Comment 3

[Nicholas Nethercote [inactive]]

> It should be relatively simple to fix the parser to skip lines that aren't a number of kB.

Removing it altogether will be even easier.  Future-proof, too! :)

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: Remove the Linux-only smaps memory reporters.

Pretty straightforward.  ResidentUniqueReporter was the only thing salvaged
from MapsMemoryReporter.cpp;  it was moved into nsMemoryReporterManager.cpp.

8 files changed, 66 insertions(+), 773 deletions(-)

Comment 5

[Andrew McCreight [:mccr8]]

Jed, how likely do you think this would be useful in the future?  I'm not really sure what this is even measuring.

Comment 6

[Justin Lebar (not reading bugmail)]

> (jlebar might, but he probably won't read this.)

I'm still lurking despite my better judgement.  :)

These have been marginally useful on B2G, particularly because they let you tell how much memory is anonymous.  On desktop, I think these are the only reporters which tell you how much memory is swapped out.

But njn's points are well taken.

Comment 7

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

This lets us see how much of a process's memory usage is due to mapped files (more to the point, which ones), which seems as if it could potentially be important in memory-constrained environments like b2g.  If we have another way to get that info then ignore this, but if we're left looking at only malloc'ed space then we don't have the whole picture.

But if people who've actually been working on this stuff in the past haven't found it useful, then maybe it's not.

Comment 8

[Andrew McCreight [:mccr8]]

(In reply to Jed Davis [:jld] from comment #7)
> But if people who've actually been working on this stuff in the past haven't
> found it useful, then maybe it's not.

Ok, thanks!  I guess we'll have to keep this in mind in case we start getting weird OOM issues on B2G.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 799254 [details] [diff] [review]
Remove the Linux-only smaps memory reporters.

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

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +72,5 @@
>  {
>      return GetResident(aN);
>  }
>  
> +#define HAVE_RESIDENT_UNIQUE_REPORTER

Maybe add a blank line to separate the define from the class a bit. But maybe that's the style of the file.

@@ +73,5 @@
>      return GetResident(aN);
>  }
>  
> +#define HAVE_RESIDENT_UNIQUE_REPORTER
> +class ResidentUniqueReporter MOZ_FINAL : public MemoryUniReporter

The false parallel of UniqueReporter and UniReporter is unfortunate. ;)

@@ +93,5 @@
> +    // statm's "shared" value actually counts pages backed by files, which has
> +    // little to do with whether the pages are actually shared.  smaps on the
> +    // other hand appears to give us the correct information.
> +    //
> +    // We could calculate this data within the smaps reporter, but the overhead

This comment needs to be updated, as there's no smaps reporter any more.

Comment 10

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d93007abf58

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6d93007abf58