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?

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 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?

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

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!

Comment on attachment 130647 [details] [diff] [review] v1.1 Addressed bryner's comments.

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

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

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 on attachment 131331 [details] [diff] [review] Fix leaks Makes sense and it does fix the problem (I checked.)

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.

This checkin have added a warning on brad TBox: +content/xbl/src/nsXBLPrototypeBinding.cpp:1284 + `nsXBLKeyEventHandler*handler' might be used uninitialized in this function