Repository navigation
jQuery.event.fix() may force layout #1746
Description
Originally reported by anonymous at: http://bugs.jquery.com/ticket/14147
Accessing some of the event properties while copying them (like jQuery.event.fix() does in event[ prop ] = originalEvent[ prop ] at https://github.com/jquery/jquery/blob/master/src/event.js#L511) may force style recalculation and layout.
Screenshots of Timeline in Chrome Developer Tools: http://i.imgur.com/Zuh6WDE.png http://i.imgur.com/JWLYF2N.png
Issue reported for jQuery 1.10.2
Activity
mgol commentedon Oct 21, 2014
Comment author: dmethvin
I could certainly believe this happens for events like mousemove where we populate and normalize the x/y position in our Event object. Browsers may defer doing that work until something asks for the position, and in this case that something is jQuery. Not sure it can be avoided though if we want this information to be in our object.
Adding paul.irish for his insight.
mgol commentedon Oct 21, 2014
Comment author: dmethvin
(In #14164) It's blocked by an open ticket, but it is probably better to handle the others on a case-by-case basis rather than a meta-ticket.
mgol commentedon Oct 21, 2014
Comment author: jbedard
I recently ran across this issue as well, which lead me here.
How about using ES5 getters to delay the copying/calculating of event properties? Would that be an option for 2.0?
Something similar to what was previously discussed on the forum and more recently the original post author blogged about a solution.
mgol commentedon Oct 21, 2014
Comment author: dmethvin
The problem is that ES5 getters are (still) slow in just about every browser. There are also some bugs and quirks, even in our list of 2.x-supported browsers. https://bugzilla.mozilla.org/show_bug.cgi?id=626021
It's true that if you never get any event properties, or perhaps only get one property one time, it can be faster. But in the case of a mousemove event for example, I suspect it's going to be extremely common that the caller's own event handler will want that information and would have forced the layout anyway.
Additionally, the layout is forced here only if you enter with a "dirty" layout from some other DOM change caused by a previous event or handler.
mgol commentedon Oct 21, 2014
Comment author: jbedard
ES5 getters are definitely slow still, but after trying this out I think this might still be worth it.
Don't you think most event handlers only access a few event properties? Yet the $.event.fix loop accesses them all. The fix loop is often a bottleneck on its own without a forced layout.
The scenario I've seen this bug occur a few times is toggling a class on mouse enter/leave. So this bug forces a layout, then the event listener simply toggles a class based on the event.type which sets the dirty flag again. In this case that extra layout is completely unnecessary, even the $.event.fix loop copying everything was unnecessary.
The only problem would be the ES5 getters. But at least in FF/chrome when only calling those getters 0-10 times it seems worth it.
Would you be interested in a pull request to at least see it in action?
mgol commentedon Oct 21, 2014
Comment author: dmethvin
I think this could be examined as part of #12031 which I wanted to tackle for 1.12/2.2. There are some issues to be decided there, but potentially you could just use the native event object for 2.x and skip "fixing" entirely.
mgol commentedon Oct 21, 2014
Comment author: jbedard
If #12031 provides a new event interface which skips $.event.fix that would be great, especially if it still provides things such as event namespaces & selectors.
But if anyone cares to look at what I tried: https://github.com/jbedard/jquery/commit/fd617744638ae16299c10cbcdb2935e3f45c5f27
This turns all hooks into es5 getters instead of filters and prop copying. I think the es5 setter is actually slower then just calling the getter over and over (needs testing). This avoids the forced layout and removes the $.event.fix loop, but does make accessing the properties more expensive due to the getter. In the cases I tried this was well worth it but I assume the change is too big/risky for such a stable API...
timmywil commentedon Jan 30, 2015
The trac issue referred to here is #1735 on github.
jbedard commentedon May 23, 2015
I've updated the change in https://github.com/jbedard/jquery/tree/event_properties.
Originally I was using the event speed test but that has been deleted. I'll try to get a jsperf test going. Probably something which simulates mouse events with different handlers that access different properties? @dmethvin did you have any specific type of test you'd like me to try?
dmethvin commentedon May 26, 2015
Yeah, a realistic test is going to be messy here. I guess it might be possible to make a layout change via modifying a class, then fire a MouseEvent for mousemove that goes through our plumbing.
jbedard commentedon Jun 4, 2015
Here's a first attempt: https://jsperf.com/jquery-event-props
Note that is the latest from master without the change ("old") and with ("new").
But I think the class attribute change isn't forcing a relayout, otherwise I think the offsetX test should be much slower then the button test. I'm also not seeing a relayout it in the chrome timeline.
Let me know what you think. We'll probably want a lot more test cases such as handlers that do more then access 0-1 event properties...
havenchyk commentedon Aug 5, 2015
We also noticed about "forced layout"/repaints in FF. Is there any progress regarding this issue?
mgol commentedon Aug 5, 2015
@havenchyk Not yet. This issue has a "Future" milestone as we're currently focusing on finishing 3.0.0 and there's still a lot to do. Additional perf tests, pull requests with tests confirming they improve performance could make it possible to make 3.0.0 but I wouldn't hold hopes high otherwise.
mgol commentedon Aug 5, 2015
BTW, the issue @dmethvin linked to, https://bugzilla.mozilla.org/show_bug.cgi?id=626021 is now fixed so maybe getters are not such a huge problem anymore. But still, PRs welcome, especially with perf tests.
dmethvin commentedon Aug 5, 2015
I think this problem on the jQuery side would be fixed by the solution proposed by @jbedard in #2337, at least for jQuery's own event handling code. The original problem is that the layout is dirty coming into the handler. Any attempt in your own code's event handler to read properties that depend on layout will force layout as well. Any high-frequency handler such as mousemove or scroll should defer changes to a requestAnimationFrame handler if possible to throttle the change rate and avoid this kind of problem.
havenchyk commentedon Aug 5, 2015
@dmethvin do you mean that the problem on my side as well? I've disabled all the event handlers in my app and timeline shows me that the most time is taken by
jQuery.event.fixand it triggers layout and only in the Firefox (Chrome 43 and IE11 work smooth).I will be glad to help to determine, if it's really the problem of jQuery. Can I help with something there?
dmethvin commentedon Aug 5, 2015
If you don't have any active event handlers, jQuery shouldn't have an event handler attached either. Can you create a test case at jsbin.com?
havenchyk commentedon Aug 7, 2015
@dmethvin there were problems with flexbox performance in FF in my case, sorry for disturbing.
Event: Remove fixHooks, propHooks; switch to ES5 getter with addProp
Event: Remove fixHooks, propHooks; switch to ES5 getter with addProp