estools / esquery

ECMAScript AST query library.
http://estools.github.io/esquery/
Other
816 stars 88 forks source link

Optimize hot code paths #134

Closed jviide closed 1 year ago

jviide commented 1 year ago

This pull request aims to optimize some hot code paths of esquery. These changes include the things we encountered with @marvinhagemeister, who outlined them in the excellent article Speeding up the JavaScript ecosystem - eslint, plus some additional ones added after the post was published.

The specific changes are as follows:

We microbenchmarked the effect of each of these changes, but to get a better of their real life impact I tested the eslint-plugin-unicorn ESLint plugin on the codebase at my workplace. The plugin relies heavily on selectors. Linting times were as follows:

Cumulatively these optimizations cut down the overhead added by eslint-plugin-unicorn by more than 50%, at least in our setup. The biggest bang for the buck came from hoisting constants (2s reduction) and avoiding for-of transpilation (2s reduction).

The changes pass the tests and should be fully backwards compatible.

floroz commented 1 year ago

This pull request aims to optimize some hot code paths of esquery. These changes include the things we encountered with @marvinhagemeister, who outlined them in the excellent article Speeding up the JavaScript ecosystem - eslint, plus some additional ones added after the post was published.

really impressive investigation and great article 👏

gajus commented 1 year ago

@michaelficarra Is it possible to merge and release these changes?

michaelficarra commented 1 year ago

@gajus Thanks for the ping.

gajus commented 1 year ago

Thank you all ❤️

michaelficarra commented 1 year ago

@jviide I pushed some additional tweaks. Can you review and confirm that the performance has not regressed for you? After that, this should be good to go.

jviide commented 1 year ago

@michaelficarra Looks good, there were no performance regressions in my environment.

jviide commented 1 year ago

@michaelficarra A side note about the tests: On Node 18 the tests also failed for me (like they seem to be failing in the CI), until I changed the mocha --require chai/register-assert part in package.json to mocha --require chai/register-assert.js. Don't really know why.

michaelficarra commented 1 year ago

@jviide It seems to work when I remove the ^ from the dependency's semver spec, so we'll just do that. I guess they made a breaking change. Thanks for the pointer though.

jviide commented 1 year ago

Awesome, thank you for the review and the merge 🙂

michaelficarra commented 1 year ago

Published 1.4.1.