206321 - Share event listeners between XBL event handlers

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Peter Van der Beken [:peterv]]

Right now we create one event listener for every XBL event handler. This leads
to  bloat, especially with multiple key handlers per binding (see bug 148636).
There's several ways we can share event listeners. First of all we can share the
listener only for key handlers or for all handlers. Second we could share the
listener per binding or we could share them across bindings.
I tried implementing sharing for all listeners and per binding. However,
thinking about this it seems suboptimal, since I doubt we have many non-key
handlers per binding.

A shared listener can be uniquely identified by
  1) target
  2) event type
  3) use capture
  4) is command
Sharing listeners across bindings would probably mean having a mapping through a
hashtable (in the binding manager?) with those 4 properties as key. If we
implement the sharing per binding we can probably use an array since the target
is constant and there wouldn't be that many variations of 2, 3 and 4 per
binding. This does mean we end up with more listeners though.
Anyone have thoughts on this?

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: WIP

This makes us have one handler per nsXBLPrototypeHandler, except for key
handlers which have one handler per type/capture/eventgroup per
nsXBLPrototypeBinding (one per nsXBLPrototypeBinding in the most common case).
Reduces bloat and seems to mostly improve performance, but I need to verify my
testing.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Files that can be removed

Comment 3

[Brian Ryner (not reading)]

Comment on attachment 125043 [details] [diff] [review]
WIP

Sorry I haven't gotten to this yet... is the attachment description "WIP"
accurate, or do you think this is functionally complete?

Comment 4

[Peter Van der Beken [:peterv]]

I originally marked it WIP because I wasn't sure about the approach I took. I
still welcome comments on the ideas behind the patch, but I now feel more
confident that this is the right way, I've been running with the patch for a
while and so far I haven't run into regressions.
One thing that worries me is the removal of nsXBLEventHandler::MarkForDeath,
which used to null out mProtoHandler and mEventReceiver in
nsXBLBinding::ChangeDocument. I don't do that anymore, since mEventReceiver is
gone and mProtoHandler could be needed by another nsXBLBinding which uses the
same handler object.

Here's some performance test results (I haven't been able to get reliable
startup times).

New window open:
* without patch
avgOpenTime:593
minOpenTime:562
maxOpenTime:688
medOpenTime:572.5

* with patch
avgOpenTime:589
minOpenTime:551
maxOpenTime:687
medOpenTime:573

Pageload:
* without patch
Avg. Median : 1589 msec		Minimum     : 407 msec
Average     : 1880 msec		Maximum     : 7601 msec

* with patch
Avg. Median : 1581 msec		Minimum     : 396 msec
Average     : 1870 msec		Maximum     : 7313 msec

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This is great! I really want this. If this is implemented then we can stop
scrollbars from bubbling mouse events by just attaching the right <handler>s to
the <scrollbar> XBL, without bloating.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 125043 [details] [diff] [review]
WIP

