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

Enhancement: Investigate caching type checking APIs #6425

Closed
4 tasks done
JoshuaKGoldberg opened this issue Feb 5, 2023 · 4 comments
Closed
4 tasks done

Enhancement: Investigate caching type checking APIs #6425

JoshuaKGoldberg opened this issue Feb 5, 2023 · 4 comments
Assignees
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance
Milestone

Comments

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Feb 5, 2023

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

typescript-estree

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Continuing from #6404 (comment): we've been talking off-and-on for ages now about adding a thin layer of caching around TypeScript's type checking APIs.

do we want to do a weakmap memo on these? Eg

function memoize<TNode, TRet>(fn: (node: TNode) => TRet): (node: TNode) => TRet {
  const cache = new WeakMap<TNode, TRet>();

  return (node) => {
    const cachedRet = cache.get(node);
    if (cachedRet) {
      return cachedRet;
    }

    const newRet = fn(node);
    cachedRet = cache.set(node, newRet);
    return newRet;
  };
}

That would allow us to enforce that if two rules request the exact same node then we'll 100% use a fast path.

Doing a quick look at the implementation of getTypeAtLocation - it doesn't do any fast caching, so it does do a bunch of work.

IDK if this sort of thing would actually save any real time though - would be worth a test. Maybe we can file an issue to look at this later?

Additional Info

This'll require performance testing. Does it actually speed things up? Does it cause out-of-memory issues on our repo? Let's find out!

Strong candidate for including in the v6 betas. I'd like to get real world user testing to make sure this doesn't explode anybody.

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Feb 23, 2023

Removing this from the v6 milestone for now. I don't think I'll get this in in time. I'm guessing that if the caching is dangerous / trade-off-y enough to be considered a breaking change, we'd want to have the behavior be opt-in behind a flag to start anyway.

@bradzacher bradzacher modified the milestones: 6.0.0, 7.0.0 Mar 7, 2023
@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Apr 8, 2023
@jportner
Copy link

@JoshuaKGoldberg Is this issue concerned with the same problem as #243 / #389?

I would have commented on one of those, but they appear to have been closed due to inactivity, and they are locked to contributors. I've been troubleshooting extremely poor ESLint performance on my TypeScript project (single tsconfig, 3.8k files, 2M LOC). Specifically it's been painful trying to use the ESLint plugin in VS Code, which we use to format files on save as well (in conjunction with Prettier).

For any file over a few hundred lines, ESLint becomes very unresponsive (any change can take 5-10 seconds to reflect in the UI).
Linting one of our largest files (3k lines... ugh) using the CLI takes ~17 seconds. The vast majority of this time is not individual rules, but generating the AST, apparently.

The recommended workaround in the above linked issues is to use project: undefined in our ESLint config, which brings the lint time down to ~1.5 seconds.
Unfortunately we had to disable several rules that rely on TypeScript to do this.


If the enhancement you're describing here is intended to address this problem, I'd be interested in assisting, though I'm not certain where to start.

If I'm barking up the wrong tree here -- apologies for hijacking this issue, would it be OK to re-open one of those issues to continue the discussion there? Or would you recommend opening a new issue?

@JoshuaKGoldberg
Copy link
Member Author

A new issue would be nice - worst case scenario it gets closed and linked as a duplicate 😄. But if you can provide us concrete reproduction steps -including your codebase, how to start developing on our local machines, and how to trigger the lint issues- that would be lovely.

A couple of things things to read through first:

@JoshuaKGoldberg
Copy link
Member Author

JoshuaKGoldberg commented Jul 17, 2023

I'm more hopeful about the EXPERIMENTAL_useProjectService parser option (#6754). Closing this for now.

If someone does think this feature would benefit them, feel free to post an explanation why! Here if it's unlocked, or in a new issue after.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 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 enhancement New feature or request package: typescript-estree Issues related to @typescript-eslint/typescript-estree performance Issues regarding performance
Projects
None yet
3 participants