Closed Bug 458202 Opened 11 years ago Closed 11 years ago

Speed up event handling

Categories

(Core :: DOM: Events, defect)

x86
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files)

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: nobody → Olli.Pettay
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)
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 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+
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 460001
You need to log in before you can comment on or make changes to this bug.