smaps trees shouldn't show up where smaps isn't available

RESOLVED FIXED in mozilla9

Status

()

Toolkit
about:memory
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

unspecified
mozilla9
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

6 years ago
The RSS/vsize/swap breakdown trees appear on Mac and Windows, even though we have no data to fill them in with.  They simply shouldn't be there.

An easy way to fix this (and improve the experience on Linux at the same time) would be not to show completely empty smaps trees.  On Linux, this would mean not showing the "swap usage" tree most of the time, since swap is usually empty.

This might be confusing, because someone expecting Firefox to tell them how much swap is being used wouldn't even see the heading.  OTOH, in normal circumstances, it means that you don't have to click the header to discover that swap is empty.
(Assignee)

Updated

6 years ago
Depends on: 674290
IMO, the best behaviour is:
- Always show all three trees on Linux, even if empty.
- Never show the trees on other platforms.

(I've said before that having to click on the "swap" header to see that the tree is empty doesn't strike me as a problem.)

If that's too hard, then not showing empty trees on Linux is better than showing empty trees on other platforms.  But hopefully the best behaviour is possible.
(Assignee)

Comment 2

6 years ago
I'll see how hard it is to do OS detection from aboutmemory.js.
Failing that, is it possible to communicate "present-but-empty" somehow from MapsMemoryReporter.cpp?  E.g. just a single "maps/swap" reporter that has a zero value?
(Assignee)

Comment 4

6 years ago
Created attachment 560080 [details] [diff] [review]
Patch v1
Attachment #560080 - Flags: review?(khuey)
Comment on attachment 560080 [details] [diff] [review]
Patch v1

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

::: xpcom/base/MapsMemoryReporter.cpp
@@ +120,5 @@
> +  }
> +
> +  PRBool mSeenResident;
> +  PRBool mSeenVsize;
> +  PRBool mSeenSwap;

Use C++ bool please.

@@ +256,5 @@
>  MapsReporter::ParseMapping(
>    FILE *aFile,
>    nsIMemoryMultiReporterCallback *aCallback,
> +  nsISupports *aClosure,
> +  CategoriesSeen &aCategoriesSeen)

I know you fixed this in the other patch, but make this a pointer here.
Attachment #560080 - Flags: review?(khuey) → review+

Updated

6 years ago
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.