344986 - Simplify nsEventListenerManager

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Olli Pettay [:smaug][[e-mail]]]

I think nsEventListenerManager could be a bit simpler.
Current it has several kinds of lists for event listeners (and handling all those
lists makes the code to look quite ugly), but probably those could be merged
to one, since usually there aren't that many listeners in one node.
I hope that doesn't regress performance too badly.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Something like this

This makes nsListenerStruct bigger, but removes some members from nsEventListenerManager. Fixes bug 276846.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Something like this

Missed one file

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 229564 [details] [diff] [review]
Something like this

Couple of driveby comments just so I don't forget

>Index: widget/public/nsGUIEvent.h
>@@ -409,17 +410,17 @@ public:
...
>   // Additional type info for user defined events
>-  nsHashKey*  userType;
>+  nsCOMPtr<nsIAtom>     userType;

Why all the whitespace?

>Index: content/events/public/nsMutationEvent.h

> #define NS_MUTATION_ATTRMODIFIED                      (NS_MUTATION_START+5)
> #define NS_MUTATION_CHARACTERDATAMODIFIED             (NS_MUTATION_START+6)
>+#define NS_MUTATION_END                               (NS_MUTATION_START+6)

Shouldn't this be +7?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: + some small fixes

This is getting closer to something for a review.
Fixes bunch of bugs too: bug 276846, usage of nsIDOMMutationListener,
usage of nsIDOMLoadListener with bfcache...
Removes ~450 lines, though many lines are macro defs.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: ready for comments

This should be ready at least for comments.
I run dhtml tests 4 times with and without this patch and 3 times results 
were better with the patch.
The patch adds caching members to ELM, so that there is no need to search for
listeners every time.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 229665 [details] [diff] [review]
ready for comments

(also binary size decreases a bit with the patch)

I think this could be ready for a review. Imo  simplifies ELM (though someone might think something else..) and makes the code a bit more manageable. We really don't need that many macro definitions for all kinds of bits.

I think NS_MUTATION_END is right, it points to the last mutation event.

nsEvent::userType has that many spaces so that  it aligns with something, in this case with nsEvent::target.

nsListenerStruct is used so that if mEventType != 0 listener was registered using an event type, otherwise it was registered using IID.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Smaug: could you give a short overview of the changes in this patch? To aid reviewing.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Reminder for myself, give the overview of the patch!

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Finally some comments.

So this patch removes NS_EVENT_BITS_* and combines all event listeners to one 
list. nsListenerStruct is also changed so that it contains the event type, possible an atom for user defined events, or if a listener interface is used
for listener registration it contains a pointer to the EventTypeData.

::HandleEvent is the method which is called most often. sLatestEvent* were added to cache some search result so that there is no need to loop sEventTypes
every time. Similar kind of optimization is mNoListenerForEvent, which is set 
whenever event is tryed to be handled but there is no event listener for that.
This way ::HandleRelease can return early in most cases.
(HandleRelease is after called normally 4 times during each event dispatch)

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 229665 [details] [diff] [review]
ready for comments

>Index: content/events/src/nsEventListenerManager.cpp
>+#define EVENT_TYPE_EQUALS( ls, type, userType ) \
>+  (ls->mEventType && ls->mEventType == type && \
>+  (ls->mEventType != NS_USER_DEFINED_EVENT || ls->mTypeAtom == userType))

Should this be

#define EVENT_TYPE_EQUALS( ls, type, userType ) \
  (ls->mEventType == NS_USER_DEFINED_EVENT ? ls->mTypeAtom == userType : ls->mEventType == type)

?

