The default bug view has changed. See this FAQ.

Add a dummy 'dom' memory reporter behind a compilation flag

VERIFIED FIXED in mozilla7

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 542161 [details] [diff] [review]
Patch v1

I was hoping to put this behind a preference but user prefs a read _after_ ::Init().
Attachment #542161 - Flags: review?(nnethercote)
(Assignee)

Comment 1

6 years ago
Nicholas, is there a test suite for the memory reporter?
Why not just make this on by default?
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 667474
(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.
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 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.
Attachment #542161 - Flags: review?(nnethercote)
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
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).
Attachment #542161 - Attachment is obsolete: true
Attachment #542457 - Flags: review?(nnethercote)
(Assignee)

Comment 9

6 years ago
Created attachment 542500 [details] [diff] [review]
Patch v1.1
Attachment #542457 - Attachment is obsolete: true
Attachment #542457 - Flags: review?(nnethercote)
Attachment #542500 - Flags: review?(nnethercote)
> 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 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().
Attachment #542500 - Flags: review?(nnethercote) → review+
(Assignee)

Updated

6 years ago
Flags: in-testsuite-
Whiteboard: [needs review] → [inbound]
(Assignee)

Comment 12

6 years ago
http://hg.mozilla.org/mozilla-central/rev/6ecca1869718
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7

Comment 13

6 years ago
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.