Closed Bug 904720 Opened 6 years ago Closed 6 years ago

Add a count of event listeners

Categories

(Toolkit :: about:memory, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox26 --- affected

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(2 files, 3 obsolete files)

We may want to grow some sort of census tree since we can't display this information in the explicit tree right now :-/
Attachment #789707 - Flags: review?(justin.lebar+bug)
Comment on attachment 789707 [details] [diff] [review]
0014-Bug-XXXXXX-Add-reports-of-event-listener-counts-to-a.patch

>diff --git a/content/base/src/nsDocument.cpp b/content/base/src/nsDocument.cpp
>--- a/content/base/src/nsDocument.cpp
>+++ b/content/base/src/nsDocument.cpp

>+  // XXXkhuey the const_cast is ugly, but we're not passing false for
>+  // "creation allowed" so it's ok.  Reason # 14382143 why boolean parameters
>+  // are terrible.
>+  if (nsEventListenerManager* elm =
>+        const_cast<nsIDocument*>(this)->GetListenerManager(false)) {
>+    aWindowSizes->mEventListenerCount += elm->ListenerCount();
>+  }

Nit: To appease my sense of cleanliness, could you take out the "XXXkhuey"?
Bonus points if you do

  GetListenerManager(/* creationAllowed */ false);

here and elsewhere.

>diff --git a/dom/base/nsWindowMemoryReporter.cpp b/dom/base/nsWindowMemoryReporter.cpp
>--- a/dom/base/nsWindowMemoryReporter.cpp
>+++ b/dom/base/nsWindowMemoryReporter.cpp

>@@ -210,16 +210,18 @@ CollectWindowReports(nsGlobalWindow *aWi
>          "Memory used by the comment nodes in a window's DOM.");
>   aWindowTotalSizes->mDOMCommentNodes += windowSizes.mDOMCommentNodes;
> 
>   REPORT("/dom/event-targets", windowSizes.mDOMEventTargets,
>          "Memory used by the event targets table in a window's DOM, and the "
>          "objects it points to, which include XHRs.");
>   aWindowTotalSizes->mDOMEventTargets += windowSizes.mDOMEventTargets;
> 
>+  aWindowTotalSizes->mEventListenerCount += windowSizes.mEventListenerCount;
>+
>   REPORT("/dom/other", windowSizes.mDOMOther,
>          "Memory used by a window's DOM that isn't measured by the other "
>          "'dom/' numbers.");
>   aWindowTotalSizes->mDOMOther += windowSizes.mDOMOther;
> 
>   REPORT("/property-tables",
>          windowSizes.mPropertyTables,
>          "Memory used for the property tables within a window.");

>@@ -364,16 +366,26 @@ nsWindowMemoryReporter::CollectReports(n
>     nsresult rv;                                                              \
>     rv = aCb->Callback(EmptyCString(), NS_LITERAL_CSTRING(_path),             \
>                        nsIMemoryReporter::KIND_OTHER,                         \
>                        nsIMemoryReporter::UNITS_BYTES, _amount,               \
>                        NS_LITERAL_CSTRING(_desc), aClosure);                  \
>     NS_ENSURE_SUCCESS(rv, rv);                                                \
>   } while (0)
> 
>+#define REPORT_COUNT(_path, _amount, _desc)                                   \
>+  do {                                                                        \
>+    nsresult rv;                                                              \
>+    rv = aCb->Callback(EmptyCString(), NS_LITERAL_CSTRING(_path),             \
>+                       nsIMemoryReporter::KIND_OTHER,                         \
>+                       nsIMemoryReporter::UNITS_COUNT, _amount,               \
>+                       NS_LITERAL_CSTRING(_desc), aClosure);                  \
>+    NS_ENSURE_SUCCESS(rv, rv);                                                \
>+  } while (0)
>+

>+  REPORT_COUNT("/dom-event-listeners", windowTotalSizes.mEventListenerCount,
>+               "This is the sum of all windows' 'dom/event-listener' numbers.");

"Absolute paths" don't begin with "/".

It looks like you forgot to add the per-window reporter, or maybe you tried to
add it under explicit, but couldn't.  I think we want the per-window numbers,
otherwise leaks become pretty difficult to find.

Similarly, would it make sense to separate out event listeners more?  At the
very least, we could separate them into window/document/node event listeners.

I like the idea of having a "census" tree.  It's not clear to me if all
reporters that count things (as opposed to reporters which count events, like
the page-fault reporters) would go in there, or what.  But we already have a
bunch of counting reporters (js-compartments, content-parents) that we might be
able to put into a "census" tree, and we're thinking about adding more (e.g.
counts for observers and prefs).