>+nsEventListenerManager::AddEventListener(nsIDOMEventListener *aListener,

>+  nsListenerStruct* ls = nsnull;
>+  for (PRInt32 i = 0; i < mListeners->Count(); i++) {
>+    ls = NS_STATIC_CAST(nsListenerStruct*, mListeners->ElementAt(i));
>+    nsRefPtr<nsIDOMEventListener> listener = ls->mListener.Get();

Use nsCOMPtr, it'll give you some extra features that you'll probably want. Or does that assert?

Note to self, i'm on nsEventListenerManager::AddEventListener

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 229665 [details] [diff] [review]
ready for comments

>+nsEventListenerManager::AddEventListener(nsIDOMEventListener *aListener,
>+                                         PRUint32 aType,
>+                                         nsIAtom* aTypeAtom,
>+                                         const EventTypeData* aTypeData,
>                                          PRInt32 aFlags,
>                                          nsIDOMEventGroup* aEvtGrp)
> {
>   NS_ENSURE_TRUE(aListener, NS_ERROR_FAILURE);
>+  NS_ENSURE_TRUE((aType || aTypeData), NS_ERROR_FAILURE);

No need for the extra parens.

>+  if ((aType >= NS_MUTATION_START && aType <= NS_MUTATION_END) ||
>+      (ls->mTypeData && ls->mTypeData->iid &&
>+       ls->mTypeData->iid->Equals(NS_GET_IID(nsIDOMMutationListener)))) {
>+    mMayHaveMutationListeners = PR_TRUE;
>+    // Go from our target to the nearest enclosing DOM window.
>+    nsCOMPtr<nsPIDOMWindow> window;
>+    nsCOMPtr<nsIDocument> document;
>+    nsCOMPtr<nsINode> node(do_QueryInterface(mTarget));
>+    if (node){
>+      document = node->GetOwnerDoc();
>+    }
>+    if (document) {
>+      window = document->GetInnerWindow();
>+    } else {
>+      window = do_QueryInterface(mTarget);
>+    }

Nit: you could put the |if (document)| check inside the |if (node)| check since that's the only time it can pass.

Add a space before the '{' in |if (node){|


> nsEventListenerManager::RemoveEventListener(nsIDOMEventListener *aListener, 

>+  for (PRInt32 i = 0; i < mListeners->Count(); ++i) {

Cache the result of the ->Count() call (this may be a good thing in other loops).

>+    ls = NS_STATIC_CAST(nsListenerStruct*, mListeners->ElementAt(i));
>+    nsRefPtr<nsIDOMEventListener> listener = ls->mListener.Get();
>+    if (listener == aListener &&
>+        ls->mGroupFlags == group &&
>+        ((ls->mFlags & ~NS_PRIV_EVENT_UNTRUSTED_PERMITTED) == aFlags) &&
>+        (EVENT_TYPE_EQUALS(ls, aType, aUserType) ||
>+         (!(ls->mEventType) &&
>+          EVENT_TYPE_DATA_EQUALS(ls->mTypeData, aTypeData)))) {
>+      mListeners->RemoveElement((void*)ls);

RemoveElementAt(i) would be faster.

>+nsEventListenerManager::GetIdentifierForEvent(nsIAtom* aEvent)
>+{
>+  if (aEvent == nsLayoutAtoms::onmousedown)
>+    return NS_MOUSE_LEFT_BUTTON_DOWN;
>+  if (aEvent == nsLayoutAtoms::onmouseup)
>+    return NS_MOUSE_LEFT_BUTTON_UP;
...

It'd probably be faster (due to instruction cache) and cleaner to turn this into a table and a loop. Or it could be a hash even if it's called often enough.

>+nsEventListenerManager::HasMutationListeners(PRBool* aListener)
>+{
>+  *aListener = PR_FALSE;
>+  if (mMayHaveMutationListeners && mListeners) {
>+    PRInt32 count = mListeners->Count();
>+    for (PRInt32 i = 0; i < count; ++i) {
>+      nsListenerStruct* ls = NS_STATIC_CAST(nsListenerStruct*,
>+                                            mListeners->FastElementAt(i));
>+      if (ls &&
>+          (ls->mEventType >= NS_MUTATION_START &&
>+           ls->mEventType <= NS_MUTATION_END) ||
>+          (ls->mTypeData && ls->mTypeData->iid &&
>+           ls->mTypeData->iid->Equals(NS_GET_IID(nsIDOMMutationListener)))) {
>+        *aListener = PR_TRUE;

Add a break or return?

r=me with that.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comments, using hashtable

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 237346 [details] [diff] [review]
+comments, using hashtable

>+#define NS_SET_EVENT_ID(_event, _id) \
>+  success = (success && (gEventIdTable->Put(nsLayoutAtoms::on##_event, _id)))

You could make that sucess &&= gEventIdTable...

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 237346 [details] [diff] [review]
+comments, using hashtable

Jst, could you look at this?
Or at least say what you think about this kind of change.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 237346 [details] [diff] [review]
+comments, using hashtable

I'm all for a change like this! Some comments below...

- In nsEventListenerManager::GetTypeDataForEvent(nsIAtom *aEvent):

Both the method name and the argument name suggest that the argment that should be passed to this method is an event, where as it really is just the name of an event. Maybe rename this to GetTypeDataForEventName() and name the argument aName?

- In nsEventListenerManager::AddEventListener()

+  if (!mListeners) {
+    mListeners = new nsVoidArray();
+    NS_ENSURE_TRUE(mListeners, NS_ERROR_OUT_OF_MEMORY);
   }

I wonder what the most common case is here. I'd guess that it's really uncommon for an event listener manager to have *no* listeners in it. If that's true then we could make nsEventListenerManager::mListeners be a nsVoidArray instead of a pointer to one. Fewer mallocs. And also, what's the most common number of event listeners? If it's 1, then it might even make sense to use the type nsSmallVoidArray. To make this change we'd need to instrument this code a bit (and I'd be fine with doing that in a separate bug, of course).

+static void
+InitializeEventIdTable() {

That opening brace really needs to be on a line of its own.

- In nsEventListenerManager::AddEventListenerByType():
 {
-  PRInt32 subType;
-  EventArrayType arrayType;
   nsCOMPtr<nsIAtom> atom = do_GetAtom(NS_LITERAL_STRING("on") + aType);

Something to consider for another bug, but wouldn't it make more sense to no include the "on" in the atoms here? It's cheaper to strip out the on where we have a string starting with "on" than it it to always add it before atomizing etc.

In nsEventListenerManager.h, in the definition of nsListenerStruct:

+  PRUint32                                      mEventType;
+  nsCOMPtr<nsIAtom>                             mTypeAtom;
+  PRUint16                                      mFlags;
+  PRUint16                                      mGroupFlags;
+  PRBool                                        mHandlerIsString;
+  const EventTypeData*                          mTypeData;

If we *really* care, we could eliminate the separate mHandlerIsString and use the low bit of either mTypeAtom (in which case it couldn't be an nsCOMPtr any more) or maybe mTypeData, unless we can fit it into one of the flags memgers, that is...

sr=jst

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Since an nsVoidArray is just 4 bytes these days there's no point in using nsVoidArray* for this, even if it's common to have no listeners.

Using nsSmallVoidArray seems like a really good idea if there's often just 1 listener, but that'd require some more instrumentation. nsSmallVoidArray only has overhead in cykles these days, not in footprint, so it's likely a good idea.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: nsVoidArray* -> nsVoidArray

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Checked in, but this may have caused tp to go up a bit.
I'll look at the problem tomorrow and back out if necessary.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

I did some jproffing (IIRC, bz did too). I think the problem
is in ::HandleEvent. I'll post a new just slightly modified patch which reduces
iterations of mListeners and does some other small optimizations.
It should help, but whether it fixes the whole tp regression... dunno,
have to test with tbox.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Attachment: fixes to ::HandleEvent

Quite small changes to ELM::HandleEvent (and only to that method).
The main change is to move (useTypeInterface || useGenericInterface) to be before mListeners.IndexOf(ls) != -1. No need to test whether listener is 
actually still in mListeners, if the listener won't handle the event anyway.
Having (useTypeInterface || useGenericInterface) earlier saves also few other
things...
Using jprof and the performance testcase darin once posted to the newsgroup (tp2?) this seems to bring ::HandleEvent back to what it is originally.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 238091 [details] [diff] [review]
fixes to ::HandleEvent

There is some regression related to keyevents and menubar. Will re-ask review once I've solved that problem.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v5

The only change to the previous patch is in ::HandleEvent
    PRBool useTypeInterface =
+     ((ls->mEventType == NS_EVENT_TYPE_NULL) ||
+       ls->mEventType == aEvent->message) &&
     EVENT_TYPE_DATA_EQUALS(ls->mTypeData, typeData);

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 239481 [details] [diff] [review]
v5

argh, there is still something wrong. The old ELM is really way too complicated and undocumented.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v6

The problem I saw was that when using event type specific listeners (for example nsIDOMLoadListener) it doesn't matter what event type is actually used for registration ("load", "unload"); the listener should 
be in the listener list only once, even if registered many times using
different event type (event type in the same category).

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v8

The previous patch was missing mListenerRemoved = PR_TRUE;
in RemoveScriptEventListener.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

btw, I've run jprof with dhtml and tp2 tests and the latest patch shouldn't cause performance regressions.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 240222 [details] [diff] [review]
v8

looks good. r/sr=sicking

Comment 28

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This was actually a significant codesize increase -- not what I was expecting from the line count decrease.  All of the increase on prometheus-vm can be attributed to the size of a single function: InitializeEventIdTable.  Could that instead be written using an array and a loop?

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #28)
> This was actually a significant codesize increase -- not what I was expecting
> from the line count decrease.  All of the increase on prometheus-vm can be
> attributed to the size of a single function: InitializeEventIdTable.  Could
> that instead be written using an array and a loop?
> 

I was surprised to see the codesize increase.
I'll make a patch to change eventIdTable.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Attachment: reduces codesize a bit

Comment 31

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 241435 [details] [diff] [review]
reduces codesize a bit

>+  if (!gEventIdTable || !gEventIdTable->Init(NS_ARRAY_LENGTH(eventIdArray))) 

You probably want int(NS_ARRAY_LENGTH(eventIdArray) / 0.75) + 1.  See the comments in pldhash.h about PL_DHashTableSetAlphaBounds.

I also wonder whether you're better off using nsIAtom** and making the array static (although that does often require relocation at library load time -- although maybe less so these days with prelinking, etc.).

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +comments

Comment 33

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 241712 [details] [diff] [review]
+comments

r+sr=dbaron.  Might not be a bad idea to ask brendan (via email?) whether there ought to be a macro in pldhash.h for the 0.75 so we don't have to write it here as a magic number...

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, a macro that takes the number of entries and returns an appropriate size seems like a good idea.

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 241712 [details] [diff] [review]
+comments

Checked in, filed Bug 356116 for the macro.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Zdiff:-3280