Last Comment Bug 667468 - Add a dummy 'dom' memory reporter behind a compilation flag
: Add a dummy 'dom' memory reporter behind a compilation flag
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Mounir Lamouri (:mounir)
:
Mentors:
Depends on:
Blocks: 663271 667474
  Show dependency treegraph
 
Reported: 2011-06-27 08:29 PDT by Mounir Lamouri (:mounir)
Modified: 2011-09-20 05:02 PDT (History)
4 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (7.41 KB, patch)
2011-06-27 08:29 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (7.69 KB, patch)
2011-06-28 07:10 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v1.1 (7.55 KB, patch)
2011-06-28 09:24 PDT, Mounir Lamouri (:mounir)
n.nethercote: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2011-06-27 08:29:07 PDT
Created attachment 542161 [details] [diff] [review]
Patch v1

I was hoping to put this behind a preference but user prefs a read _after_ ::Init().
Comment 1 Mounir Lamouri (:mounir) 2011-06-27 08:33:18 PDT
Nicholas, is there a test suite for the memory reporter?
Comment 2 Boris Zbarsky [:bz] 2011-06-27 08:43:38 PDT
Why not just make this on by default?
Comment 3 Mounir Lamouri (:mounir) 2011-06-27 08:47:38 PDT
(In reply to comment #2)
> Why not just make this on by default?

Because I expect it to be really wrong at the beginning and I don't want users to use it. In the other hand, I don't want to have to maintain a dozen of patches in my queue.
I can just remove the flag as soon as I believe the value is correct enough.
Comment 4 Nicholas Nethercote [:njn] 2011-06-27 18:23:51 PDT
(In reply to comment #1)
> Nicholas, is there a test suite for the memory reporter?

The only test is this one:  toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul 

In the first part it runs each of the memory reporters (ie. calls GetMemoryUsed), but the results aren't checked because you can't check them because they're non-deterministic.  (If you can think of ways to check I'd be interested to hear.)

The second part is the majority of the checking, and it's more of a test for the presentation of about:memory.  It temporarily unregisters all the memory reporters and registers a bunch of fake, deterministic reporters.  Then it loads about:memory, and the output is checked.  Then the fake reporters are removed and the real reporters re-registered at the end.

Beware that nsIMemoryReporter is undergoing lots of changes right now: bug 664486, bug 666075, bug 653627.
Comment 5 Nicholas Nethercote [:njn] 2011-06-27 18:25:26 PDT
I should have said:  I don't see a good way to test the DOM reporters, other than what test_aboutmemory.xul already does in its first part -- ie. call GetMemoryUsed(), which at least will detect if the reporter crashes.
Comment 6 Nicholas Nethercote [:njn] 2011-06-27 19:08:19 PDT
Comment on attachment 542161 [details] [diff] [review]
Patch v1

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

::: dom/base/Makefile.in
@@ +111,5 @@
>  	nsContentPermissionHelper.cpp \
>  	nsStructuredCloneContainer.cpp \
>  	nsDOMNavigationTiming.cpp \
>  	nsPerformance.cpp	\
> +	nsDOMMemoryReporter.cpp \

Adding new files seems like overkill, though it is more reasonable if you are expecting nsDomMemoryReporter.{cpp,h} to become bigger as more additions are made.  Is that the case?

::: dom/base/nsDOMMemoryReporter.cpp
@@ +51,5 @@
> +  NS_RegisterMemoryReporter(new nsDOMMemoryReporter());
> +}
> +
> +NS_IMETHODIMP
> +nsDOMMemoryReporter::GetMemoryUsed(PRInt64* aMemoryUsed) {

It's weird having GetMemoryUsed() in the .cpp file and all the other Get*() functions in the .h file;  none of the other reporters do that.  I think you should move the other Get*() functions here, they don't need to be in a header file.

::: dom/base/nsDOMMemoryReporter.h
@@ +40,5 @@
> +
> +#include "nsIMemoryReporter.h"
> +
> +
> +class nsDOMMemoryReporter: public nsIMemoryReporter {

xpcom/base/nsIMemoryReporter.idl defines a macro NS_MEMORY_REPORTER_IMPLEMENT that simplifies the creation of simple memory reporters.  It's not appropriate for more complicated ones, but you could use it here;  maybe you're anticipating things will get more complicated in subsequent bugs?

@@ +54,5 @@
> +  NS_IMETHOD GetMemoryUsed(PRInt64* aMemoryUsed);
> +
> +private:
> +  // Protect ctor, use Init() instead.
> +  nsDOMMemoryReporter();

Why protect the constructor?  This looks different to all the other memory reporters, eg. StorageMemoryReporter in storage/src/mozStorageConnection.cpp.

::: layout/build/nsLayoutStatics.cpp
@@ +122,5 @@
>  #include "nsRefreshDriver.h"
>  
>  #include "nsHyphenationManager.h"
> +#include "nsDOMMemoryReporter.h"
> +#include "mozilla/Preferences.h"

I don't think you need mozilla/Preferences.h.

@@ +266,5 @@
>    nsFrameList::Init();
>  
>    NS_SealStaticAtomTable();
>  
> +#ifdef DOM_MEMORY_REPORTER

This needs an "XXX" or "TODO" comment pointing to bug 663271, and explaining that it's never true for normal builds.
Comment 7 Mounir Lamouri (:mounir) 2011-06-28 07:08:50 PDT
(In reply to comment #6)
> ::: dom/base/Makefile.in
> @@ +111,5 @@
> >  	nsContentPermissionHelper.cpp \
> >  	nsStructuredCloneContainer.cpp \
> >  	nsDOMNavigationTiming.cpp \
> >  	nsPerformance.cpp	\
> > +	nsDOMMemoryReporter.cpp \
> 
> Adding new files seems like overkill, though it is more reasonable if you
> are expecting nsDomMemoryReporter.{cpp,h} to become bigger as more additions
> are made.  Is that the case?

There is no real appropriate place. In addition, I prefer to create a new file instead of putting this in some arbitrary file with misc stuff.

> ::: dom/base/nsDOMMemoryReporter.cpp
> @@ +51,5 @@
> > +  NS_RegisterMemoryReporter(new nsDOMMemoryReporter());
> > +}
> > +
> > +NS_IMETHODIMP
> > +nsDOMMemoryReporter::GetMemoryUsed(PRInt64* aMemoryUsed) {
> 
> It's weird having GetMemoryUsed() in the .cpp file and all the other Get*()
> functions in the .h file;  none of the other reporters do that.  I think you
> should move the other Get*() functions here, they don't need to be in a
> header file.

The methods in the header are inlined. AFAIK, inline methods should be in the header. Though, I misplaced the 'inline' keyword.

> ::: dom/base/nsDOMMemoryReporter.h
> @@ +40,5 @@
> > +
> > +#include "nsIMemoryReporter.h"
> > +
> > +
> > +class nsDOMMemoryReporter: public nsIMemoryReporter {
> 
> xpcom/base/nsIMemoryReporter.idl defines a macro
> NS_MEMORY_REPORTER_IMPLEMENT that simplifies the creation of simple memory
> reporters.  It's not appropriate for more complicated ones, but you could
> use it here;  maybe you're anticipating things will get more complicated in
> subsequent bugs?

I did that on purpose. I don't want to use this macro (it is obfuscating the code) and if the DOM memory reporter reports more than one value (like content VS chrome), this class could be kept and transparently report different things.

> @@ +54,5 @@
> > +  NS_IMETHOD GetMemoryUsed(PRInt64* aMemoryUsed);
> > +
> > +private:
> > +  // Protect ctor, use Init() instead.
> > +  nsDOMMemoryReporter();
> 
> Why protect the constructor?  This looks different to all the other memory
> reporters, eg. StorageMemoryReporter in storage/src/mozStorageConnection.cpp.

Same as above. When nsDOMMemoryReporter will report multiple values, it will be easily doable and calling ::Init() will create all needed memory reporter. In addition, I don't think we want to create more than one memory reporter per type.
Comment 8 Mounir Lamouri (:mounir) 2011-06-28 07:10:35 PDT
Created attachment 542457 [details] [diff] [review]
Patch v1.1

Re-asking a review with a few changes (moved inline keyword and add a TODO comment).
Comment 9 Mounir Lamouri (:mounir) 2011-06-28 09:24:00 PDT
Created attachment 542500 [details] [diff] [review]
Patch v1.1
Comment 10 Nicholas Nethercote [:njn] 2011-06-28 17:12:38 PDT
> When nsDOMMemoryReporter will report multiple values, it will
> be easily doable and calling ::Init() will create all needed memory
> reporter. In addition, I don't think we want to create more than one memory
> reporter per type.

Trust me, you *really* want to look at bug 666075.  Instead of creating N reporters, you'll be able to create 1 reporter that makes N measurements.  See bug 661474 for an example.  It's fine to use nsIMemoryReporter for now, but nsIMemoryMultiReporter will make your life easier once it lands.
Comment 11 Nicholas Nethercote [:njn] 2011-06-28 17:15:25 PDT
Comment on attachment 542500 [details] [diff] [review]
Patch v1.1

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

r=me with the nit below fixed.

BTW, you'll have to make minor changes to account for bug 664291 landing.  No need to re-review for those.

::: dom/base/nsDOMMemoryReporter.h
@@ +59,5 @@
> +};
> +
> +/**
> + * Inline functions.
> + */

Repeating my comment from the first review:  these don't need to be inline.  They're not inlined in any other reporters, and perf isn't critical here.  Please move them into the .cpp next to GetMemoryUsed().
Comment 12 Mounir Lamouri (:mounir) 2011-06-29 08:42:59 PDT
http://hg.mozilla.org/mozilla-central/rev/6ecca1869718
Comment 13 Ioana (away) 2011-09-20 05:02:31 PDT
toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul (the test named by Nicholas in commnets #4 and #5) has passed on all the OSs:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420304&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420581&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420427&full=1
https://tbpl.mozilla.org/php/getParsedLog.php?id=6420534&full=1

Note You need to log in before you can comment on or make changes to this bug.