220228 - Improve nsEvent initialization

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Brian Ryner (not reading)]

Right now, many fields of nsEvent and its subclasses are not initialized
usefully when the object is constructed.  In particular, eventStructType should
be set automatically by the constructor.  Also, I think we should go ahead and
zero out all members.  I don't believe there will be a measurable speed hit, and
it will make the code cleaner and more robust.

Comment 1

[Brian Ryner (not reading)]

Attachment: patch

In addition to what I mentioned above, this patch:

- makes it possible to pass in the event message when the event is constructed
- adds NS_TEXT_TEXT as the message for an NS_TEXT_EVENT.  Having the same
constant double as the message and eventStructType is just asking for
confusion.  Similar changes for NS_COMPOSITION_* and NS_RECONVERSION_*.
- removes NS_DRAGDROP_EVENT as a struct type since it's not actually a unique
type of struct.
- factors out some checks in nsDOMEvent::SetEventType to hopefully make it a
bit more efficient

Comment 2

[Brian Ryner (not reading)]

It looks like there's a small increase in code size with this patch - 1396 bytes
of code in gklayout, and other much smaller amounts elsewhere.  I guess this is
probably from either members that weren't initialized before (in which case I
think not having uninitialized memory justifies the increase) or the fact that
we could be now zeroing members and then immediately setting them to something
else.  If that's the case, I may be able to make it better by allowing more
fields to be initialized via constructor parameters.

Comment 3

[Brian Ryner (not reading)]

-- also, this is more code than just using memset() to zero the struct, as a few
callers are doing now.  memset() is slower by quite a bit in my tests though...
for chunks of memory of this size, it looks like the function call overhead
outweighs any benefit you get from a block-zero operation.

Comment 4

[Brian Ryner (not reading)]

Attachment: patch v2

This is pretty much the same, but I made some changes specifically aimed at
reducing the code size bloat from the checkin.	In particular, I changed
several more call sites to use the new constructor form of nsEvent so that we
only initialize the message once.  Also, I factored part of nsDOMEvent's ctor
into a separate method since gcc3 seems to duplicate the code in constructors
(for the in-charge and not-in-charge versions).

There is still s small increase in code size here... about 1.4 KB total.  I
think some of that is definitely from initializing members that were left
uninitialized before, and I think that increase is worth it for that.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 132476 [details] [diff] [review]
patch v2

What's with the renaming of NS_TEXT_EVENT to NS_TEXT_TEXT? Seems like the old
name better described what it was...?

- In nsGUIEvent.h:

+struct nsInputEvent : public nsGUIEvent
...
+  /// PR_TRUE indicates the shift key is down
+  PRBool	   isShift;	   
+  /// PR_TRUE indicates the control key is down
+  PRBool	   isControl;	   
+  /// PR_TRUE indicates the alt key is down
+  PRBool	   isAlt;	   
+  /// PR_TRUE indicates the meta key is down (or, on Mac, the Command key)
+  PRBool	   isMeta;

Seems like those are just screaming to be PRPackedBool's :-)

+struct nsMouseEvent : public nsInputEvent
[...]
+  /// The number of mouse clicks
+  PRUint32	   clickCount;		
+  /// Special return code for MOUSE_ACTIVATE to signal
+  /// if the target accepts activation (1), or denies it (0)
+  PRBool	   acceptActivation;	       

Those could be squeezed into 32 bits too (PRPackedBool + PRUint16, or by using
24 bits for the PRUint32), if we care...

- In nsDOMEvent::AllocateEvent():

+  //Allocate internal event
+  nsAutoString eventType(aEventType);
+  if (eventType.EqualsIgnoreCase("MouseEvents")) {
+    mEvent = new nsMouseEvent();
+  }
+  else if (eventType.EqualsIgnoreCase("MouseScrollEvents")) {
+    mEvent = new nsMouseScrollEvent();
+  }
...

Would using NS_LITERAL_STRING()'s and nsCaseInsensitiveStringComparator() be
more code than using the nsAutoString temporary? If not, you should do that...

- In content/events/src/nsEventStateManager.cpp:

-  nsMouseEvent event;
-  event.eventStructType = NS_MOUSE_EVENT;
-  event.message = NS_CONTEXTMENU;
+  nsMouseEvent event(NS_CONTEXTMENU);
   event.widget = mEventDownWidget;
   event.clickCount = 1;
   event.point = mEventPoint;
   event.refPoint = mEventRefPoint;

Looks like most nsMouseEvents (and other UI events too?) often get their widget
and other members set after initialization. Would it not make sense to make the
ctor (or provide an alternate ctor) that takes the widget (and others) as an
argument to avoid setting members twice?

