Skip to content

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

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

Closed
bradzacher opened this issue Dec 2, 2022 · 3 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue performance Issues regarding performance repo maintenance things to do with maintenance of the repo, and not with code/docs

Comments

@bradzacher
Copy link
Member

Suggestion

https://artemis.sh/2022/08/07/emulating-calculators-fast-in-js.html

I read this a while ago but didn't think about it's possible performance application in our parser.

Our core conversion logic is one massive switch/case (currently 159 cases).

According to the stackoverflow comment (which has references to the v8 source code) - if you've got more than 128 cases then the switch might not get properly optimised and thus acts slower than hand-written if/else.

One workaround is to use a "jump table" instead of a switch. Jump tables rely on the case value instead being the key of an object - meaning the lookup is instead always as performant as a property lookup (which is usually O(1)).

We should investigate if switching to a "jump table" improves the perf of our parser.

For reference the conversion would be something like this:

switch (node.kind) {
  case SyntaxKind.SourceFile: // ...
  case SyntaxKind.Block: // ...
  // ...
}

// to

const JUMP_TABLE = {
  [SyntaxKind.SourceFile]: (node: ts.SourceFile) => { /* ... */ },
  [SyntaxKind.Block]: (node: ts.Block) => { /* ... */ },
  // ...
};

JUMP_TABLE[node.kind](node);

For this task we'll want to:

  1. instrument typescript-estree to measure the performance baseline with the current code.
  2. make the conversion to a jump table.
  3. measure the performance after the change.
  4. if (3) < (1), then merge the change.
@bradzacher bradzacher added performance Issues regarding performance repo maintenance things to do with maintenance of the repo, and not with code/docs accepting prs Go ahead, send a pull request that resolves this issue labels Dec 2, 2022
@liuxingbaoyu
Copy link
Contributor

@bradzacher
Copy link
Member Author

Yeah that's where I saw the blog post (in the 4.9RC release notes!)
I completely forgot about it until now when I was thinking about the perf of our parser.

Hopefully we can net some decent wins with this!

@bradzacher
Copy link
Member Author

As per investigation on #6371 - time savings are going to amount to less than a tenth of a percent in most cases. Not worth making this change.

@bradzacher bradzacher closed this as not planned Won't fix, can't repro, duplicate, stale Jan 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue performance Issues regarding performance repo maintenance things to do with maintenance of the repo, and not with code/docs
Projects
None yet
2 participants