991412 - provide static convenience method for retrieving RSS

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While investigating transient memory issues its often helpful to print the current RSS value with instrumentation printf's.  This is currently possible by retrieving the nsMemoryReporterManager service and calling GetResidentFast(), but its a bit more work than necessary.  The RSS routines are completely stateless, so we could provide a more convenient static method to make this easier.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Provide static convenience method to retrieve RSS. (v1)

This is the patch I am using now.  I don't love the DirectGetResidentFast() method name, but I didn't spend a lot of time thinking of names either.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8401031 [details] [diff] [review]
Provide static convenience method to retrieve RSS. (v1)

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

r=me with nits.

::: xpcom/base/nsMemoryReporterManager.cpp
@@ +1498,5 @@
>  #endif
>  }
>  
> +/*static*/
> +int64_t nsMemoryReporterManager::DirectGetResidentFast()

Just call it ResidentFast().

::: xpcom/base/nsMemoryReporterManager.h
@@ +8,5 @@
>  #define nsMemoryReporterManager_h__
>  
>  #include "nsIMemoryReporter.h"
> +#include "nsITimer.h"
> +#include "nsServiceManagerUtils.h"

These don't look necessary.

@@ +155,5 @@
>      }
>    };
>    SizeOfTabFns mSizeOfTabFns;
>  
> +  static int64_t DirectGetResidentFast();

Please add a comment that this is present because it's useful for ad hoc profiling, so that some eager beaver won't remove it later because it's not used :)

Also, move the decl above the decl of |struct SizeOfTabFns|, so that it's just after the AmountFns, which are related.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Thanks for the quick review.  I'll post an updated patch with your requested changed later today.

About the includes, though:

(In reply to Nicholas Nethercote [:njn] from comment #2)
> ::: xpcom/base/nsMemoryReporterManager.h
> @@ +8,5 @@
> >  #define nsMemoryReporterManager_h__
> >  
> >  #include "nsIMemoryReporter.h"
> > +#include "nsITimer.h"
> > +#include "nsServiceManagerUtils.h"
> 
> These don't look necessary.

They are necessary to include in arbitrary other files, actually.  The nsMemoryReporterManager is getting some declarations due to luck where its currently used.  Its the same reason I had to add them to the patch in bug 964096.  For example, see bug 964096 comment 14.

If this lands I'll change the patch in bug 964096 to use this new static method.

Comment 4

[Nathan Froyd [:froydnj]]

Another way to do this is to make the relevant methods in the IDL [infallible].

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Nathan Froyd [AFK through Apirl 2nd] (:froydnj) from comment #4)
> Another way to do this is to make the relevant methods in the IDL
> [infallible].

I think this will get us the int64_t returned directly instead of using the out parameter, but I don't think it will make the method static, will it?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Ben Kelly [:bkelly] from comment #5)
> (In reply to Nathan Froyd [AFK through Apirl 2nd] (:froydnj) from comment #4)
> > Another way to do this is to make the relevant methods in the IDL
> > [infallible].
> 
> I think this will get us the int64_t returned directly instead of using the
> out parameter, but I don't think it will make the method static, will it?

Ah, right.  No, it won't.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Provide static convenience method to retrieve RSS. (v2)

Updated patch addressing review feedback.  Carried the r+ forward.

Note, I verified again that I need those headers.  If you include nsMemoryReporterManager.h into arbitrary C++ files you'll get the following compilation errors without the added includes.

Without the nsITimer.h include I get this:

/srv/mozilla-central/xpcom/base/nsMemoryReporterManager.h:213:   instantiated from here
../../dist/include/nsCOMPtr.h:572: error: no matching function for call to 'nsCOMPtr_base::nsCOMPtr_base(nsITimer*&)'
../../dist/include/nsCOMPtr.h:438: note: candidates are: nsCOMPtr_base::nsCOMPtr_base(nsISupports*) (base constructor)
../../dist/include/nsCOMPtr.h:435: note:                 nsCOMPtr_base::nsCOMPtr_base(const nsCOMPtr_base&) (base constructor)

Without the nsServiceManagerUtils.h include I get this:

/srv/mozilla-central/xpcom/base/nsMemoryReporterManager.h:38: error: 'do_GetService' was not declared in this scope

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Pushed to mozilla-inbound:

  https://hg.mozilla.org/integration/mozilla-inbound/rev/1ad0bc421a4c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1ad0bc421a4c