Avoid reading outside of collection bounds#3769
Conversation
|
@mathiasbynens, thanks for your PR! By analyzing the history of the files in this pull request, we identified @timmywil, @markelog and @gibson042 to be potential reviewers. |
8633fc7 to
2a1564b
Compare
Consider the following collection:
const array = ['a', 'b', 'c'];
Retrieving `array[0]` can be done relatively quickly. However, when the property doesn’t exist on the receiver, JavaScript engines must continue to look up the prototype chain until either the property is found or the chain ends. This is inherently slower than *not* doing any prototype chain lookups. Retrieving an out-of-bounds index, e.g. `array[3]`, triggers this scenario, resulting in decreased performance.
This patch changes the way some loops are written to avoid running into the slow case unnecessarily.
A more in-depth explanation featuring a jQuery-specific example can be found here: https://ripsawridge.github.io/articles/blink-mysterium/
2a1564b to
9d8431c
Compare
|
Will duplicate sizzle comment here as well :) – Hey. This looks nice to me. How this reflects on the byte size though? Maybe you could provide a small jsperf as well? |
This change adds 6 bytes after minification + gzip. |
Nice, I think it's worth it. Blog post has interesting insights, but it seems you have to understand Chrome internals to fully appreciate it, that's why I was thinking to check it with our usual approach to the perf pulls. Thinking a bit further, internally, we were thinking to write loops in same matter across the code base. Maybe this even worth to write a custom eslint rule, so we could put in the jquery preset and all. Since such cases are fairly generalised? |
Great idea!
👍 |
| cleanData: function( elems ) { | ||
| var data, elem, type, | ||
| special = jQuery.event.special, | ||
| length = elems.length, |
There was a problem hiding this comment.
Do we really need to put the lengths in a variable? I thought most JS engines optimize for this, or at least that the perf difference isn't significant.
|
Were there only two cases in the code base? I guess the others are in Sizzle and you have that covered with a separate PR. Like @markelog I would be interested in a jsperf to show the difference this makes. |
|
@mathiasbynens did you want to put together a jsperf for this? Given what the |
|
Key @mathiasbynens, are you still interested in finishing this, including addressing @dmethvin's comments? |
|
Closing & re-opening the PR to trigger the EasyCLA check... |
|
Summary
Consider the following collection:
Retrieving
array[0]can be done relatively quickly. However, when the property doesn’t exist on the receiver, JavaScript engines must continue to look up the prototype chain until either the property is found or the chain ends. This is inherently slower than not doing any prototype chain lookups. Retrieving an out-of-bounds index, e.g.array[3], triggers this scenario, resulting in decreased performance.This patch changes the way the
cleanDataloop is written to avoid running into the slow case unnecessarily.A more in-depth explanation can be found here: https://ripsawridge.github.io/articles/blink-mysterium/
Checklist
Similar patch for Sizzle: jquery/sizzle#407