Make event dispatch optimizable by engines #2834
Conversation
|
By analyzing the blame information on this pull request, we identified @dmethvin, @timmywil and @gibson042 to be potential reviewers |
|
Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA). 📝 Please visit http://contribute.jquery.org/CLA/ to sign. After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know. If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check. |
|
Nice. I know that has been mentioned in https://github.com/petkaantonov/bluebird/wiki/Optimization-killers, do you have some perf data? |
topaxi
force-pushed
the
optimizable-event-dispatch
branch
from
0dfba7b to
94ac452
Compare
- Do not assign to function parameters - Do not pass arguments object to other functions
topaxi
force-pushed
the
optimizable-event-dispatch
branch
from
94ac452 to
247589e
Compare
|
No real perf data yet. Can look into it tomorrow :) |
|
So I'm not sure how to build actual meaningful perf benchmarks. While in the devtools the Chrome Profile: Firefox Profile: |
topaxi
force-pushed
the
optimizable-event-dispatch
branch
from
900cae0 to
76e710d
Compare
|
I really, really hate this kind of hacks. What articles like https://github.com/petkaantonov/bluebird/wiki/Optimization-killers forget to tell you, is that you need to improve performance only those parts of code that are slow, but not those parts that could be optimized - if you can, doesn't mean you should do... the definition of premature opmitization. Like what if we do this - increase the size, introduce additional complexity, all that. But the next day, new Chrome comes out and with it, new v8 and with it, new Crankshaft/TurboFan, which do the optimization for arguments value but in the same time... new FF comes out and with it, new spiderMonkey and with it, new IonMonkey/ So now we left with increased size, convoluted logic and without any speed improvements in Chrome but with decreased speed in FF. Bluebird is the most obfuscated, weird and unsupported code there is on JS, pretty much no one understand it, except for its author... and that for lib that doesn't have to be that fast because it deals with async operations anyways, just like jQuery doesn't have to that fast because it deals with DOM which is always the slower part of any front-end JS there is, this is probably because you don't see any "improvements" in your devtools. Since there is no argument for this change i.e. -
I'm closing it |
Well I do see improvements in the devtools, I just don't see them in my pseudo-benchmark on jsperf.com. I'm not even sure if the jsperf.com benchmarks are reliable for these kind of things. When I use this change with I'm currently programming a "selectable" like extension for ember here: https://github.com/topaxi/ember-snip Using https://github.com/mrdoob/stats.js/ I see the following FPS on my notebook while randomly dragging around on the screen: Firefox: Chrome: I suspect this change might even have a bigger impact on mobile devices. If these "hacks" are not welcome, feel free to leave this PR closed. |
It is reliable, it is specifically build for these kind of things.
If you could put together something that we could check ourselves? I'm still strongly against these changes, but it is enough evidence for re-opining |
|
After reading through http://blog.methvin.com/2013/12/please-stop-jsperfcom-abuse.html by @dmethvin I don't feel very confident about making a real test-case based on jsperf.com. @dmethvin Would you have time to create some benchmark with me this weekend or some time next week? You can reach me on twitter (@topaxi) or irc.freenode.org (topaxi). Edit: Updated JSPerf for the dispatch function only: http://jsperf.com/jquery-optimized-event-dispatch/4 |
|
Nice job of figuring out how to do a bench test, your results are compelling, added couple more browsers to your data, they all show either same results (however i suspect this is However, We can't test real events (if would like to came up with some test-suite that would be great though), we can test |
|
Yes this is the same result I mentioned in my first (well second really) comment. This is why I added a The |
|
@markelog I updated your bench using a mousemove event (which I observed the real-world speedups in my browsers), as triggering a click event (which occurs not very much, usually...) has a special case in https://github.com/jquery/jquery/blob/master/src/event.js#L505. Chrome seems slower on my desktop, but faster on mobile. |
|
I can't look at this in detail right now but we would definitely want to see the full path with real events and/or |
|
Added couple more browsers to your example, on my chrome mobile (although "mouse" event on mobile?) it doesn't show any improvements, latest IE (those three dots), Safari (just like FF but in the opposite way) (and even sometimes FF) mobile Safari (expectedly, {although "mouse" event on mobile?}) still show significant speed decrease. Whenever or not or our That sort of inconsistency is what i was ranting about, you did chrome optimization and yet you saw FF only improvements. "Tomorrow" it might be the opposite. Just shot in the dark it seems. I agree we need to improve performance of event handling if we can, but i'd argue that fast compiler optimizations is not the way. |
Actually, triggering events is pretty common use-case, i would think they are reflect the "real world" cases. |
Yes, but not as frequent as mousemove or touchmove events, they fire very rapidly. If a click event or similar costs even 10ms more and a mouse- or touchmove costs 5ms less, it might actually be worth it. |
|
I was questioning the
part, not whenever or not it would be worth it. That would be a different discussion. |
|
Anyways, what does the core team need to merge or close this request? I'll be happy to work out some approved benchmarks. On all my observations and profiling, I think it should be worthwhile to remove code that actually prevents engines from optimization. I wasn't targeting any engine as all of them have troubles with those two changes I made. On a side-note, other projects like Ember.js even put this in their guidelines (I respect your choice to not add such rules to yours). |
I'd say proof of speed increase, right now, we definitely established that it will cause speed decrease. Which is the opposite. If real events would work faster, we would need to decide what is more important - Now of "how", is the part where you can get creative :-). There is no universally good way to do it. One might argue that you can take real site, profile it, with latest version of jQuery with and without your changes, on all browsers you have access to. Or you can use the
That is that part i disagree with, i don't care whenever code is optimized or not, i care if that code is fast enough.
This is why i bring up Bluebird, a lot of people praised and impress with it. Without understanding it.
We might just do that, if you will able to prove your point :-) |
|
The mousemove/pointermove scenarios (and maybe window.scroll) are the most plausible scenarios where the event subsystem might cause a bottleneck. Once you get all the other parts of the execution path involved, though, it may be that deoptimization of jQuery.event.dispatch isn't enough of a difference to be noticed. In that case it would be good to find the real bottlenecks and fix those, either in addition to or in place of this patch. One real perf problem that we know of is that with real events we currently copy and normalize the event object as soon as the event arrives, through I would love to see some kind of real-world repeatable benchmark we could use to judge whether we're really improving things or just optimizing code for no reason. |
|
Just a small update, I took the benchmark of @markelog and reversed the examples (http://jsperf.com/trigger-vs-opt-trigger/5), seems like the second one is always slower. Even putting the same two test-cases causes the second one to slow down by around 20%. I guess some GC behaviour is interfering here. I'll look into some other way of doing the benchmarks later this evening. Even if this means I have to do one test-case at a time and do a Ops/sec table by hand. |
|
http://jsperf.com/trigger-vs-opt-trigger/9 Now they are the same (more or less). Now there is question of whenever we want this. |
|
Gotta say, i hated proving opposite point :/ it is hard to be impartial |
|
@dmethvin What you do think? Should we land? |
|
So the con to landing it is that it's a Chrome-specific optimization, I don't know if other browsers disable optimization when On the pro side, we already do browser-specific things like isolate try/catch into a function because of Chrome, even though other browsers like Edge will optimize such functions. Our team has been stable enough (yay us!) that we'll have enough organizational memory to come back and change things if it turns out it is no longer needed. Plus it's nice to see the dispatch speed double for cases like mousemove and pointermove which can be bottlenecks. So what the heck, let's land it. Nice work @topaxi! Since @topaxi already has a framework to test perf for these cases, I'd love to see #2337 comment happen in 3.0 so that |
|
Here's an updated version of my change which @dmethvin mentioned if anyone wants to try it out. There were definitely still a few questions about that though. Especially if the perf hit of getter/setters is worth avoiding the fix-loop... I seem to recall the set on first get was also questionable. There also appears to be an argument assignment in |
|
@jbedard your link isn't working for me. Can you open a new PR? we can discuss/experiment there. Nice catch on the arg assignment! |
|
Opps, I've fixed the link in the comment. Also opened #2860. |
|
I would ask for some clarification comment before the loop. Also, i'm not sure if actually see speed increase, right now, proof seems inconclusive, in one test we see speed decrease in the other speed increase but for Firefox only, whereas this should be v8 specific opt. Without any explanation. Would still love some explanation from the @jdalton and his overall opinion about it. |
|
I'd be curious if only removing the argument assignment would make a difference, but keeping the |
|
@jbedard param assignments are only bad if you do this while also using an The issue is that the A small example: function foo(bar, baz) {
console.log(arguments[0] === 'bar')
console.log(arguments[1] === 'baz')
// so far so good.. but if we assign something to arguments
arguments[0] = 'foo'
console.log(arguments[0] === 'foo') // still good
console.log(bar === 'foo') // Oops, by assigning arguments[0] we also reassigned bar
// this also works the other way around
baz = 'foo'
console.log(baz === 'foo') // Okay, this is what we wanted
console.log(arguments[1] === 'foo') // Oops, we somehow changed something in arguments
}
foo('bar', 'baz') // this logs 6 times `true`And this is why all engines (to date) deoptimize if we pass arguments around or assign to params when an arguments object is being used. So to avoid these mistakes, some guidelines agreed to avoid param reassigned at all times, while it (at least to my knowledge) is only bad while using To summarize params and arguments optimization issues: // OK, param reassignment but no arguments object
function(foo) {
foo = somethingelse()
}
// Bad, param reassignment with arguments object
function(foo) {
foo = somethingelse()
console.log(arguments[0])
}
// Bad, passing arguments to an other function
function(foo) {
var args = Array.prototype.slice.call(arguments)
console.log(args[0])
}
// Good, converting arguments to array
function(foo) {
var args = new Array(arguments.length)
for (var i = 0; i < arguments.length; i++) {
args[i] = arguments[i]
}
console.log(args[0])
} |
|
Still sounds like a good thing to do, and it can be done without the |
|
I'd like to land this, with a small addition of a comment that says we're doing the explicit loop to allow JS engines to optimize the function. Any objections? |
|
@dmethvin SGTM. |
8 participants
The event dispatch method is not optimizable by modern JS engines. This is especially crucial if a lot of events are dispatched (mousemove and touchmove come to mind).
Fixed by not reassigning function param
Fixed by not pass arguments object to other Array#slice()
Now optimizable by v8, spidermonkey and most probably all the other engines:
\o/