Closed Bug 976024 Opened 7 years ago Closed 6 years ago

Track Unique Set Size in the Developer HUD

Categories

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

All
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
2.0 S4 (20june)

People

(Reporter: janx, Assigned: janx)

References

Details

(Keywords: perf, Whiteboard: [c=devtools p= s=2014.06.20.t u=])

Attachments

(2 files, 7 obsolete files)

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.
Keywords: perf
Whiteboard: [c=devtools p= s= u=]
Priority: -- → P2
Depends on: 1014071
Here is how to get this information in JS: https://gist.github.com/paulrouget/e59eb6dca3eeed87e46c

Not really tested.
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?
Flags: needinfo?(nicolas.b.pierron)
(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.
Flags: needinfo?(nicolas.b.pierron)
(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?
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...
(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.
Summary: Add unique set size to "App memory" in the Developer HUD → Track Unique Set Size in the Developer HUD
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)
Attachment #8435801 - Attachment is obsolete: true
Attached file gaia pull request
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.
Attachment #8435840 - Flags: review+
Attachment #8435840 - Attachment is obsolete: true
Attachment #8435924 - Attachment is obsolete: true
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.
Attachment #8435927 - Flags: review+
Attachment #8435927 - Attachment is obsolete: true
Comment on attachment 8435990 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

21's r+.
Attachment #8435990 - Flags: review+
Attachment #8435990 - Attachment is obsolete: true
Comment on attachment 8436217 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Still 21's r+.
Attachment #8436217 - Flags: review+
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?
Attachment #8436217 - Flags: review?(n.nethercote)
Comment on attachment 8435841 [details] [review]
gaia pull request

Vivien please have a look :) also, have a good barbecue tomorrow!
Attachment #8435841 - Flags: review?(21)
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.
Attachment #8436217 - Flags: review?(n.nethercote) → review-
Attachment #8436217 - Attachment is obsolete: true
Comment on attachment 8438760 [details] [diff] [review]
Track Unique Set Size in the Developer HUD. r=21, r=njn

Vivien's r+.
Attachment #8438760 - Flags: review+
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.
Attachment #8438760 - Flags: review?(n.nethercote)
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.
Attachment #8438760 - Flags: review?(n.nethercote) → review+
Keywords: checkin-needed
Hi Jan, do you have a Try link for this by chance? :)
Keywords: checkin-needed
Attachment #8438760 - Attachment is obsolete: true
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.
Attachment #8439905 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/105ab6435043
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S4 (20june)
Whiteboard: [c=devtools p= s= u=] → [c=devtools p= s=2014.06.20.t u=]
Blocks: 989263
Component: Gaia::System → Developer Tools
You need to log in before you can comment on or make changes to this bug.