976024 - Track Unique Set Size in the Developer HUD

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Developer Tools, defect, P2)

Description

[Jan Keromnes [:janx]]

Unique Set Size is an important metric for app memory footprint. It is different from the Developer HUD's web-oriented "App memory", because USS is system-oriented, shows the total memory used by a gecko process, and is what's freed when the app is killed.

Comment 1

[Paul Rouget [:paul]]

Here is how to get this information in JS: https://gist.github.com/paulrouget/e59eb6dca3eeed87e46c

Not really tested.

Comment 2

[Jan Keromnes [:janx]]

Thanks Paul! I was planning on adding a quick `getUSS()` method to Panos' memory actor, using the `static int64_t ResidentUniqueFast()` method Nicolas is adding to the underlying `nsMemoryReporterManager` in bug 1014071.

Not really sure which approach is best. Yours looks easier and doesn't add the actors' crazy overhead in every gecko process. Nicolas, any thoughts on Paul's approach?

Comment 3

[Nicolas B. Pierron [:nbp]]

(In reply to Jan Keromnes [:janx] from comment #2)
> Not really sure which approach is best. Yours looks easier and doesn't add
> the actors' crazy overhead in every gecko process. Nicolas, any thoughts on
> Paul's approach?

I really admire the ability that people have to use a lot of features in tiny portions of code.
uss.js uses:
 - OS.File (instead of fopen)
 - Task (?!)
 - Generators (is that really needed? I do not think there is much overhead at reading /proc)
 - Regexp (parseInt(line.split(":")) should be enough)

The good thing which might be interesting is that we might want to profile the memory from the main process and not within the child processes.  In which case this is just a matter of having an extra argument to the function extracted in my patch.

Comment 4

[Paul Rouget [:paul]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> (In reply to Jan Keromnes [:janx] from comment #2)
> > Not really sure which approach is best. Yours looks easier and doesn't add
> > the actors' crazy overhead in every gecko process. Nicolas, any thoughts on
> > Paul's approach?
> 
> I really admire the ability that people have to use a lot of features in
> tiny portions of code.

I don't think we should use this code. It was just a quick draft proof of concept.

But for the sake of the argument: 1) is there anything better than OS.File? Because we spent a lot of time working on an easy and efficient way to read files, and the result is OS.File. 2) Task makes my life easier when I write async code. It's used almost everywhere in our code base, so chances that it's already loaded. 3) Generators? What's wrong with that? 4) regex: yep, we can do better. parseInt(split(":")[1]) will most probably do the work.

Am I wrong to assume that all the JS is nothing compared to reading a /proc/ file?

Comment 5

[[:fabrice] Fabrice Desré]

Using code that spawns the os.file worker to get memory usage is not a great idea. We need something very lean here. Just running this code will cause a 1MB memory usage in the process if the os.file worker was not already loaded.
c++ we need in this situation, or maybe js-ctypes...

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Paul Rouget [:paul] (slow to respond. Ping me on IRC) from comment #4)
> I don't think we should use this code. It was just a quick draft proof of
> concept.
> 
> But for the sake of the argument: 1) is there anything better than OS.File?
> Because we spent a lot of time working on an easy and efficient way to read
> files, and the result is OS.File. 2) Task makes my life easier when I write
> async code. It's used almost everywhere in our code base, so chances that
> it's already loaded. 3) Generators? What's wrong with that? 4) regex: yep,
> we can do better. parseInt(split(":")[1]) will most probably do the work.
> 
> Am I wrong to assume that all the JS is nothing compared to reading a /proc/
> file?

/proc/ files are faster to read than files stored on the flash / hard-drive, as even if there is a system call involve the file content is generated by the kernel.  So the I/O wait is just the time needed for the kernel to answer, and not the time needed for the hard drive to answer.

Yes, JS is not nothing, it is blocking the main thread.

3) Generators are not yet running in Baseline.

Comment 7

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21

Comment 8

[Jan Keromnes [:janx]]

Comment on attachment 8435801 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21

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

