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

Avoid reading outside of collection bounds #3769

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@mathiasbynens
Contributor

mathiasbynens commented Aug 29, 2017

Summary

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 the cleanData 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


Similar patch for Sizzle: jquery/sizzle#407

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Aug 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.

mention-bot commented Aug 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.

Avoid reading outside of collection bounds
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/
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Aug 29, 2017

Member

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?

Member

markelog commented Aug 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?

@mathiasbynens

This comment has been minimized.

Show comment
Hide comment
@mathiasbynens

mathiasbynens Aug 29, 2017

Contributor

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 Sizes
271593  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
Contributor

mathiasbynens commented Aug 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 Sizes
271593  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
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Aug 29, 2017

Member

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?

Member

markelog commented Aug 29, 2017

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?

@mathiasbynens

This comment has been minimized.

Show comment
Hide comment
@mathiasbynens

mathiasbynens Aug 29, 2017

Contributor

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?

👍

Contributor

mathiasbynens commented Aug 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?

👍

@@ -285,9 +285,11 @@ jQuery.extend( {
cleanData: function( elems ) {
var data, elem, type,
special = jQuery.event.special,
length = elems.length,

This comment has been minimized.

@dmethvin

dmethvin Aug 29, 2017

Member

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.

@dmethvin

dmethvin Aug 29, 2017

Member

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.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Aug 29, 2017

Member

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.

Member

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

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin May 15, 2018

Member

@mathiasbynens did you want to put together a jsperf for this? Given what the cleanData loop does I am not sure this will realy make a difference.

Member

dmethvin commented May 15, 2018

@mathiasbynens did you want to put together a jsperf for this? Given what the cleanData loop does I am not sure this will realy make a difference.

@timmywil timmywil added this to the 3.4.0 milestone Sep 3, 2018

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Sep 5, 2018

Member

Key @mathiasbynens, are you still interested in finishing this, including addressing @dmethvin's comments?

Member

mgol commented Sep 5, 2018

Key @mathiasbynens, are you still interested in finishing this, including addressing @dmethvin's comments?

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