>Index: src/nsXBLBinding.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLBinding.cpp,v
>retrieving revision 1.161
>diff -u -p -r1.161 nsXBLBinding.cpp
>--- src/nsXBLBinding.cpp	21 May 2003 03:14:47 -0000	1.161
>+++ src/nsXBLBinding.cpp	5 Jun 2003 22:36:47 -0000
>@@ -1083,10 +869,71 @@ nsXBLBinding::ExecuteDetachedHandler()
> NS_IMETHODIMP
> nsXBLBinding::UnhookEventHandlers()
> {
>-  if (mFirstHandler) {
>-    // Unhook our event handlers.
>-    mFirstHandler->RemoveEventHandlers();
>-    mFirstHandler = nsnull;
>+  nsXBLPrototypeHandler* handlerChain = mPrototypeBinding->GetPrototypeHandlers();
>+
>+  if (handlerChain) {
>+    nsCOMPtr<nsIDOMEventReceiver> receiver = do_QueryInterface(mBoundElement);
>+    nsCOMPtr<nsIDOM3EventTarget> target = do_QueryInterface(receiver);
>+    nsCOMPtr<nsIDOMEventGroup> systemEventGroup;
>+
>+    nsXBLPrototypeHandler* curr;
>+    for (curr = handlerChain; curr; curr = curr->GetNextHandler()) {
>+      nsXBLEventHandler* handler = curr->GetCachedEventHandler();
>+      if (handler) {
>+        nsCOMPtr<nsIAtom> eventAtom = curr->GetEventName();
>+        if (!eventAtom) 
>+          continue;
>+
>+        nsAutoString type;
>+        eventAtom->ToString(type);
>+
>+        // Figure out if we're using capturing or not.
>+        PRBool useCapture = (curr->GetPhase() == NS_PHASE_CAPTURING);
>+
>+        // If this is a command, remove it from the system event group, otherwise 
>+        // remove it from the standard event group.
>+
>+        // This is a weak ref. systemEventGroup above is already a
>+        // strong ref, so we are guaranteed it will not go away.
>+        nsIDOMEventGroup* eventGroup = nsnull;
>+        if (curr->GetType() & NS_HANDLER_TYPE_XBL_COMMAND) {
>+          if (!systemEventGroup)
>+            receiver->GetSystemEventGroup(getter_AddRefs(systemEventGroup));
>+          eventGroup = systemEventGroup;
>+        }
>+
>+        target->RemoveGroupedEventListener(type, handler, useCapture,
>+                                           eventGroup);
>+      }
>+    }

Won't the key handlers be in the list of prototype handlers?  So you need to
check that it's not a key handler before removing it here, like you do up in
InstallEventHandlers()?


>Index: src/nsXBLEventHandler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLEventHandler.cpp,v
>retrieving revision 1.62
>diff -u -p -r1.62 nsXBLEventHandler.cpp
>--- src/nsXBLEventHandler.cpp	24 Mar 2003 04:00:58 -0000	1.62
>+++ src/nsXBLEventHandler.cpp	5 Jun 2003 22:36:47 -0000
>@@ -40,127 +40,159 @@
>+nsresult
>+nsXBLEventHandler::HandleEvent(nsIDOMEvent* aEvent)

should be NS_IMETHODIMP, no?


> nsresult
>-nsXBLEventHandler::GetTextData(nsIContent *aParent, nsAString& aResult)
>+nsXBLKeyEventHandler::HandleEvent(nsIDOMEvent* aEvent)

Same here.

Looks good otherwise.  I don't anticipate any problems with the MarkForDeath()
change you did.  Thanks for cleaning this up!

Comment 7

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Comment 8

[Peter Van der Beken [:peterv]]

Comment on attachment 130647 [details] [diff] [review]
v1.1

Addressed bryner's comments.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It would be great if we can land this soon :-)

Comment 10

[Harshal Pradhan]

Looks like peterv already checked this in. 

Is this what made tinderbox leaks go up ?

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1063289340.19217.gz

Bloat/Leak Delta Report
--------------------------------------------------------------------------------------
Current file:  /mnt/4/tinderbox/brad/Linux_2.4.20abackupboy3_Clobber/bloat-cur.log
Previous file: /mnt/4/tinderbox/brad/Linux_2.4.20abackupboy3_Clobber/bloat-prev.log
----------------------------------------------leaks------leaks%------bloat------bloat%
TOTAL                                           584     121.21%   14755328     
-0.33%
--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsVoidArray                                      80          -
nsXBLKeyEventHandler                            240          -
TOTAL                                           320

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Fix leaks

Yeah, those leaks are mine, this patch should fix it but I'm still building to
verify. I was double-addrefing by creating into a raw pointer (1st addref) and
then calling AppendObject (2nd addref) and only releasing once (in the
destructor of the nsCOMArray).

Comment 12

[Harshal Pradhan]

Comment on attachment 131331 [details] [diff] [review]
Fix leaks

Makes sense and it does fix the problem (I checked.)

Comment 13

[Peter Van der Beken [:peterv]]

Checked in leak fix too (bonsai didn't record that checkin or the removal of
files from attachment 129309 [details] [diff] [review]).
mZ (and Z) went down by about 15k on Linux, about 6k on Windows. Brad also
recorded a reduction in Maximum Heap from 7.98M to 7.96M and a reduction in
Allocations from 234K to 230K. No idea if those are reliable numbers and real
reductions or not. Still, on the bloat test the number of nsXBLEventHandlers
went down from 1530 to 38 nsXBLEventHandlers and 10 nsXBLKeyEventHandlers so I'm
hoping for some good results for bug 148636.

Comment 14

[Aleksey Nogin]

This checkin have added a warning on brad TBox:

+content/xbl/src/nsXBLPrototypeBinding.cpp:1284
+ `nsXBLKeyEventHandler*handler' might be used uninitialized in this function