We don't have to do it in this bug, though.

This looks good, but I'd like to have a look at the per-window memory
reporters.
Attachment #789707 - Flags: review?(justin.lebar+bug)
And please include sample output from about:memory.  It makes it much easier to review new memory reporters.
Attached file example output (obsolete) —
So I decided to scope creep this and implement the census tree.  Here's what the output looks like.
Attachment #789707 - Attachment is obsolete: true
Attached patch Patch (obsolete) — Splinter Review
Attachment #804061 - Flags: review?(n.nethercote)
Comment on attachment 804061 [details] [diff] [review]
Patch

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

As we discussed on IRC, I'm uncomfortable with a couple of things.

- Duplicating the measurements (bytes and counts) of various DOM nodes is a bit odd.  Potentially useful, but not something we've done before.

- Adding different kinds of dom things (e.g. text nodes and event listeners) doesn't make much sense.

The original motivation for this bug was to detect event listener leaks, right?  I'd be happy with a patch that just has an "event-listener-census" tree;  it's the other bits that I'm not happy with.  (I've thought before about augmenting bytes measurement with counts, e.g. for all the JS gc-things.  I'll give that some more thought.)
Attachment #804061 - Flags: review?(n.nethercote) → review-
Comment on attachment 804061 [details] [diff] [review]
Patch

I assume a simpler patch is coming.
Attachment #804061 - Flags: review?(bugs)
Attached patch PatchSplinter Review
I dropped everything but event-listeners and event-targets.
Attachment #804061 - Attachment is obsolete: true
Attachment #804593 - Flags: review?(n.nethercote)
Comment on attachment 804593 [details] [diff] [review]
Patch


