Closed Bug 991412 Opened 6 years ago Closed 5 years ago

provide static convenience method for retrieving RSS

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bkelly, Assigned: bkelly)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8401031 - Flags: review?(n.nethercote)
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.
Attachment #8401031 - Flags: review?(n.nethercote) → review+
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.
Blocks: 964096
Another way to do this is to make the relevant methods in the IDL [infallible].
(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?
(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.
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
Attachment #8401031 - Attachment is obsolete: true
Attachment #8402676 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1ad0bc421a4c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.