798024 - Don't hide smaps when displaying a memory report dump loaded from a file or from the clipboard

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

We hide smaps when showing memory reports because they take up a lot of memory and don't help much.

However, smaps are pretty useful if only so we can see a process's RSS and PSS.  When we're loading a memory report from a file, presumably we don't care about Firefox's memory usage, so I don't see a reason to hide this data.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 668158 [details] [diff] [review]
Patch, v1

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

Current logic:

- Non-verbose: ignore maps trees
- Verbose:     show maps trees, but collapse them

With this change:

- Non-verbose, non-JSON: ignore maps trees
- Non-verbose, JSON:     show maps trees, but collapse them (and they'll contain tiny nodes)
- Verbose:               show maps trees, but collapse them

Is that right?

The logic is getting complicated.

> However, smaps are pretty useful if only so we can see a process's RSS and PSS.

So you just want to see the total RSS and PSS numbers?  We've already got "resident" in the "Other Measurements" section, as well as "vsize".  Would adding single reporters for "pss" and "swap" suffice?

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +375,5 @@
>      let process = function(aIgnoreSingle, aIgnoreMulti, aHandleReport) {
>        processMemoryReporters(aIgnoreSingle, aIgnoreMulti, aHandleReport);
>      }
> +    appendAboutMemoryMain(body, process, gMgr.hasMozMallocUsableSize,
> +                          /* fromFileOrClipboard = */ false);

Would |alwaysShowMaps| be a better parameter name?

Comment 3

[Justin Lebar (not reading bugmail)]

> Is that right?

Yes.

> Would adding single reporters for "pss" and "swap" suffice?

Maybe?

I have a history of being overly optimistic about how useful the smaps data will prove.  But it's hard to argue that the full data won't be useful without first making it available for some time.

In particular, if we start aggressively sharing memory between processes, the full RSS and PSS breakdowns could be useful.

> Would |alwaysShowMaps| be a better parameter name?

Yes, I like that better.

Comment 4

[Nicholas Nethercote [inactive]]

> In particular, if we start aggressively sharing memory between processes,
> the full RSS and PSS breakdowns could be useful.

Sure, but you can always load in verbose mode...

Comment 5

[Justin Lebar (not reading bugmail)]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> > In particular, if we start aggressively sharing memory between processes,
> > the full RSS and PSS breakdowns could be useful.
> 
> Sure, but you can always load in verbose mode...

If we made "show verbose" not forget the fact that it had been showing a file/JSON dump, I'd be happy with that.  But that's a non-trivial change, I think.

Comment 6

[Nicholas Nethercote [inactive]]

Comment on attachment 668158 [details] [diff] [review]
Patch, v1

Alright, grumble grumble.

Comment 7

[Justin Lebar (not reading bugmail)]

> Would |alwaysShowMaps| be a better parameter name?

I went with |aForceShowSmaps|.

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8bda10a190c2