807181 - Report unique set size (USS) on Linux/B2G

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Justin Lebar (not reading bugmail)]

This would be useful for B2G.

Patch coming up from bug 802110.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 2

[Justin Lebar (not reading bugmail)]

This re-parses smaps, which is perhaps not great.  But the alternative would mean that we'd have to run the whole smaps multi-reporter in order to get this value, which I thought was worse.

Comment 3

[Nicholas Nethercote [inactive]]

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

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

It's a shame this can't be read from proc/<pid>/statm, the way resident and vsize can.  (I don't suppose uss is equal to |resident - share| from that file?)

Assuming it's not, can you add a comment explaining what you said in comment 2 - i.e.:

- Ideally this would be in the maps multi-reporter, because in this form it causes smaps to be parsed twice when all the memory reporters are run (e.g. when about:memory is loaded).
- But we need it to be a single reporter so that the USS-reading tool can get it as efficiently as possible.

::: xpcom/base/MapsMemoryReporter.cpp
@@ +516,5 @@
>  
> +static nsresult GetUSS(int64_t *n)
> +{
> +    FILE *f = fopen("/proc/self/smaps", "r");
> +    NS_ENSURE_STATE(f);

This leaves *n unfilled if it fails.  Is that ok?  Probably...

@@ +524,5 @@
> +    char line[256];
> +    while(fgets(line, sizeof(line), f)) {
> +        int64_t val = 0;
> +        if(sscanf(line, "Private_Dirty: %d kB", &val) == 1 ||
> +           sscanf(line, "Private_Clean: %d kB", &val) == 1) {

%d combined with int64_t doesn't sound right.  %lld?

@@ +525,5 @@
> +    while(fgets(line, sizeof(line), f)) {
> +        int64_t val = 0;
> +        if(sscanf(line, "Private_Dirty: %d kB", &val) == 1 ||
> +           sscanf(line, "Private_Clean: %d kB", &val) == 1) {
> +            total += int64_t(val) * 1024; // convert from kB to bytes

|val| doesn't need to be cast to int64_t, it already is one.

@@ +542,5 @@
> +    GetUSS,
> +    "Memory mapped by the process that is present in physical memory and not "
> +    "shared with any other processes.  This is also known as the process's "
> +    "unique set size (USS).  This is the amount of RAM we'd expect to be freed "
> +    "if we closed this process.")

Good description.

Comment 4

[Nicholas Nethercote [inactive]]

Hmm, is it worth having the full USS tree, like we have for RSS and PSS?

Comment 5

[Justin Lebar (not reading bugmail)]

> It's a shame this can't be read from proc/<pid>/statm, the way resident and vsize can.  (I don't 
> suppose uss is equal to |resident - share| from that file?)

Oh, that's a good idea!  But I must not be reading smaps or statm correctly, because I can't get the numbers to add up.

I have an smaps and statm from Firefox that I captured right after one another.  Their RSS'es match up within 1MB.  But statm reports shared == 11277 pages == 45108 kB.  While if I sum up all of the "Shared_*" entries in smaps, I get 23648 kB.

dhylands, do you have any idea what is going on here?

Comment 6

[Justin Lebar (not reading bugmail)]

Ah, the kernel docs explain better than the man page.  According to [1], the "shared" value in statm is "number of pages that are shared (i.e. backed by a file)".

OTOH I checked the kernel source and I'm pretty sure that smaps is telling us whether the mapping is /actually/ shared, i.e. more than one process has those physical pages mapped.

I'll see if I can write a patch for the man page.  I've always wanted to do that.  :)

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=Documentation/filesystems/proc.txt;h=a1793d670cd01bd374eddf54ffdfc768504291ff;hb=HEAD

Comment 7

[Justin Lebar (not reading bugmail)]

Patch sent to the man-pages list.  I'll update this patch soon.

Comment 8

[Dave Hylands [:dhylands]]

Apparently in 2.4 the 3rd field of statm was a count of pages in the mm whose page_count was more than 1, and 2.6 the 3rd field of statm corresponds to the total file-backed extent (whatever that means). I checked the kernel sources and it comes from a counter named MM_FILEPAGES

From what I can tell in the 2.6 kernel the "shared" column isn't particularly useful for anything.

I found a really good page which has some code for properly calculating USS
http://www.eqware.net/Articles/CapturingProcessMemoryUsageUnderLinux/index.html
This seems like it would give us the best answers.

Comment 9

[Justin Lebar (not reading bugmail)]

Although he alludes to the fact that smaps is somehow wrong, he never explains why.  I'm skeptical...

I suppose one way to test would be to compile his code and compare it to the result we calculate here.

Comment 10

[Justin Lebar (not reading bugmail)]

> I suppose one way to test would be to compile his code and compare it to the result we 
> calculate here.

I tried running these programs.  It looks like he takes a snapshot of each process's proc/maps file and then analyzes it in a second step.  Unfortunately I couldn't get that analysis step to work; it just outputted an empty file.

I'm not at all convinced this is doing something that smaps doesn't, and we already have evidence that smaps is correct (in bug 802110 comment 7, I checked that killing plugin-container on my desktop made uss increase by roughly the size of libxul), so I think I'm going to check this in as-is.

Comment 11

[Justin Lebar (not reading bugmail)]

> This leaves *n unfilled if it fails.  Is that ok?  Probably...

Our calling conventions (which I'm sure are written down somewhere, but I've never read myself) indicate that you shouldn't read an outparam if the function you're calling fails.  But better to avoid that footgun and initialize the outparam anyway.

> %d combined with int64_t doesn't sound right.  %lld?

Good catch.  I changed val to |long long|, because this is real sscanf, not this NSPR business which equates long long to int64.

https://hg.mozilla.org/integration/mozilla-inbound/rev/be0d3f1de9c2

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/be0d3f1de9c2

Comment 13

[Justin Lebar (not reading bugmail)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: Less insight into where memory is going in B2G
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): This is not particularly scary, and only has the potential to break people who open about:memory on Linux.
String or UUID changes made by this patch: none

Comment 14

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f17d8fafe331