Closed Bug 798024 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Details

Attachments

(1 file)

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.
Assignee: nobody → justin.lebar+bug
Summary: Don't hide smaps when displaying a memory report loaded from a file → Don't hide smaps when displaying a memory report dump loaded from a file or from the clipboard
Attached patch Patch, v1Splinter Review
Attachment #668158 - Flags: review?(n.nethercote)
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?
> 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.
> 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...
(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 on attachment 668158 [details] [diff] [review]
Patch, v1

Alright, grumble grumble.
Attachment #668158 - Flags: review?(n.nethercote) → review+
> Would |alwaysShowMaps| be a better parameter name?

I went with |aForceShowSmaps|.
https://hg.mozilla.org/mozilla-central/rev/8bda10a190c2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.