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 #407

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@mathiasbynens
Contributor

mathiasbynens commented Aug 29, 2017

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.


Similar patch for jQuery: jquery/jquery#3769

@jsf-clabot

This comment has been minimized.

Show comment
Hide comment
@jsf-clabot

jsf-clabot Aug 29, 2017

CLA assistant check
All committers have signed the CLA.

jsf-clabot commented Aug 29, 2017

CLA assistant check
All committers have signed the CLA.

@mathiasbynens mathiasbynens referenced this pull request Aug 29, 2017

Open

Avoid reading outside of collection bounds #3769

4 of 4 tasks complete
@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Aug 29, 2017

Member

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

Hey.

This looks nice to me. How this reflects on the byte size though? Maybe you could provide a small jsperf as well?

@@ -671,8 +671,10 @@ setDocument = Sizzle.setDocument = function( node ) {
// Fall back on getElementsByName
elems = context.getElementsByName( id );
length = elems.length;
i = 0;

This comment has been minimized.

@markelog

markelog Aug 29, 2017

Member

This could moved to the body of a for right?

@markelog

markelog Aug 29, 2017

Member

This could moved to the body of a for right?

This comment has been minimized.

@mathiasbynens

mathiasbynens Aug 29, 2017

Contributor

Yes, for both i and length. However, i was already outside of the for, so I decided to follow the existing style.

@mathiasbynens

mathiasbynens Aug 29, 2017

Contributor

Yes, for both i and length. However, i was already outside of the for, so I decided to follow the existing style.

This comment has been minimized.

@markelog

markelog Aug 29, 2017

Member

Oh, that because of the https://contribute.jquery.org/style-guide/js/#good-examples, i.e. it should be fine to put i to the body, as long its initialised on top of the scope, like we doing here – https://github.com/jquery/jquery/blob/692f9d4db30c9c6c4f6bc76005cf153586202fa6/src/effects.js#L614 :)

@markelog

markelog Aug 29, 2017

Member

Oh, that because of the https://contribute.jquery.org/style-guide/js/#good-examples, i.e. it should be fine to put i to the body, as long its initialised on top of the scope, like we doing here – https://github.com/jquery/jquery/blob/692f9d4db30c9c6c4f6bc76005cf153586202fa6/src/effects.js#L614 :)

@@ -671,8 +671,10 @@ setDocument = Sizzle.setDocument = function( node ) {
// Fall back on getElementsByName
elems = context.getElementsByName( id );
length = elems.length;

This comment has been minimized.

@gibson042

gibson042 Aug 30, 2017

Member

NodeList#length can be an element in IE<9 (and we should have a test where that is the case on this line). cf. f6e2fc5#diff-7d34356c0b7229dd5da8b6c7711b32b7R1733

@gibson042

gibson042 Aug 30, 2017

Member

NodeList#length can be an element in IE<9 (and we should have a test where that is the case on this line). cf. f6e2fc5#diff-7d34356c0b7229dd5da8b6c7711b32b7R1733

This comment has been minimized.

@mathiasbynens

mathiasbynens Aug 31, 2017

Contributor

So, something like this?

if (typeof elem.length === 'number') {
  fastPath();
} else {
  slowPathForLegacyBrowsers();
}
@mathiasbynens

mathiasbynens Aug 31, 2017

Contributor

So, something like this?

if (typeof elem.length === 'number') {
  fastPath();
} else {
  slowPathForLegacyBrowsers();
}

This comment has been minimized.

@gibson042

gibson042 Aug 31, 2017

Member

Yes, provided we can get it without excessive file size increase.

@gibson042

gibson042 Aug 31, 2017

Member

Yes, provided we can get it without excessive file size increase.

This comment has been minimized.

@gibson042

gibson042 Aug 31, 2017

Member

example:

for ( ; i !== len && (elem = elems[i]) != null; i++ ) {
@gibson042

gibson042 Aug 31, 2017

Member

example:

for ( ; i !== len && (elem = elems[i]) != null; i++ ) {
@@ -702,11 +704,13 @@ setDocument = Sizzle.setDocument = function( node ) {
tmp = [],
i = 0,
// By happy coincidence, a (broken) gEBTN appears on DocumentFragment nodes too
results = context.getElementsByTagName( tag );
results = context.getElementsByTagName( tag ),
length = results.length;

This comment has been minimized.

@gibson042

gibson042 Aug 30, 2017

Member

Same problem here.

@gibson042

gibson042 Aug 30, 2017

Member

Same problem here.

This comment has been minimized.

@mathiasbynens

mathiasbynens Sep 10, 2017

Contributor

Done.

@mathiasbynens

mathiasbynens Sep 10, 2017

Contributor

Done.

@@ -1054,17 +1058,19 @@ Sizzle.uniqueSort = function( results ) {
var elem,
duplicates = [],
j = 0,
i = 0;
i = 0,
length = results.length;

This comment has been minimized.

@gibson042

gibson042 Aug 30, 2017

Member

Same problem here.

@gibson042

gibson042 Aug 30, 2017

Member

Same problem here.

This comment has been minimized.

@mathiasbynens

mathiasbynens Sep 10, 2017

Contributor

Done.

@mathiasbynens

mathiasbynens Sep 10, 2017

Contributor

Done.

mathiasbynens added some commits Aug 29, 2017

Core: 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.
@gibson042

This comment has been minimized.

Show comment
Hide comment
@gibson042

gibson042 Sep 15, 2017

Member

This is pretty big... can you put together a jsPerf or other benchmark (example, and yes I'm aware of the irony) showing the value?

   raw     gz Sizes
 65502  19586 dist/sizzle.js
 19851   7421 dist/sizzle.min.js

   raw     gz Compared to master @ 840b647cd1ddd9d9aec3cde73af3f697fbf068d7
  +194    +51 dist/sizzle.js
   +82    +54 dist/sizzle.min.js
Member

gibson042 commented Sep 15, 2017

This is pretty big... can you put together a jsPerf or other benchmark (example, and yes I'm aware of the irony) showing the value?

   raw     gz Sizes
 65502  19586 dist/sizzle.js
 19851   7421 dist/sizzle.min.js

   raw     gz Compared to master @ 840b647cd1ddd9d9aec3cde73af3f697fbf068d7
  +194    +51 dist/sizzle.js
   +82    +54 dist/sizzle.min.js
i = 0;
while ( (elem = elems[i++]) ) {
for ( ; i !== length && (elem = elems[ i ]) != null; i++ ) {

This comment has been minimized.

@timmywil

timmywil Sep 18, 2017

Member

This is probably not an issue, but I feel like i < length is safer.

@timmywil

timmywil Sep 18, 2017

Member

This is probably not an issue, but I feel like i < length is safer.

This comment has been minimized.

@gibson042

gibson042 Sep 18, 2017

Member

i < length would definitely be an issue, because length might be a DOM node in IE<9.

@gibson042

gibson042 Sep 18, 2017

Member

i < length would definitely be an issue, because length might be a DOM node in IE<9.

This comment has been minimized.

@timmywil

timmywil Sep 18, 2017

Member

makes sense.

@timmywil

timmywil Sep 18, 2017

Member

makes sense.

This comment has been minimized.

@timmywil

timmywil Sep 18, 2017

Member

Perhaps we could put in a comment relating that as a reminder.

@timmywil

timmywil Sep 18, 2017

Member

Perhaps we could put in a comment relating that as a reminder.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 4, 2017

Member

If the Chrome perf quirk was the main driver for this, that has been fixed.

Member

dmethvin commented Nov 4, 2017

If the Chrome perf quirk was the main driver for this, that has been fixed.

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