>-SizeOfEventTargetObjectsEntryExcludingThisFun(
>+static PLDHashOperator
>+EventTargetObjectsEnumeratorFunc(
Hmm, a bit vague name. Sure, event targets are enumerated, but that is rather obvious when dealing with
mEventTargetObjects.
Perhaps CollectDETHSizeAndListenerCount
(or without DETH)

>   nsPtrHashKey<nsDOMEventTargetHelper> *aEntry,
>-  MallocSizeOf aMallocSizeOf,
>   void *arg)
> {
>-  nsISupports *supports = aEntry->GetKey();
>-  nsCOMPtr<nsISizeOfEventTarget> iface = do_QueryInterface(supports);
>-  return iface ? iface->SizeOfEventTargetIncludingThis(aMallocSizeOf) : 0;
>+  auto windowSizes = static_cast<nsWindowSizes*>(arg);
Please, don't use auto. It just makes the code harder to read (in this case).

>+
>+  nsDOMEventTargetHelper *et = aEntry->GetKey();
* goes with the type

>+
>+#define REPORT_COUNT(_pathTail, _amount, _desc)                               \
>+  do {                                                                        \
>+    if (_amount > 0) {                                                        \
>+        nsAutoCString path(censusWindowPath);                                 \
>+        path += _pathTail;                                                    \
>+        nsresult rv;                                                          \
>+        rv = aCb->Callback(EmptyCString(), path, nsIMemoryReporter::KIND_OTHER,\
>+                      nsIMemoryReporter::UNITS_COUNT, _amount,                \
>+                      NS_LITERAL_CSTRING(_desc), aClosure);                   \
>+        NS_ENSURE_SUCCESS(rv, rv);                                            \
>+    }                                                                         \
>+  } while (0)
>+
I see some other code around here using 4 space indentation, but 2 is correct, and the
file isn't consistent, so 2 please.


>+  REPORT_COUNT("/dom/event-targets", windowSizes.mDOMEventTargetsCount,
>+               "Number of event targets in the event targets table in a "
>+               "window's DOM, such as XHRs.");
Somehow the comment should make it clear that it isn't about DOM nodes
Attachment #804593 - Flags: review?(bugs) → review+
Comment on attachment 804593 [details] [diff] [review]
Patch

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

Summing event-listeners and event-targets still doesn't completely make sense, but they're close enough that I'll let it slide.  "census/" is too general for the tree prefix, though;  how about "events-census" or "event-counts"?

And, as mentioned below, please double-check everything to make sure you don't double-count, because DMD won't detect it if you do -- it only knows about things measured with malloc_size_of.

::: content/base/src/nsDocument.cpp
@@ +11089,5 @@
>        mExtraPropertyTables[i]->SizeOfExcludingThis(aWindowSizes->mMallocSizeOf);
>    }
>  
> +  if (nsEventListenerManager* elm = GetExistingListenerManager()) {
> +    aWindowSizes->mDOMEventListenersCount += elm->ListenerCount();

|aWindowSizes| is no longer accurate... |aWindowAmounts|?

@@ +11156,5 @@
>      *p += nodeSize;
> +
> +    if (nsEventListenerManager* elm = node->GetExistingListenerManager()) {
> +      aWindowSizes->mDOMEventListenersCount += elm->ListenerCount();
> +    }

You did the same thing in nsIDocument::DocSizeOfExcludingThis() above.  Are both necessary, or does this double-count?

::: dom/base/nsGlobalWindow.cpp
@@ +11543,5 @@
>      mNavigator ?
>        mNavigator->SizeOfIncludingThis(aWindowSizes->mMallocSizeOf) : 0;
>  
> +  aWindowSizes->mDOMEventTargetsSize +=
> +    mEventTargetObjects.SizeOfExcludingThis(nullptr,

Comment here that you use |nullptr| because the things pointed to by the entries are measured by the call to EnumerateEntries() below.
Attachment #804593 - Flags: review?(n.nethercote) → review+
(In reply to Olli Pettay [:smaug] from comment #9)
> Comment on attachment 804593 [details] [diff] [review]
> Patch
> 
> 
> >-SizeOfEventTargetObjectsEntryExcludingThisFun(
> >+static PLDHashOperator
> >+EventTargetObjectsEnumeratorFunc(
> Hmm, a bit vague name. Sure, event targets are enumerated, but that is
> rather obvious when dealing with
> mEventTargetObjects.
> Perhaps CollectDETHSizeAndListenerCount
> (or without DETH)

Done.

> >   nsPtrHashKey<nsDOMEventTargetHelper> *aEntry,
> >-  MallocSizeOf aMallocSizeOf,
> >   void *arg)
> > {
> >-  nsISupports *supports = aEntry->GetKey();
> >-  nsCOMPtr<nsISizeOfEventTarget> iface = do_QueryInterface(supports);
> >-  return iface ? iface->SizeOfEventTargetIncludingThis(aMallocSizeOf) : 0;
> >+  auto windowSizes = static_cast<nsWindowSizes*>(arg);
> Please, don't use auto. It just makes the code harder to read (in this case).

I disagree, since the type name is in the cast, but ok.

> >+
> >+  nsDOMEventTargetHelper *et = aEntry->GetKey();
> * goes with the type

Fixed.

> >+
> >+#define REPORT_COUNT(_pathTail, _amount, _desc)                               \
> >+  do {                                                                        \
> >+    if (_amount > 0) {                                                        \
> >+        nsAutoCString path(censusWindowPath);                                 \
> >+        path += _pathTail;                                                    \
> >+        nsresult rv;                                                          \
> >+        rv = aCb->Callback(EmptyCString(), path, nsIMemoryReporter::KIND_OTHER,\
> >+                      nsIMemoryReporter::UNITS_COUNT, _amount,                \
> >+                      NS_LITERAL_CSTRING(_desc), aClosure);                   \
> >+        NS_ENSURE_SUCCESS(rv, rv);                                            \
> >+    }                                                                         \
> >+  } while (0)
> >+
> I see some other code around here using 4 space indentation, but 2 is
> correct, and the
> file isn't consistent, so 2 please.

Fixed.

> 
> >+  REPORT_COUNT("/dom/event-targets", windowSizes.mDOMEventTargetsCount,
> >+               "Number of event targets in the event targets table in a "
> >+               "window's DOM, such as XHRs.");
> Somehow the comment should make it clear that it isn't about DOM nodes

Done.
(In reply to Nicholas Nethercote [:njn] from comment #10)
> Comment on attachment 804593 [details] [diff] [review]
> Patch
> 
> Review of attachment 804593 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Summing event-listeners and event-targets still doesn't completely make
> sense, but they're close enough that I'll let it slide.  "census/" is too
> general for the tree prefix, though;  how about "events-census" or
> "event-counts"?

I went with event-counts.

> And, as mentioned below, please double-check everything to make sure you
> don't double-count, because DMD won't detect it if you do -- it only knows
> about things measured with malloc_size_of.
> 
> ::: content/base/src/nsDocument.cpp
> @@ +11089,5 @@
> >        mExtraPropertyTables[i]->SizeOfExcludingThis(aWindowSizes->mMallocSizeOf);
> >    }
> >  
> > +  if (nsEventListenerManager* elm = GetExistingListenerManager()) {
> > +    aWindowSizes->mDOMEventListenersCount += elm->ListenerCount();
> 
> |aWindowSizes| is no longer accurate... |aWindowAmounts|?

I don't want to change these/rename the struct in this bug.  Happy to file a followup.

> @@ +11156,5 @@
> >      *p += nodeSize;
> > +
> > +    if (nsEventListenerManager* elm = node->GetExistingListenerManager()) {
> > +      aWindowSizes->mDOMEventListenersCount += elm->ListenerCount();
> > +    }
> 
> You did the same thing in nsIDocument::DocSizeOfExcludingThis() above.  Are
> both necessary, or does this double-count?

They are both necessary, and they are not the same thing.  The one in nsIDocument is counting the event listeners attached to the document itself.  The one in nsDocument is counting the event listeners attached to each node in the document.

> ::: dom/base/nsGlobalWindow.cpp
> @@ +11543,5 @@
> >      mNavigator ?
> >        mNavigator->SizeOfIncludingThis(aWindowSizes->mMallocSizeOf) : 0;
> >  
> > +  aWindowSizes->mDOMEventTargetsSize +=
> > +    mEventTargetObjects.SizeOfExcludingThis(nullptr,
> 
> Comment here that you use |nullptr| because the things pointed to by the
> entries are measured by the call to EnumerateEntries() below.

Done.
MemShrink needs this for 1.2.
blocking-b2g: --- → koi+
Kyle,

Has there been any progress here? This is a koi+.
Flags: needinfo?(khuey)
Yeah, this really shouldn't be koi+.  It should just land with approval when it's ready.
blocking-b2g: koi+ → ---
Flags: needinfo?(khuey)
Comment on attachment 804593 [details] [diff] [review]
Patch

Approved for 1.2.
Attachment #804593 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/43b42fb3dc19
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Target Milestone: mozilla28 → mozilla27
Depends on: 937227
Depends on: 957021
You need to log in before you can comment on or make changes to this bug.