Skip to content

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Jan 24, 2023

PR Checklist

Overview

This PR converts the parser's internal conversion logic from a switch to a jump table.
I tried two different versions of this:

  1. direct conversion of the switch to a jump table in the class.
  2. conversion of the entire logic from a class to a function, in addition to the jump table conversion

I found the following results:

  • Original cumulative AST conversion across our codebase was ~765ms.
  • With the changes from (1), conversion time barely moved to ~750ms (~1% reduction).
  • With the changes from (2), conversion time moved a bit more to ~685ms (~10% reduction).

I was hoping for a bit more than that, but ultimately that's not a that bad. In the grand scheme of things, however, this is only a ~0.2% reduction in the total lint time for our repo. This is because on our codebase the lint rules themselves take ~70% of the overall lint time - because we use type-aware rules.

TODO:

  • finish the class -> functional conversion and fix tests/types.
  • try this on a larger repo that doesn't use type-aware rules to see how well it translates to runtime reduction.

@bradzacher bradzacher added the performance Issues regarding performance label Jan 24, 2023
@nx-cloud
Copy link

nx-cloud bot commented Jan 24, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 70f8810. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


🟥 Failed Commands
Node 18 - nx test typescript-estree
Node 14 - nx test typescript-estree --coverage=false
nx run-many --target=typecheck --all --parallel
✅ Successfully ran 41 targets

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @bradzacher!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

@netlify
Copy link

netlify bot commented Jan 24, 2023

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 70f8810
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/63cf416c8ce5480008c729e4
😎 Deploy Preview https://deploy-preview-6371--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@bradzacher
Copy link
Member Author

I spent some time testing and retesting this on the large codebase at canva (~48k files) where they don't use type-aware linting.
Total saved time with this change was less than 300ms (25.22s -> 24.98s).
Total time for a lint run is a little over 6 minutes (362s), so conversion time is ~6% of the lint run. Time saved with this change was 0.06%.

It really is just insignificant - not worth landing this, closing.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
performance Issues regarding performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repo: investigate switching core TS->ESTree conversion logic from a "switch/case" to a "jump table" for performance improvements
1 participant