Closed
Bug 798024
Opened 10 years ago
Closed 10 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)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)
Details
Attachments
(1 file)
7.58 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → justin.lebar+bug
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #668158 -
Flags: review?(n.nethercote)
![]() |
||
Comment 2•10 years ago
|
||
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?
Assignee | ||
Comment 3•10 years ago
|
||
> 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•10 years ago
|
||
> 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...
Assignee | ||
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
Comment on attachment 668158 [details] [diff] [review] Patch, v1 Alright, grumble grumble.
Attachment #668158 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 7•10 years ago
|
||
> Would |alwaysShowMaps| be a better parameter name?
I went with |aForceShowSmaps|.
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8bda10a190c2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•