sinkEvents performance enhacements
John Labanca
This email addresses some proposed performance enhancements to GWT's sinkEvent mechanism that could lead to potentially breaking changes.
There a two related problems listed below, with a few proposed solutions that offer significant benefit with relatively little drawback. The solutions are not mutually exclusive, meaning we could implement all, none, or some combination of them.
Problem 1:
========
Reference Issue: Issue 1491
Most GWT Widgets sink all applicable events in their constructors even if the user never adds a listener for the event, resulting in wasted calls to onBrowserEvent() for events that users don't care about. For example, the Label Widget sinks ONCLICK, all MOUSEEVENTS, and ONMOUSEWHEEL in its constructor, but most uses of a Label widget will not require a ClickListener or MouseListener. That means that an average GWT application sinks lots of events on Labels that are never used, but still result in calls to onBrowserEvent.
Problem 2:
========
Reference Issue: Issue 908
The sinkEvents method is inefficient because it assigns a handler (or null) to events even if the assigned value hasn't changed. Since the cost of assigning events (even to null) in JavaScript is very expensive on most browsers, sinkEvents reduces performance dramatically by assigning far more event handlers than necessary.
Many Widgets are nested in a hierarchy, where each parent Widget sinks more events. This can result in 5+ calls to sinkEvents for a single Widget, where each call resinks the already sunk events and assigns unsunk events to null.
Solution 1: Only sink events as needed
============================
Joel suggests that we only sink events as needed. For example, we only sink mouse events on Labels when the user adds a MouseListener. This change allows us to reduce the overall number of events sunk in a GWT application, thus reducing the number of wasted calls to onBrowserEvent. Applications should see generally improved performance.
The one drawback that Joel points out is that onBrowserEvent will no longer receive the events unless the user adds a listener. For example, if a user creates a subclass of Label and adds an onBrowserEvent method, mouse events will not exist unless the user adds a MouseListener or manually sinks MOUSEEVENTS. This is a breaking change for users who currently rely on the events being sunk.
See issue 1491 for a more detailed explanation of this approach.
Solution 2: Add DOM methods to sink specific events
=======================================
Instead of using the sinkEvents method to sink events en mass, we can divide the work over multiple methods. For example, DOM.sinkOnMouseUp would only sink the ONMOUSEUP event, and DOM.sinkMouseEvents would sink all mouse events. This will improve efficiency because we will not be assigning the unhandled events to null, as we currently do. Also, we do not need to compare the ctor bits passed into sinkEvents for each event. Instead, we just sink all of the events described by the method.
The sinkEvents method will continue to exist as a depreciated method to support existing code.
Solution 3: Improve sinkEvents efficiency
==============================
22dsse proposed that we improve the sinkEvents method so that it only sinks or unsinks events that have changed. To do this, we first get a set of bits that represent the differences between the currently sunk events and the events that we ultimately want sunk. Then for each event, we check if their is a change, and we sink, unsink, or leave the event. This approach addresses the issue that sinkEvents currently assigns handlers (an expensive operation) to events even the handler already exists.
The algorithm that 22dsse describes has huge performance gains of 30%-70% for widgets that only sink a few events (less than half of the 17 supported) and for widgets that make repeated calls to the sinkEvents method, such as Widgets in a deep hierarchy where events or sunk in every parent's constructor.
This approach does require more comparisons, so there are some cases where efficiency actually decreases slightly. For example, sinkEvents would be slower for a widget that sinks all events in one shot because this approach adds extra overhead to prevent unnecessary handler assignment, but we actually want to set all handlers. However, there are very few Widgets that sink more than half of the events and only make one call to sinkEvents in their hierarchy, so this is a very rare scenario.
See issue 908 for a more detailed explanation of this approach.
Cumulative Solution:
===============
If you consider all three of the above solutions, we can realize huge performance gains in startup time and run time.
Solution 1 reduces the number of unused event handlers, but increases the number of calls to sinkEvents, the most inefficient part of the event process. When paired with Solution 3, subsequent calls to sinkEvents don't hinder performance, resulting in a gain all around. Solution two improves the performance of sinkEvents by seperating it into seperate functions, resulting in even more performance improvements.
--
Thanks,
John LaBanca
jlab...@google.com
Matthew Mastracci
> This email addresses some proposed performance enhancements to GWT's
> sinkEvent mechanism that could lead to potentially breaking changes.
>
> There a two related problems listed below, with a few proposed solutions
> that offer significant benefit with relatively little drawback. The
> solutions are not mutually exclusive, meaning we could implement all, none,
> or some combination of them.
Have you taken a look at the potential solution I posted a few days
ago? It turns out that most of the events can be sunk on the Window
object only and can use a manual bubbling step to correctly locate the
target(s) of the event. The runtime overhead is negligble, the code
size impact is negligible, but the event setup time is dramatically
improved. It may even turn out that we only need to sink mouseover
and mouseout (to get the correct relativeTarget/fromElement/toElement)
value and high-frequency events such as mousemove, mousewheel and
onscroll. As well, this means that we could replace sinkEvents (per
solution #2) and replace it with only a few specific event-handler-
hookup methods.
http://groups.google.com/group/Google-Web-Toolkit-Contributors/browse_frm/thread/7d88356239a99d22#
My prototype showed a remarkable difference in the amount of time
required for setting up events. I think that it would be most
effective in combination with the above solutions.
Emily Crutcher
Events.sinkMouseEvents(Element),
Events.unsinkMouseEvents(Element)
seems very clear and allows us to do whatever magic behind the scenes that we want to.
"There are only 10 types of people in the world: Those who understand binary, and those who don't"
John Labanca
Your suggest of using a global handler is certainly a good option, but we will need to do some research to determine its feasibility and performance gains. Since you already setup a demo, I'm guessing that this is a reasonable approach that doesn't require a major structural change. However, I'm sure there will be some issues related to events that don't bubble natively, and how to bubble events through the Widget hierarchy. We should probably get some details on how you implemented this to figure out if it is a viable solution, but I also think it would work in addition to the other proposals.
Miroslav Pokorny
What needs to be done before focusing on event sinking is redoing the
way widgets are assembled etc.
A few points above were made such as needlessly sinking events and
later one this is reversed etc. I believe the team needs to focus on
the lowest level in this case Widget,Panels and how they act and are
setup. After this some of your problems disappear and then you might
find that this sinking issue isnt as important as what was once
envisioned.
Miroslav Pokorny
should be moved to a new class. The Dom.eventXXX methods are really
ugly. The developer is forced to double qualify the event method they
are interested in once with the DOM class followed by a event and then
the actual method. SHould be collapsed from DOM.eventGetTarget to
Evente.getTarget.
--
mP
Miroslav Pokorny
but from a public api perspective the event methods should be moved
from the DOM class.
--
mP
Matt M
to window or document (according to MSDN docs and FF tests) so those
will certainly need to be hooked up on the element regardless.
In my prototype, the event bubbling implementation climbs the element
hierarchy from event.target -> document. By doing it through the HTML
hierarchy, it mimics how the events would propagate through the normal
bubbling implementation and ignores the widget hierarchy. This allows
us to change the event model without requiring any other client code
to change.
Note that the prototype doesn't take into account event cancellation
and multiple elements sinking the same event. These should be
reasonably straight-forward to add, however. It would also be better
to combine the event bubbling code with the event preview code, but
this wasn't done in the prototype code.
The prototype code is available at the link below. The important
modifications are within the standard DOM implementation's init()
method. The only other modification was the gutting of the sinkEvents
method.
http://www.box.net/shared/zh1cy9fg74
On Sep 26, 1:45 pm, "John Labanca" <jlaba...@google.com> wrote:
> Matthew -
>
> Your suggest of using a global handler is certainly a good option, but we
> will need to do some research to determine its feasibility and performance
> gains. Since you already setup a demo, I'm guessing that this is a
> reasonable approach that doesn't require a major structural change.
> However, I'm sure there will be some issues related to events that don't
> bubble natively, and how to bubble events through the Widget hierarchy. We
> should probably get some details on how you implemented this to figure out
> if it is a viable solution, but I also think it would work in addition to
> the other proposals.
>
> On 9/26/07, Emily Crutcher <e...@google.com> wrote:
>
>
>
>
>
> > I like solution two if we can create a separate class rather than using
> > DOM,as there will be quite a few new methods and DOM is already pretty big.
> > Events.sinkMouseEvents(Element),
> > Events.unsinkMouseEvents(Element)
> > seems very clear and allows us to do whatever magic behind the scenes that
> > we want to.
>
> > On 9/26/07, John Labanca < jlaba...@google.com> wrote:
>
> > > Contributers -
>
> > > This email addresses some proposed performance enhancements to GWT's
> > > sinkEvent mechanism that could lead to potentially breaking changes.
>
> > > There a two related problems listed below, with a few proposed solutions
> > > that offer significant benefit with relatively little drawback. The
> > > solutions are not mutually exclusive, meaning we could implement all, none,
> > > or some combination of them.
>
> > > Problem 1:
> > > ========
> > > Reference Issue: Issue 1491<http://code.google.com/p/google-web-toolkit/issues/detail?id=1491>
>
> > > Most GWT Widgets sink all applicable events in their constructors even
> > > if the user never adds a listener for the event, resulting in wasted calls
> > > to onBrowserEvent() for events that users don't care about. For example,
> > > the Label Widget sinks ONCLICK, all MOUSEEVENTS, and ONMOUSEWHEEL in its
> > > constructor, but most uses of a Label widget will not require a
> > > ClickListener or MouseListener. That means that an average GWT application
> > > sinks lots of events on Labels that are never used, but still result in
> > > calls to onBrowserEvent.
>
> > > Problem 2:
> > > ========
> > > Reference Issue: Issue 908<http://code.google.com/p/google-web-toolkit/issues/detail?id=908>
> > > divide the work over multiple methods. For example, DOM.sinkOnMouseUpwould only sink the ONMOUSEUP event, and
> > > jlaba...@google.com
>
> > --
> > "There are only 10 types of people in the world: Those who understand
> > binary, and those who don't"
>
> --
> Thanks,
> John LaBanca
Ian Petersen
> In my prototype, the event bubbling implementation climbs the element
> hierarchy from event.target -> document. By doing it through the HTML
> hierarchy, it mimics how the events would propagate through the normal
> bubbling implementation and ignores the widget hierarchy. This allows
> us to change the event model without requiring any other client code
> to change.
I'd like to throw a spanner in these works:
<div><label for="foo">Name:</label> <input type="text" id="foo" /></div>
If you click on the label element, the text box gets focussed, and I
think it gets a click event, too. I haven't done any testing to
confirm the click event, but it's a test case that needs to be covered
if you're going to re-implement event bubbling.
Ian
--
Tired of pop-ups, security holes, and spyware?
Try Firefox: http://www.getfirefox.com
mP
John Labanca
There are some pretty big performance issues for applications that use large numbers of Widgets, such as in Trees or Grids. I wouldn't say its a huge problem, but any measurable increase in startup times is always a good thing, especially if it makes the code cleaner.
Consider that Label and HTML elements always sink onmousemove events, so moving your cursor around the page results in many extraneous calls to onBrowserEvent for those elements that you cross.
Joel Webber
caused performance problems for a number of developers building
real-world apps (including us at Google).
It sounds like we're converging on a two-part solution:
1. Change the behavior of widgets to sink lazily only those events
which they actually need (i.e. in addFooListener(), etc). This will
eliminate a large number of calls to sinkEvents() for the reasons John
describes.
2. Optimize DOM.sinkEvents() (particularly on IE) such that it
minimizes the number of event property sets, which are known to be
surprisingly expensive on IE.
The structure suggested in issue 908 seems to do a good job of
handling (2). And it will become even more important if we lazily sink
events in widgets.
@mmastrac: I really like the creative solution you've come up with for
even further optimizations, but like John I have some concerns about
difficult-to-handle corner cases this might create. I don't want to
lose track of the idea, but I would like to get this simple change in
first, because I believe it will handle the 80% case.
I'm going to mark 908 and 1491 for the 1.5 release. If anyone has any
strong objections to the minor compatibility problems associated with
1491 (detailed in the issue and above), please speak now or forever
hold your peace :)
joel.
Matt M
> @mmastrac: I really like the creative solution you've come up with for
> even further optimizations, but like John I have some concerns about
> difficult-to-handle corner cases this might create. I don't want to
> lose track of the idea, but I would like to get this simple change in
> first, because I believe it will handle the 80% case.
Joel - that's fine. It's something that can be implemented later. I
think we'll see a big performance gain with what you have here. If
this solution does pan out as well, it'll be another big win for the
next version.
Once 908 and 1491 are in and tested, we can try out this other method
using a few events at a time (onclick, onmouseup, onmousedown). If it
works flawlessly for those events, we can try it out on keyboard
events, focus events and eventually try all of the events. If some
events can't be caught at the window level, those can be omitted and
continue to live at the element level. This can be done for the
standard browsers first, then ported over to the IE browsers.
Joel Webber
starting with the simple stuff you know will work, then moving on to
the more aggressive stuff.
joel.
Miroslav Pokorny
I did somehting similar to this when refactoring the base
Widge/Composite classes. I created a single class which holds all the
collections for all the basic listeners (mouse/key/focus etc). With
little effort it could be changed to lazily sink its bits and all sub
classes are auto updated. The current approach of every widget class
managing listeners is code duplication and stops this from being as
easy to do.
With this there is no need for any widget to have its own local
listener collection. Theoretically it could differ sinking bits until
that listener is registered.
This is yet another case to strengthen W + C and include all the
functionality that is typical of creating widgets. Lazily sinking bits
is a perfect thing that should be handled and done in one class which
all others leverage.
--
mP
Joel Webber
Miroslav Pokorny
> I definitely like the idea of doing this in a way that shares the code
> across widgets and makes it easier for everyone to follow the pattern.
>
> I'm a little wary of adding collections for all the basic listeners to every
> widget ( e.g. if every widget has a LoadListenerCollection field/instance,
> that would seem pretty heavy), when most only use a small set of them (but I
> may have misunderstood your suggestion). Can you give a rough idea of what
> this structure looks like in your code?
>
Whilst all classes would have fields for each listener collection,
there would only be instances for the collections that were actually
used. eg if the widget exposed keyboardListeners only that field would
have a KBLC with all the other listener fields set to null.
So yes there would be a LLC field, MouseL* field, FocusEventsL* field etc.
--
mP
Miroslav Pokorny
> On 10/2/07, Joel Webber <j...@google.com> wrote:
> > I definitely like the idea of doing this in a way that shares the code
> > across widgets and makes it easier for everyone to follow the pattern.
> >
> > I'm a little wary of adding collections for all the basic listeners to every
> > widget ( e.g. if every widget has a LoadListenerCollection field/instance,
> > that would seem pretty heavy), when most only use a small set of them (but I
> > may have misunderstood your suggestion). Can you give a rough idea of what
> > this structure looks like in your code?
> >
>
> Whilst all classes would have fields for each listener collection,
> there would only be instances for the collections that were actually
> used. eg if the widget exposed keyboardListeners only that field would
> have a KBLC with all the other listener fields set to null.
>
> So yes there would be a LLC field, MouseL* field, FocusEventsL* field etc.
>
The last issue would be who init the field.. I would suggest providing
protected addXXXListener and removeXXXListener for all the events. The
widget would then only have to decorate the appropriate add and remove
methods and increase their visilbilty to public.
The protected method could do the work of lazily initialising the collection.
eg
private MouseListenerCollection mouseListenerCollection;
protected MouseListenerCollection getMouseListenerCollection(){
return mouseListenerCollection
}
protected void setMouseListenerCollection( MouseListenerCollection
mouseListenerCollection ){
this.mouseListenerCollection=mouseListenerCollection;
}
protected void addMouseListenerCollection( final MouseListener mouseListener ){
MouseListenerCollection listeners = this.getMouseLIstenerCollection();
if( null == listeners ){
listeners = new MouseListenerCollections();
this.setMouseListenerCollection( listeners );
}
listeners.add( mouseListener );
}
etc . The sub class would only do a if the widget impl
SourcesMouseEvents ( or whatever the interface is called - you get the
idea).
public void addMouseListenerCollection( final MouseListener mouseListener ){
super.addMouseListenerCollection( mouseListener );
}
mP
type by reading DOM.eventGetType() and another (the yellow box) which
creates a wrapper of the appropriate type based on the event...
Eg if its a mousemove a MouseMoveEvent instance is returned and so on.
http://groups.google.com/group/Google-Web-Toolkit-Contributors/web/NewEvents.zip
Move the mouse aimless over the green box until its time vaule reaches
5 secs. Repeat for the yellow box. You will notice that the moves /
second average is virtually identical in both cases.
If you want to add a busy loop inside either feel free too.