Share event listeners between XBL event handlers

RESOLVED FIXED in mozilla1.6alpha

Status

()

P1
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

Trunk
mozilla1.6alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

16 years ago
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?
(Assignee)

Comment 1

16 years ago
Created attachment 125043 [details] [diff] [review]
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.
(Assignee)

Updated

16 years ago
Attachment #125043 - Flags: review?(jkeiser)
(Assignee)

Updated

15 years ago
Attachment #125043 - Flags: superreview?(bryner)
(Assignee)

Comment 2

15 years ago
Created attachment 129309 [details] [diff] [review]
Files that can be removed
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?
(Assignee)

Comment 4

15 years ago
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
Status: NEW → ASSIGNED
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 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!
Attachment #125043 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 7

15 years ago
Created attachment 130647 [details] [diff] [review]
v1.1
Attachment #125043 - Attachment is obsolete: true
(Assignee)

Comment 8

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

Addressed bryner's comments.
Attachment #130647 - Flags: superreview+
Attachment #130647 - Flags: review?(john)

Updated

15 years ago
Attachment #130647 - Flags: review?(john) → review+
(Assignee)

Updated

15 years ago
Priority: -- → P1
Target Milestone: --- → mozilla1.6alpha
(Assignee)

Updated

15 years ago
Attachment #125043 - Flags: review?(john)

Updated

15 years ago
Blocks: 215928
It would be great if we can land this soon :-)

Comment 10

15 years ago
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

(Assignee)

Comment 11

15 years ago
Created attachment 131331 [details] [diff] [review]
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).
(Assignee)

Updated

15 years ago
Attachment #131331 - Flags: superreview?(bryner)
Attachment #131331 - Flags: review?(keeda)

Comment 12

15 years ago
Comment on attachment 131331 [details] [diff] [review]
Fix leaks

Makes sense and it does fix the problem (I checked.)
Attachment #131331 - Flags: review?(keeda) → review+
Attachment #131331 - Flags: superreview?(bryner) → superreview+
(Assignee)

Comment 13

15 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 14

15 years ago
This checkin have added a warning on brad TBox:

+content/xbl/src/nsXBLPrototypeBinding.cpp:1284
+ `nsXBLKeyEventHandler*handler' might be used uninitialized in this function
You need to log in before you can comment on or make changes to this bug.