Skip to content

Selector: avoid calls to setAttribute( "id" ) by examining selector #431

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

Closed
wants to merge 1 commit into from
Closed

Conversation

wartmanm
Copy link
Contributor

Additionally, use :scope when available.

This implements the optimization proposed in issue 430. I modified the official benchmark to use document.documentElement so the relevant code path would be reached, and it has shown a modest but consistent improvement in every browser I've tested. It can be found here.

In practice, I've seen this change more than halve the load times of some pages in Edge, which seems to have particularly slow setAttribute calls.

I don't know if this is the best possible order in which to try alternatives, but it is the best I could come up with. Using the selector verbatim when possible appears to be the fastest across browsers by a small margin. I haven't been able to find a clear winner between :scope and getAttribute("id"). :scope is probably not able to be shared with CSS or with other qSA calls, but #id should be, and my hope is that this advantage outweighs the cost of calling getAttribute.

Copy link
Member

@gibson042 gibson042 left a comment

Choose a reason for hiding this comment

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

I like the general idea, but this size increase is too expensive for "a modest but consistent improvement".

   raw     gz Sizes
 66275  19804 dist/sizzle.js
 20050   7453 dist/sizzle.min.js

   raw     gz Compared to master @ 3116795bba9a0c3d624e0718006b25aa5568d4cb
  +955   +264 dist/sizzle.js
  +275    +83 dist/sizzle.min.js

But I am still interested in the size and performance results from two simpler approaches (both in isolation and together):

  • making selector analysis more coarse by requiring the scope fix for any selector containing a space or greater than sign, regardless of use as combinators (e.g., rcomplex = /[>\x20\t\r\n\f]/)
  • when possible, using :scope instead of a synthetic id

@wartmanm
Copy link
Contributor Author

Thank you for your consideration!

I noticed that simply extracting the scopeSelector function had a size cost:

   raw     gz Sizes
 65375  19561 dist/sizzle.js
 19794   7362 dist/sizzle.min.js

   raw     gz Compared to last run
   +55    +21 dist/sizzle.js
   +19     -8 dist/sizzle.min.js

So after inlining it again, here are the results:

                     Coarse selector analysis     :scope only            Both
 Sizes                             raw     gz      raw     gz      raw     gz 
 dist/sizzle.js                  65564  19593    65562  19588    65778  19654 
 dist/sizzle.min.js              19843   7389    19888   7408    19934   7423 
                                                                              
 Compared to master                raw     gz      raw     gz      raw     gz 
 dist/sizzle.js                   +244    +53     +242    +48     +458   +114 
 dist/sizzle.min.js                +68    +19     +113    +38     +159    +53 

Performance-wise, it seems like a toss-up. I think most of the gain is from avoiding setAttribute() and not the selectors, so :scope benefits more selectors on the browsers that support it, but selector analysis benefits more browsers.

@gibson042
Copy link
Member

I concur with your assessment. Let's refocus this PR to preserve the existing id-based scope fix but use coarse regex analysis to skip it for selectors that are obviously combinator-free, and then we can revisit the possibility of using :scope in a followup.

@wartmanm
Copy link
Contributor Author

I updated the branch and got rid of a couple more characters. (down to +54, minified).
The regex >|(?:^|[^,]|\\.)[\x20\t\r\n\f] would also identify comma-separated selectors with spaces, but I don't know if it's worth the extra 16 or so characters.

@gibson042 gibson042 added this to the 2.3.4 milestone Nov 4, 2018
@gibson042 gibson042 closed this in 71fe25c Nov 4, 2018
@wartmanm wartmanm deleted the scope branch December 6, 2018 19:55
mgol added a commit to mgol/sizzle that referenced this pull request Oct 11, 2019
An optimization added in jquery#431 skips the temporary IDs for selectors
not using child or descendant combinators. There are a few issues with this
optimization, though:
1. For sibling combinators, this pushes a selector with a leading combinator to
   qSA directly which crashes and falls back to a slower Sizzle route.
2. qSA in IE/Edge doesn't find elements with an empty name attribute selector
   (`[name=""]`) but it does find them when there's an ID prefix for the
   selector (`#test [name=""]`). Therefore, skipping the ID prefix more hurts
   than helps.
3. After jquery/jquery#4454 & jquery#453, all modern browsers other than
   Edge don't get temporary IDs assigned as we leverage the :scope pseudo-class.
   Therefore, the optimization doesn't buy us as much.
4. The regex testing the child/descendant combinators checks for `+` and
   whitespace as whitespace is how a descendant selector is constructed.
   However, users often insert spaces to selectors for readability reasons, not
   knowing it would cause a performance penalty.
mgol added a commit to mgol/sizzle that referenced this pull request Oct 12, 2019
An optimization added in jquery#431 skips the temporary IDs for selectors
not using child or descendant combinators. There are a few issues with this
optimization, though:
1. For sibling combinators, this pushes a selector with a leading combinator to
   qSA directly which crashes and falls back to a slower Sizzle route.
2. qSA in IE/Edge doesn't find elements with an empty name attribute selector
   (`[name=""]`) but it does find them when there's an ID prefix for the
   selector (`#test [name=""]`). Therefore, skipping the ID prefix more hurts
   than helps.
3. After jquery/jquery#4454 & jquery#453, all modern browsers other than
   Edge don't get temporary IDs assigned as we leverage the :scope pseudo-class.
   Therefore, the optimization doesn't buy us as much.
4. The regex testing the child/descendant combinators checks for `+` and
   whitespace as whitespace is how a descendant selector is constructed.
   However, users often insert spaces to selectors for readability reasons, not
   knowing it would cause a performance penalty.
mgol added a commit to mgol/sizzle that referenced this pull request Oct 13, 2019
An optimization added in jquery#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery/jquery#4454 & jquery#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes
mgol added a commit to mgol/sizzle that referenced this pull request Oct 13, 2019
An optimization added in jquery#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery/jquery#4454 & jquery#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes
mgol added a commit to mgol/jquery that referenced this pull request Oct 13, 2019
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes
mgol added a commit to mgol/jquery that referenced this pull request Oct 13, 2019
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes

Ref jquery/sizzle#431
mgol added a commit to mgol/sizzle that referenced this pull request Oct 13, 2019
An optimization added in jquery#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery/jquery#4454 & jquery#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes

Ref jquerygh-431
mgol added a commit to mgol/sizzle that referenced this pull request Oct 13, 2019
An optimization added in jquery#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery/jquery#4454 & jquery#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-431
mgol added a commit to mgol/jquery that referenced this pull request Oct 13, 2019
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquery/sizzle#431
mgol added a commit that referenced this pull request Oct 14, 2019
An optimization added in #431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery/jquery#4454 & #453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Closes gh-460
Ref gh-431
mgol added a commit to jquery/jquery that referenced this pull request Oct 14, 2019
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after #4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Closes gh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Sep 8, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Sep 12, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Sep 19, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Sep 21, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Oct 3, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Oct 4, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Nov 17, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Dec 1, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
mgol added a commit to mgol/jquery that referenced this pull request Dec 13, 2022
An optimization added in jquery/sizzle#431 skips the temporary IDs for selectors
not using child or descendant combinators. For sibling combinators, though, this
pushes a selector with a leading combinator to qSA directly which crashes and
falls back to a slower Sizzle route.

This commit makes selectors with leading combinators not skip the selector
rewriting. Note that after jquery#4454 & jquery/sizzle#453, all modern
browsers other than Edge leverage the :scope pseudo-class, avoiding temporary
id attributes.

Ref jquerygh-4509
Ref jquery/sizzle#431
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants