Skip to content
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

Pre-filter and sort rules for 400% perf boost #246

Merged
merged 5 commits into from
Jul 2, 2019

Conversation

TrevorBurnham
Copy link
Contributor

@TrevorBurnham TrevorBurnham commented Jun 30, 2019

Closes #193

Currently, the Lint() function loops through all 4361 rules on every node. The cost of all that looping really adds up! For most projects, most of those rules are irrelevant because they only fail for very old or esoteric browsers. In the project I used to test this branch, the browser list is

[
  'last 3 chrome version',
  'last 3 firefox version',
  'last 2 edge version',
  'last 2 safari version',
  'IE 11'
]

and only 1679 of the rules potentially fail. For a project that only supports modern browsers (no IE11), the relevant set of rules would be much smaller.

This PR filters the set of rules based on the targeted browser list, removing the need to iterate through irrelevant rules on every node. It does that filtering only once per browser list. This change reduces the time it takes to run compat/compat against my test project from ~5.5s to ~2.5s. 🚀


I've marked this as WIP because the code here is very messy, so please take it as a proof of concept. If you like this change, let me know and I'll tidy it up!

@TrevorBurnham
Copy link
Contributor Author

TrevorBurnham commented Jul 1, 2019

Now that #245 is merged, I've rebased this branch. I'll try tidying up the code later this week.

@TrevorBurnham
Copy link
Contributor Author

TrevorBurnham commented Jul 1, 2019

I've added another major optimization here: Instead of iterating over all the rules on every node (checking that the rule type matches the node type on each iteration), I pre-sort the rules by their astNodeType. So now CallExpression nodes only run against CallExpression rules, NewExpression nodes only run against NewExpression rules, etc. I've also made each node type run only the validation logic required for that node type, instead of running a type-agnostic validation function.

The impact of these changes is huge. On the current master, my test project takes ~5.5s to run compat/compat. On this branch, it only takes ~1s! 🎉

The code could still use some cleanup, but I'm thrilled with the improvement. For comparison, this takes compat/compat from being by far the most expensive rule in my project's linter config to ranking in third place, between react/no-direct-mutation-state and react/sort-comp. 🥉

@TrevorBurnham TrevorBurnham changed the title WIP: Pre-filter rules based on targeted browsers Pre-filter and sort rules for 400% perf boost Jul 2, 2019
@TrevorBurnham
Copy link
Contributor Author

@amilajack OK, I've done some refactoring to tidy up the code on this branch and improve Flow coverage. I hope it's reasonably clear and well-organized now. Please let me know if there are any changes you'd like to see!

@amilajack
Copy link
Owner

Awesome! This is a huge improvement to this project! Thank you so much 🎉. I'll add this to the 3.3.0 release as well.

@amilajack amilajack merged commit a2f2ff1 into amilajack:master Jul 2, 2019
@TrevorBurnham TrevorBurnham deleted the 193-perf-part-two branch July 2, 2019 16:55
TrevorBurnham pushed a commit to TrevorBurnham/eslint-plugin-compat that referenced this pull request Jul 3, 2019
amilajack pushed a commit that referenced this pull request Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint duration in v3
2 participants