::: b2g/chrome/content/devtools.js
@@ +547,5 @@
>      let watch = this._watching;
>      let front = this._fronts.get(target);
>  
> +    if (watch.uss) {
> +      front.residentUnique().then(value => {

Why is `value` here the same as `data` after `front.measure()`? (i.e. an object with 'total', 'domSize', etc instead of the expected USS value)

Comment 9

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21

Comment 10

[Jan Keromnes [:janx]]

Attachment: gaia pull request

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8435840 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21

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

I'm fine with the JS changes but you really need a review from njn for the nsIMemoryReporter bits.

::: xpcom/base/nsIMemoryReporter.idl
@@ +345,5 @@
>    readonly attribute int64_t vsize;
>    readonly attribute int64_t vsizeMaxContiguous;
>    readonly attribute int64_t resident;
>    readonly attribute int64_t residentFast;
> +  readonly attribute int64_t residentUnique;

You need to change the uuid of the file, like in the patch I gave you :)

That's just how .idl works.

Also for those changes, you likely wants njn to do a review.

Comment 12

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21

Comment 13

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21, r=njn

Comment 14

[Jan Keromnes [:janx]]

Comment on attachment 8435927 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Vivien's r+. Nicholas already asked me in bug 1014071 to request his review on my idl change. I'll get around to do that when I feel more confident about this patch.

Comment 15

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21, r=njn

Comment 16

[Jan Keromnes [:janx]]

Comment on attachment 8435990 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

21's r+.

Comment 17

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21, r=njn

Comment 18

[Jan Keromnes [:janx]]

Comment on attachment 8436217 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Still 21's r+.

Comment 19

[Jan Keromnes [:janx]]

Comment on attachment 8436217 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Nicholas, could you please have a look at my changes to nsIMemoryReporter.idl and nsMemoryReporterManager.cpp?

Comment 20

[Jan Keromnes [:janx]]

Comment on attachment 8435841 [details] [review]
gaia pull request

Vivien please have a look :) also, have a good barbecue tomorrow!

Comment 21

[Nicholas Nethercote [inactive]]

Comment on attachment 8436217 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

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

::: xpcom/base/nsIMemoryReporter.idl
@@ +345,5 @@
>    readonly attribute int64_t vsize;
>    readonly attribute int64_t vsizeMaxContiguous;
>    readonly attribute int64_t resident;
>    readonly attribute int64_t residentFast;
> +  readonly attribute int64_t residentUnique;

Please update the comment above to mention this new attribute. "|residentUnique| (UNITS_BYTES)  The unique set size (a.k.a. USS)." would be a suitable description.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1832,5 @@
> +NS_IMETHODIMP
> +nsMemoryReporterManager::GetResidentUnique(int64_t* aAmount)
> +{
> +  *aAmount = ResidentUnique();
> +  return NS_OK;

I suspect you've only compiled this on Linux, and that you'll get compile errors on other platforms.

You need to follow the existing code more closely. Use |residentFast| as an example:

- Add a ResidentUniqueDistinguishedAmount() function for each platform. Put it just under the ResidentFastDistinguishedAmount() functions.

- Change ResidentUnique() so it's not Linux-only, and make it call ResidentUniqueDistinguishedAmount().

- You'll probably need to introduce a HAVE_RESIDENT_UNIQUE_REPORTER constant, similar to HAVE_VSIZE_AND_RESIDENT_REPORTERS.

There might be some additional changes required, but those items above should be enough to get you started.

Comment 22

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21, r=njn

Comment 23

[Jan Keromnes [:janx]]

Comment on attachment 8438760 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Vivien's r+.

Comment 24

[Jan Keromnes [:janx]]

Comment on attachment 8438760 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Thanks for reviewing, Nicholas!

(In reply to Nicholas Nethercote [:njn] from comment #21)
> > +  readonly attribute int64_t residentUnique;
> 
> Please update the comment above to mention this new attribute.
> "|residentUnique| (UNITS_BYTES)  The unique set size (a.k.a. USS)." would be
> a suitable description.

Done.

> ::: xpcom/base/nsMemoryReporterManager.cpp
> @@ +1832,5 @@
> > +NS_IMETHODIMP
> > +nsMemoryReporterManager::GetResidentUnique(int64_t* aAmount)
> > +{
> > +  *aAmount = ResidentUnique();
> > +  return NS_OK;
> 
> I suspect you've only compiled this on Linux, and that you'll get compile
> errors on other platforms.

I did. Sadly, I couldn't push this new patch to try either because `git bz` was in a bad mood.

> You need to follow the existing code more closely. Use |residentFast| as an
> example:
> 
> - Add a ResidentUniqueDistinguishedAmount() function for each platform. Put
> it just under the ResidentFastDistinguishedAmount() functions.

I did these changes, even though it changes Nicolas' code from bug #1014071, instead of simply exposing ResidentUnique to the IDL as was my plan originally.

> - Change ResidentUnique() so it's not Linux-only, and make it call
> ResidentUniqueDistinguishedAmount().

Done.

> - You'll probably need to introduce a HAVE_RESIDENT_UNIQUE_REPORTER
> constant, similar to HAVE_VSIZE_AND_RESIDENT_REPORTERS.

This constant was already there, so I just reused it.

Comment 25

[Nicholas Nethercote [inactive]]

Comment on attachment 8438760 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

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

Great! Thanks.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Hi Jan, do you have a Try link for this by chance? :)

Comment 27

[Jan Keromnes [:janx]]

Sure: https://tbpl.mozilla.org/?tree=Try&rev=d23174e7b688

Comment 28

[Jan Keromnes [:janx]]

Attachment: Track Unique Set Size in the Developer HUD. r=21

Comment 29

[Jan Keromnes [:janx]]

Comment on attachment 8439905 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21

Carrying over r+ from both Vivien and Nicholas.

Comment 30

[Jan Keromnes [:janx]]

Looks green to me https://tbpl.mozilla.org/?tree=Try&rev=d23174e7b688

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/b2g-inbound/rev/105ab6435043

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/105ab6435043