search

dperini / nwsapi

Fast CSS Selectors API Engine
MIT License
105 stars 36 forks source link

Use localName instead of nodeName #37

Closed eps1lon closed 4 years ago

eps1lon commented 4 years ago

Due to heave usage of getComputedStyle in jsdom we discovered that Element.prototype.nodeName was a bottleneck. It seems like this isn't the correct property to use for selectors but Element.prototype.localName (which is way cheaper in jsdom).

See:

It isn't clear to me how to test these changes extensively. I quickly verified it in ./test/jquery/jquery, ./test/html5/, ./test/slick/Runner/runner (all in Chrome 84) and by using this PR in jsdom's yarn test-wpt. All test suites still pass with these changes.

dperini commented 4 years ago

@eps1lon I am going to do the full testing and accept this change if it doesn't produce regressions. Thank you for the suggestion.

dperini commented 4 years ago

@eps1lon the commit has been pushed, your suggestion about the test in isHTML() seems adequate. I also removed two or three .toLowerCase() since they were not necessary any more. Not much speed gain, but surely an improvement even in nwsapi speed itself. Curious about the gain we should see in jsdom which is were this started. Looking forward to comments and more improvements/adjustments.

eps1lon commented 4 years ago

Curious about the gain we should see in jsdom which is were this started.

We had pretty big gains in @testing-library/react in getComputedStyle heavy methods. We're now caching tagName (used by nodeName) in jsdom which lead to up to 40% reduced runtime. Caching tagName and nwsapi using localName are basically equivalent. https://github.com/jsdom/jsdom/pull/3008 has more context (including a benchmark repo).

@eps1lon the commit has been pushed,

What commit? I see this PR now has conflicts. Should I resolve these and is there anything else I should be doing?

dperini commented 4 years ago

@eps1lon sorry for the mess ... I did things as I should have done from the beginning. Accepted your pull request ... I will add the small difference in another commit. After so many changes and error, I believe I would better do a git rebase too when all the new commits are in place.

eps1lon commented 4 years ago

No worries. Whatever makes your life easier.