-
Notifications
You must be signed in to change notification settings - Fork 91
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
Optimize hot code paths #134
Conversation
Transpiled for-of loops do quite a bit of extra work that can be avoided in hot paths.
really impressive investigation and great article 👏 |
@michaelficarra Is it possible to merge and release these changes? |
@gajus Thanks for the ping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM otherwise. Thanks for putting in this work, @jviide!
Thank you all ❤️ |
@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. |
@michaelficarra Looks good, there were no performance regressions in my environment. |
it's failing to install deps and I can't be bothered to find out why
@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 |
@jviide It seems to work when I remove the |
Awesome, thank you for the review and the merge 🙂 |
Published |
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:
getPath
often ends up used a lot, and each invocation ofgetPath(node, selector.name)
split selector.name. As each selector's name stays constant we could split each name once and reuse the result.nthChild
to check a node is its parent's nth child directly instead of looking it up with .indexOf. This way the check's time complexity becomes constant (O(1)) instead of linear (O(number of siblings)).this.break()
if a match is encountered.inPath
.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.