Status

()

Core
DOM: Events
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

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

10 years ago
Created attachment 341447 [details] [diff] [review]
passes mochitest, chrome and browser-chrome
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 2

10 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

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

10 years ago
Created attachment 342052 [details] [diff] [review]
threadcontextstack
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Depends on: 460001
You need to log in before you can comment on or make changes to this bug.