Closed
Bug 458202
Opened 16 years ago
Closed 16 years ago
Speed up event handling
Categories
(Core :: DOM: Events, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files)
34.90 KB,
patch
|
jst
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
32.44 KB,
patch
|
Details | Diff | Splinter Review |
I have a patch which speeds up event handling up to over 20% when using http://mozilla.pettay.fi/moztests/events/event_speed_2.html as a testcase. Need to verify that all the tests pass etc. Will post the patch soon.
Assignee | ||
Comment 1•16 years ago
|
||
Assignee: nobody → Olli.Pettay
Assignee | ||
Comment 2•16 years ago
|
||
Comment on attachment 341447 [details] [diff] [review] passes mochitest, chrome and browser-chrome Some comments about the patch: >+ >+ static nsIJSContextStack* JSContextStack() { return sJSContextStack; } Don't use do_GetService(kJSStackContractID) every time nsCxPusher::Push is used. >- PRBool Push(nsISupports *aCurrentTarget); >+ PRBool Push(nsPIDOMEventTarget *aCurrentTarget); Change the type of the parameter, so that we don't have to QI. Push would anyway return PR_FALSE if aCurrentTarget wasn't nsPIDOMEventTarget >+ /** >+ * Whether or not nsPIDOMEventTarget::WillHandleEvent will be >+ * called. Default is PR_FALSE; >+ */ >+ PRPackedBool mWantsWillHandleEvent; nsPIDOMEventTarget::WillHandleEvent is a virtual method and needed currently only by nsHTMLFormElement. So having this flag saves plenty of virtual calls. > /** > * Parent item in the event target chain. > */ >- nsCOMPtr<nsISupports> mParentTarget; >+ nsPIDOMEventTarget* mParentTarget; No need to keep strong reference, because things are stable while we're creating event target chain, and chain itself has strong references to event targets. And using nsPIDOMEventTarget because everything in the event target chain must anyway implement that interface. > /** > * If the event needs to be retargeted, this is the event target, > * which should be used when the event is handled at mParentTarget. > */ >- nsCOMPtr<nsISupports> mEventTargetAtParent; >+ nsPIDOMEventTarget* mEventTargetAtParent; Same here. Event target chain has the strong ref. > NS_IMETHOD HandleEvent(nsPresContext* aPresContext, > nsEvent* aEvent, > nsIDOMEvent** aDOMEvent, >- nsISupports* aCurrentTarget, >+ nsPIDOMEventTarget* aCurrentTarget, > PRUint32 aFlags, > nsEventStatus* aEventStatus) = 0; Have to pass nsPIDOMEventTarget so that we don't have to QI before calling nsCxPusher::Push > nsCOMPtr<nsPIDOMEventTarget> mTarget; > nsEventTargetChainItem* mChild; > nsEventTargetChainItem* mParent; > PRUint16 mFlags; > PRUint16 mItemFlags; > nsCOMPtr<nsISupports> mItemData; > // Event retargeting must happen whenever mNewTarget is non-null. >- nsCOMPtr<nsISupports> mNewTarget; >+ nsCOMPtr<nsPIDOMEventTarget> mNewTarget; Use nsPIDOMEventTarget so that we don't have to QI >-nsEventTargetChainItem::nsEventTargetChainItem(nsISupports* aTarget, >+nsEventTargetChainItem::nsEventTargetChainItem(nsPIDOMEventTarget* aTarget, > nsEventTargetChainItem* aChild) > : mChild(aChild), mParent(nsnull), mFlags(0), mItemFlags(0) > { >- nsCOMPtr<nsPIDOMEventTarget> t = do_QueryInterface(aTarget); >- if (t) { >- mTarget = t->GetTargetForEventTargetChain(); >- } >+ mTarget = aTarget->GetTargetForEventTargetChain(); Because nsEventChainPreVisitor has now nsPIDOMEventTarget* member, no need to QI. > nsresult > nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor& aVisitor, PRUint32 aFlags, >- nsDispatchingCallback* aCallback) >+ nsDispatchingCallback* aCallback, >+ PRBool aMayHaveNewListenerManagers) This is a bit hackish, but the idea is that if we know that there isn't any new event listener managers, nsEventTargetChainItem::HandleEvent doesn't need to look at one if the current target didn't have a manager earlier. We need to check all the targets during capture/target phases, but after that we can be more restrictive in bubbling phase and when handling system event group. > if (NS_SUCCEEDED(rv)) { > // Event target chain is created. Handle the chain. > nsEventChainPostVisitor postVisitor(preVisitor); > rv = topEtci->HandleEventTargetChain(postVisitor, > NS_EVENT_FLAG_BUBBLE | > NS_EVENT_FLAG_CAPTURE, >- aCallback); >+ aCallback, >+ PR_TRUE); By default we do have "new" event listener managers. >- // nsCxPusher will push and pop (automatically) the current cx onto the >- // context stack >- nsCxPusher pusher; >- if (NS_SUCCEEDED(result) && pusher.Push(aCurrentTarget)) { >+ if (NS_SUCCEEDED(result)) { > // nsIDOMEvent::currentTarget is set in nsEventDispatcher. > result = aListener->HandleEvent(aDOMEvent); > } Use Push a bit earlier... > nsEventListenerManager::HandleEvent(nsPresContext* aPresContext, > nsEvent* aEvent, nsIDOMEvent** aDOMEvent, >- nsISupports* aCurrentTarget, >+ nsPIDOMEventTarget* aCurrentTarget, > PRUint32 aFlags, > nsEventStatus* aEventStatus) ... >+ // nsCxPusher will push and pop (automatically) the current cx onto the >+ // context stack >+ nsCxPusher pusher; >+ PRBool didPush = PR_FALSE; > while (iter.HasMore()) { > nsListenerStruct* ls = &iter.GetNext(); > PRBool useTypeInterface = > EVENT_TYPE_DATA_EQUALS(ls->mTypeData, typeData); > PRBool useGenericInterface = > (!useTypeInterface && ListenerCanHandle(ls, aEvent)); > // Don't fire the listener if it's been removed. > // Check that the phase is same in event and event listener. >@@ -1173,19 +1176,24 @@ found: > ls->mFlags & NS_PRIV_EVENT_UNTRUSTED_PERMITTED)) { > if (!*aDOMEvent) { > nsEventDispatcher::CreateEvent(aPresContext, aEvent, > EmptyString(), aDOMEvent); > } > if (*aDOMEvent) { > nsRefPtr<nsIDOMEventListener> kungFuDeathGrip = ls->mListener; > if (useTypeInterface) { >+ if (didPush) { >+ didPush = PR_FALSE; >+ pusher.Pop(); >+ } > DispatchToInterface(*aDOMEvent, ls->mListener, > dispData->method, *typeData->iid); >- } else if (useGenericInterface) { >+ } else if (useGenericInterface && >+ (didPush || (didPush = pusher.Push(aCurrentTarget)))) { > HandleEventSubType(ls, ls->mListener, *aDOMEvent, > aCurrentTarget, aFlags); > } ...here. So if there are several listeners for the same event, we don't have to push many times - unless there are legacy interface C++ listeners, which haven't ever had the Push().
Attachment #341447 -
Flags: superreview?(jst)
Attachment #341447 -
Flags: review?(jst)
Assignee | ||
Comment 3•16 years ago
|
||
And the perf improvement is even better if event targets have several listeners for an event. Very artificial testcase: http://mozilla.pettay.fi/moztests/events/event_speed_3.html over 30% faster with the patch.
Comment 4•16 years ago
|
||
Comment on attachment 341447 [details] [diff] [review] passes mochitest, chrome and browser-chrome - In nsContentUtils.h: static nsIThreadJSContextStack *sThreadJSContextStack; + + static nsIJSContextStack *sJSContextStack; No need to add sJSContextStack here, sThreadJSContextStack is the same thing, so just expose the existing one. r+sr=jst with that fixed.
Attachment #341447 -
Flags: superreview?(jst)
Attachment #341447 -
Flags: superreview+
Attachment #341447 -
Flags: review?(jst)
Attachment #341447 -
Flags: review+
Assignee | ||
Comment 5•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•