217124 - nsEventListenerManager::HandleEvent is unnecessarily bloated

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Brian Ryner (not reading)]

nsEventListenerManager::HandleEvent() contains a lot of redundant code for
dealing with different event types in the same way.  We can replace the mass of
code with some const data containing the mapping of event messages to methods on
the nsIDOM*Listener interfaces.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

Here's my idea for making things better.  It makes use of C++ pointer-to-member
types to store the offset for the listener interface method that each event
message corresponds to, and the bit flag for the listener struct.  I've tested
that this builds on Linux (gcc 3.2.2) and Windows (MSVC 6).

There was no measurable change on the pageload test from this (though I was
testing on a fast machine - something might show up on a slow box).  Here's the
effect on the static footprint:

before patch
-------------
code:
10266 bytes (nsEventListenerManager::HandleEvent)

data:
0 bytes

with patch
----------
code:
2548 bytes (14 DispatchnsIDOM* functions)
746 bytes (nsEventListenerManager::HandleEvent)
913 bytes (nsEventListenerManager::FixContextMenuEvent)

data:
1024 bytes (EventDispatchData arrays)
168 bytes (EventTypeData array)

total/+/-: 4207 bytes code (-6059 bytes)
	   1192 bytes data (+1192 bytes)

Comment 2

[Brian Ryner (not reading)]

Oops, should have mentioned that those size numbers were gotten via "readelf
-s", and that I was using gcc 3.2.2 (RedHat 9) with -O2.

Comment 3

[Brian Ryner (not reading)]

Attachment: alternative patch

So, if I bend the language rules a little more, I can eliminate the multiple
dispatch methods, saving a few more KB.  This patch makes the assumption that
the pointer-to-member implementations for nsIDOMEventListener and the
nsIDOMFooListener subclasses are interchangable.  If that is the case, then you
can take the pointer-to-member for the derived class and reinterpret_cast it to
a pointer-to-member for nsIDOMEventListener; take an nsIDOMEventListener* that
_really_ points to the derived class interface, and invoke the method.	(I'm
going to hell for this, I'm sure).  With this version of the patch, code size
is as follows:

759 bytes (HandleEvent)
128 bytes (DispatchToInterface)

The other sizes are the same as before; the 14 DispatchnsIDOM* functions are
gone.  Net savings over the original patch is 2407 bytes of code.  I've also
tested this on Linux (gcc 3.2 and 3.3 this time) and Windows.

I may make one more pass at this where I use XPTC_InvokeByIndex().  Certainly
using that, with the separate dispatch functions in the first revision of this
patch (replacing the pointer-to-members with method indices), I'd be fairly
sure I wasn't violating any C++ rules.	I could do that with a single dispatch
function as well, but it would require a bit of trickery to get at the right
nsISupports pointer in a generic way.  XPTC_InvokeByIndex may slow things down
too much though.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 130323 [details] [diff] [review]
alternative patch

+static nsresult DispatchToInterface(nsIDOMEvent* aEvent,
+				     nsIDOMEventListener* aListener,
+				     GenericHandler aMethod,
+				     const nsIID& aIID,
+				     PRBool* aHasInterface)
+{
+  nsIDOMEventListener* ifaceListener = nsnull;
+  nsresult rv = NS_OK;
+  aListener->QueryInterface(aIID, (void**) &ifaceListener);
+  if (ifaceListener) {
+    *aHasInterface = PR_TRUE;
+    rv = (ifaceListener->*aMethod)(aEvent);
+    NS_RELEASE(aListener);
+  }
+  return rv;
+}

This seems reasonable to me, though we probably should QI to
nsIDOMEventListener and make sure that ifaceListener really is the same pointer
that the QI to nsIDOMEventListener returns, if that's not the case, bail, and
assert like there's no tomorrow :-)

Oh, and can we eliminate the goto?

Comment 5

[Brian Ryner (not reading)]

> This seems reasonable to me, though we probably should QI to
> nsIDOMEventListener and make sure that ifaceListener really is the same pointer
> that the QI to nsIDOMEventListener returns, if that's not the case, bail, and
> assert like there's no tomorrow 

I'm not quite sure what you mean.  Keep in mind that ifaceListener is a
nsIDOMEventListener* but will really point to the nsIDOMKeyListener (for
example) interface on the object.  If you QI to nsIDOMEventListener, it will
give you back a different pointer, because then it will really be pointing to
the nsIDOMEventListener interface (probably 4 bytes less than ifaceListener).

> Oh, and can we eliminate the goto?

|goto| seems like the most efficient way to implement this, rather than setting
a flag that is checked for in the outer loop's condition.

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 130323 [details] [diff] [review]
alternative patch

Sorry, I wasn't thinking when I wrote my initial comment. This is pretty cool
now that I see what this is really doing :-). You're dealing with pointers to
abstract methods here, i.e. pointers to methods that don't really exist (since
they're pure virtual) :-) I didn't know you could do that in C++, but it works,
so cool. So my QI  rambling was bogus... This should be good as-is.

I'll try to review later today...

Comment 7

[Brian Ryner (not reading)]

