New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jQuery.event.fix() may force layout #1746

Closed
mgol opened this Issue Oct 21, 2014 · 18 comments

Comments

Projects
None yet
5 participants
@mgol
Member

mgol commented Oct 21, 2014

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

@mgol mgol added this to the Future milestone Oct 21, 2014

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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?

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Oct 21, 2014

Member

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...

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 30, 2015

Member

The trac issue referred to here is #1735 on github.

Member

timmywil commented Jan 30, 2015

The trac issue referred to here is #1735 on github.

@jbedard

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard May 23, 2015

Contributor

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?

Contributor

jbedard commented 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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 26, 2015

Member

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.

Member

dmethvin commented 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

This comment has been minimized.

Show comment
Hide comment
@jbedard

jbedard Jun 4, 2015

Contributor

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...

Contributor

jbedard commented 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

This comment has been minimized.

Show comment
Hide comment
@havenchyk

havenchyk Aug 5, 2015

We also noticed about "forced layout"/repaints in FF. Is there any progress regarding this issue?

havenchyk commented Aug 5, 2015

We also noticed about "forced layout"/repaints in FF. Is there any progress regarding this issue?

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Aug 5, 2015

Member

@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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Aug 5, 2015

Member

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.

Member

mgol commented 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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Aug 5, 2015

Member

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.

Member

dmethvin commented 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

This comment has been minimized.

Show comment
Hide comment
@havenchyk

havenchyk 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.fix and 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?

havenchyk commented 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.fix and 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

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Aug 5, 2015

Member

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?

Member

dmethvin commented 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

This comment has been minimized.

Show comment
Hide comment
@havenchyk

havenchyk Aug 7, 2015

@dmethvin there were problems with flexbox performance in FF in my case, sorry for disturbing.

havenchyk commented Aug 7, 2015

@dmethvin there were problems with flexbox performance in FF in my case, sorry for disturbing.

@dmethvin dmethvin closed this in e61fccb May 4, 2016

@dmethvin dmethvin modified the milestones: 3.0.0, Future May 6, 2016

@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.