jquery / sizzle

A sizzlin' hot selector engine.
https://sizzlejs.com
Other
6.29k stars 951 forks source link

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

Closed wartmanm closed 6 years ago

wartmanm commented 6 years ago

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.

wartmanm commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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.