Retrievingarray[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 thannot 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 thecleanData loop 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

All authors have signed the CLA athttps://cla.js.foundation/jquery/jquery
New tests have been added to show the fix or feature works:N/A
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created athttps://github.com/jquery/api.jquery.com:N/A

Similar patch for Sizzle:jquery/sizzle#407

Copy link

mention-bot commentedAug 29, 2017

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

mathiasbynens mentioned this pull request

Aug 29, 2017

Avoid reading outside of collection boundsjquery/sizzle#407

Open

mathiasbynens force-pushed theavoid-reading-past-end-of-array branch from8633fc7 to2a1564bCompare

August 29, 2017 08:04

Avoid reading outside of collection bounds

9d8431c

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/

mathiasbynens force-pushed theavoid-reading-past-end-of-array branch from2a1564b to9d8431cCompare

August 29, 2017 08:12

Copy link

Member

markelog commentedAug 29, 2017

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?

Copy link

ContributorAuthor

mathiasbynens commentedAug 29, 2017

How this reflects on the byte size though?

This change adds 6 bytes after minification + gzip.

Running "compare_size:files" (compare_size) task   raw     gz Sizes271593  80434 dist/jquery.js 87296  30177 dist/jquery.min.js   raw     gz Compared to avoid-reading-past-end-of-array @              9d8431c8329198770212cc8952804bc83d0a8dfd     =      = dist/jquery.js     =      = dist/jquery.min.js   raw     gz Compared to master @ 692f9d4db30c9c6c4f6bc76005cf153586202fa6   +50     +6 dist/jquery.js   +19     +6 dist/jquery.min.js   raw     gz Compared to last run   +50     +6 dist/jquery.js   +19     +6 dist/jquery.min.js

Copy link

Member

markelog commentedAug 29, 2017•
edited
Loading

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?

Copy link

ContributorAuthor

mathiasbynens commentedAug 29, 2017

Thinking a bit further, internally, we were thinking to write loops in same matter across the code base.

Great idea!

Maybe this even worth to write a custom eslint rule, we can even put in the jquery preset and all. Since such cases are fairly generalised?

👍

dmethvin reviewed

Aug 29, 2017

View reviewed changes

src/manipulation.js

		cleanData:function(elems){
		vardata,elem,type,
		special=jQuery.event.special,
		length=elems.length,

Copy link

Member

dmethvinAug 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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.

Copy link

Member

dmethvin commentedAug 29, 2017

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.