>Index: tools/codesighs/autosummary.unix.bash
>Index: tools/codesighs/basesummary.unix.bash

The changes to this file look unrelated to this change...

More later...

Note to self, continue after the nsGUIEvent.h changes.

Comment 6

[Brian Ryner (not reading)]

I'm not too concerned with making this struct smaller if there's any performance
penalty since we'll generally just have one or two of these on the stack at any
given time.  Does using PRPackedBool cause any sort of unaligned memory access
that would be slower?

I experimented with using NS_LITERAL_STRING/nsCaseInsensitiveStringComparator
and it turned out to be _much_ more code (I don't have the numbers handy).

Constructors that can initialize all of the members might be a good idea.  I'll
need to see what sorts of constructors would be most useful.

And yes, the codesighs changes are unrelated.

Comment 7

[Brian Ryner (not reading)]

Comment on attachment 132109 [details] [diff] [review]
patch

removing obsolete review request

Comment 8

[Brian Ryner (not reading)]

Comment on attachment 132476 [details] [diff] [review]
patch v2

removing to-be-obsolete review request

Comment 9

[Brian Ryner (not reading)]

Attachment: patch v2.1

This adds a widget parameter to the ctor for nsInputEvent and its subclasses,
and a target node parameter for nsMutationEvent.  It should be possible to add
the rest of the input and mouse event fields, but I'm not quite sure how I'd
want to do the nsPoint members (in particular, whether it's possible to have a
default value for a nsPoint& type, or if I'd need two forms of the constructor)
-- plus I'm not sure it's worth doing that.

As for changing to PRPackedBool and compacting things in general, I want to
make accessing the members as fast as possible.  Space is not a concern.  jst,
do you know for sure whether there's a performance cost to PRPackedBool?

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 133253 [details] [diff] [review]
patch v2.1

Very very nice! r/sr=jst

Comment 11

[Johnny Stenback (:jst)]

bryner, I forgot to mention... Accessing a PRPackedBool can be a bit slower on
some CPU's AFAIK, but I seriously doubt that you could measure even the sligtest
slowdown from making those members PRPackedBools. Initialization of an nsEvent
should be *faster* with PRPackedBools since member initialization of 4
PRPackedBoo's can be done with one 32-bit write, in stead of 4. But if you're
not worried about size, then I can go either way, but I'm leaning towards
favoring PRPackedBools here... your call.

Comment 12

[Brian Ryner (not reading)]

I just did some tests and no version of gcc collapses initialization of 4
|unsigned char| (PRPackedBool) types into a single instruction (x86).  It
instead generates 4 separate 'movb' instructions (x86, move byte).

However, if we instead used a bitfield, it will initialize it with a single
instruction.

Comment 13

[Christopher Blizzard (:blizzard)]

Comment on attachment 133253 [details] [diff] [review]
patch v2.1

Woot!  Thanks, bryner.

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 133253 [details] [diff] [review]
patch v2.1

>Index: content/events/public/nsMutationEvent.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/events/public/nsMutationEvent.h,v
>retrieving revision 1.2
>diff -u -p -r1.2 nsMutationEvent.h
>--- content/events/public/nsMutationEvent.h	27 Nov 2000 07:54:31 -0000	1.2
>+++ content/events/public/nsMutationEvent.h	14 Oct 2003 07:33:39 -0000
...
>+ * The Initial Developer of the Original Code is
>+ * IBM Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2003
>+ * the Initial Developer. All Rights Reserved.


Sorry, but I don't think IBM is the initial developer of this code and it would
be wrong to claim that.  It looks like Netscape is, AFAICT.

Comment 15

[Brian Ryner (not reading)]

Attachment: updated patch

merged to the trunk and fixed caillon's license comment (just a note: i've been
told multiple ways of handling source files that are missing licenses,
including just putting a license on it as if I was writing it anew.  we don't
have any guidelines for how to handle that situation)

One change over the previous patch is that I added a form of the
nsMutationEvent ctor that takes a |nsIContent*| and QI's it into mTarget.  This
removes a lot of code duplication.

Comment 16

[Brian Ryner (not reading)]

Checked in.

Comment 17

[timeless]

This broke beos (and mac which bryner fixed). Why didn't someone cc ports people?

Comment 18

[Sergei Dolgov]

People, i will be very gratefull if you add me in future to CC in all bugs which
change BeOS port code. 
At least in order to avoid red tree in bonsai.
But also to prevent me from shock looking at unknown statements in code i'm
working on.

Comment 19

[Sergei Dolgov]

This patch effectively killed BeOS port.
No more mouse, no more keyboard