Right. There's basically one somewhat-risky assumption that my second patch
makes -- that the compiler either does not care about the type of the pointer
used (other than type-checking) when invoking a pointer-to-member function, or
that the pointer-to-member representations are identical for nsIDOMKeyListener
and nsIDOMEventListener.  For example, they could both just be represented
internally as a vtable offset for the function we're calling.  Having said that,
I'd like to go with this patch if it doesn't break anything :)

The first patch, the more I think about it, should be pretty safe if the second
one is problematic.  The only assumption it's making is that reinterpret_cast
works as advertised - that is, if the cast compiles, you can reinterpret_cast to
some type, then back to the original type, and end up with the right thing.

(If you've got Stroustrup handy, you might want to read section 15.5, I found it
handy for coding this up).

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 130323 [details] [diff] [review]
alternative patch

- In nsEventListenerManager::FixContextMenuEvent()

+  nsCOMPtr<nsIDOMEventTarget> currentTarget ( aCurrentTarget );
...
+  if ( aEvent->message == NS_CONTEXTMENU_KEY ) {
...
+      if ( scriptObj ) {
...
+    if ( currentFocus ) {

What's with the extra spaces around the parens? I'd vote for taking those out
for consistecy's sake if nothing else...

+  if (NS_OK == ret) {

NS_SUCCEEDED(ret) please.

r=jst, let's hope no compilers we care about freak out when they see this :-)

Comment 9

[Brian Ryner (not reading)]

Oh, those were written by pinkerton, I just factored the function out of
HandleEvent.  I can go ahead and reformat it.

Comment 10

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

Comment on attachment 130323 [details] [diff] [review]
alternative patch

>+#define HANDLER(x) NS_REINTERPRET_CAST(GenericHandler, x)

You should be able to get away with NS_STATIC_CAST here, assuming all the real
base classes inherit from nsIDOMEventListener, since it's the reverse of a
standard conversion.

(I wish C++ allowed some standard conversions on the member type too.)

>+#define IMPL_EVENTTYPEDATA(type) \
>+{ \
>+  s##type##Events, \
>+  sizeof(s##type##Events) / sizeof(s##type##Events[0]), \

This could probably be:
  NS_ARRAY_LENGTH(s##type##Events), \

>+  &NS_GET_IID(nsIDOM##type##Listener) \
>+}

I think this approach should work with both common types of vtables.

sr=dbaron.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 130323 [details] [diff] [review]
alternative patch

>>+#define HANDLER(x) NS_REINTERPRET_CAST(GenericHandler, x)

> You should be able to > get away with NS_STATIC_CAST here, assuming all the real
base classes inherit from nsIDOMEventListener, since it's the reverse of a
standard conversion.

I think I tried that originally and MSVC complained.  At any rate, I'll test
again and change it if it compiles.

Comment 12

[Brian Ryner (not reading)]

Actually, I think that does call for REINTERPRET_CAST.  For pointer-to-member
functions, you can only "safely" cast from base -> derived (since the derived
class is guaranteed to have that function).  Casting from derived->base is not
allowed because the base class may _not_ have the function pointed to.

Comment 13

[Brian Ryner (not reading)]

checked in... i'll keep an eye on ports for bustage related to the pointer to
member casting, etc.

Comment 14

[Brian Ryner (not reading)]

ick, I think this may have caused a slight pageload time spike (btek, at least,
seems to be headed that way).  I'll analyze it once there's some more data.

Comment 15

[[e-mail]]

My MingW build chokes on typedef NS_IMETHOD_CALLBACK(nsIDOMEventListener,
GenericHandler)(nsIDOMEvent*);

c:/mozilla/content/events/src/nsEventListenerManager.cpp:100: parse error before
`)' token

Comment 16

[Brian Ryner (not reading)]

I have no idea what the expected placement of __stdcall is in this case.  Chris,
any ideas?

Comment 17

[Brian Ryner (not reading)]

Attachment: attempt to fix mingw bustage

Can someone check whether this fixes it?

Comment 18

[[e-mail]]

c:/mozilla/content/events/src/nsEventListenerManager.cpp:101: warning:
__stdcall__ attribute only applies to function types

No idea if it runs or not yet.

Comment 19

[cls]

I won't be able to personally test this for a few days but from my experiences
with http://bugzilla.mozilla.org/show_bug.cgi?id=134113#c163 , I don't believe
this is the proper fix if you want to actually use __stdcall with mingw.  I
think you'd have to declare the typedef as part of the nsIDOMEventListener class
declaration which probably isn't possible since that's generated from a .idl. 

I'll throw the patch on the tinderbox and we'll see what happens.

Comment 20

[[e-mail]]

OK, so with that patch it builds and runs!

Comment 21

[Brian Ryner (not reading)]

My gut feeling is that the warning is bogus.  To test that theory, change the
line to:

typedef nsresult (nsIDOMEventListener::*GenericHandler)(nsIDOMEvent *);

At least with MSVC, that causes a crash because it makes the call through the
pointer-to-method not use stdcall, while the interface methods on
nsIDOM*Listener are declared with stdcall.

Comment 22

[[e-mail]]

Yeah, that works fine too :-)

Comment 23

[cls]

Attachment: move typedef into class

This gets rid of the gcc warning in the same fashion that was used in bug
134113.  I'm still not completely certain if this is the correct behavior.

Comment 24

[[e-mail]]

Well it builds and runs...