perf: add per-directory module resolution cache to typescript-go

[camchenry]

Spent some time profiling our linting runs. One hot function that kept coming up was resolveImportsAndModuleAugmentations, as in many cases around 44% of stack frames were sampled as being in this function or a function that is called by it.

The idea of this optimization is to cache the results of ResolveModuleName and ResolveTypeReferenceDirective which are called frequently by this function. In theory, I think that for the same directory and the same module name, it should always return the same results?

So far, this seems to be correct: the diagnostics we return when linting kibana, vscode, posthog and other projects all appear to be identical.

This has a big effect on performance, since this is a hot path for us seemingly.

---

initial benchmark results

/Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --version
Version: 1.50.0

Benchmark 1: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware vscode/src
  Time (mean ± σ):     10.801 s ±  0.182 s    [User: 30.387 s, System: 1.812 s]
  Range (min … max):   10.604 s … 11.236 s    20 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware vscode/src --silent
  Time (mean ± σ):      9.702 s ±  0.113 s    [User: 29.975 s, System: 1.744 s]
  Range (min … max):    9.513 s …  9.888 s    20 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware vscode/src --silent ran
    1.11 ± 0.02 times faster than OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware vscode/src

Benchmark 1: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src/core
  Time (mean ± σ):      3.572 s ±  0.090 s    [User: 18.257 s, System: 0.752 s]
  Range (min … max):    3.463 s …  3.692 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src/core --silent
  Time (mean ± σ):      2.731 s ±  0.054 s    [User: 11.797 s, System: 0.648 s]
  Range (min … max):    2.681 s …  2.857 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src/core --silent ran
    1.31 ± 0.04 times faster than OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src/core

Benchmark 1: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src
  Time (mean ± σ):     44.540 s ±  1.350 s    [User: 267.742 s, System: 8.167 s]
  Range (min … max):   43.145 s … 47.717 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src --silent
  Time (mean ± σ):     28.104 s ±  0.195 s    [User: 155.435 s, System: 5.623 s]
  Range (min … max):   27.705 s … 28.325 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src --silent ran
    1.58 ± 0.05 times faster than OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware kibana/src

Benchmark 1: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware posthog
  Time (mean ± σ):     555.7 ms ±   9.5 ms    [User: 2469.4 ms, System: 301.5 ms]
  Range (min … max):   544.7 ms … 567.5 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware posthog --silent
  Time (mean ± σ):     523.6 ms ±  23.2 ms    [User: 2437.4 ms, System: 305.4 ms]
  Range (min … max):   500.1 ms … 578.2 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware posthog --silent ran
    1.06 ± 0.05 times faster than OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware posthog

Benchmark 1: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware social-app
  Time (mean ± σ):     635.4 ms ±  24.1 ms    [User: 3131.2 ms, System: 604.0 ms]
  Range (min … max):   606.6 ms … 684.1 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware social-app --silent
  Time (mean ± σ):     587.3 ms ±  32.3 ms    [User: 2812.9 ms, System: 580.0 ms]
  Range (min … max):   554.2 ms … 650.7 ms    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-dev /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware social-app --silent ran
    1.08 ± 0.07 times faster than OXLINT_TSGOLINT_PATH=/Users/camchenry/workspace/tsgolint/tsgolint-main /Users/camchenry/.npm/_npx/f4a33d9b4519ccab/node_modules/.bin/oxlint --type-aware social-app

flamegraph before:

flamegraph after:

allocs before (kibana):

allocs after (kibana):

[camchenry]

• perf: add per-directory module resolution cache to typescript-go #764 👈 (View in Graphite)
• refactor: make E2E benchmark more realistic #763
• main

---

How to use the Graphite Merge Queue

Add the label 0-merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

[camchenry]

Okay I have de-slopified this PR. All of the code from the last commit was mostly handwritten and I updated the design to make more sense.

[camchenry]

@jakebailey I'm sorry to ping you directly, but this might be of interest to you. I did some profiling on tsgolint on real-world projects and found that resolveImportsAndModuleAugmentations is a hot function for us. I found an opportunity for us to cache data coming from ResolveModuleName and ResolveTypeReferenceDirective, which makes a big difference for our linting process.

Two questions:

1. Is this code heinously wrong? If it's not correct, we shouldn't do it, but if it seems okay, this is a very impactful change. So far it seems correct, but I am not too familiar with the typescript-go codebase.
2. Is this a hot function for tsgo too? I haven't profiled any tsgo builds, but curious if you have any interest in me upstreaming this code (and maybe making the module resolution cache optional with .Enable() or something)

[camc314]

this might not be a correct optimization, I need to verify its correctness still. take these benchmark numbers with a grain of salt, because they don't mean much if it's not correct.

@camchenry can we this so we do a more gradual release? First we can release this, with a comparison with the cache vs recomputing (panic if it's different). After 2/3 weeks, we can drop the comparison and just use the cache.

[camchenry]

this might not be a correct optimization, I need to verify its correctness still. take these benchmark numbers with a grain of salt, because they don't mean much if it's not correct.

@camchenry can we this so we do a more gradual release? First we can release this, with a comparison with the cache vs recomputing (panic if it's different). After 2/3 weeks, we can drop the comparison and just use the cache.

good idea - any thoughts on how we would like to surface that? given it's likely to be correct, we could maybe just return a diagnostic that indicates there was a caching issue?

I'll be out on vacation this week - so feel free to implement that while I'm away. I probably won't pick this up for a week or so.

[jakebailey]

I think @andrewbranch might have a better idea than me as to whether or not this is a valid optimization.

Of course, all of the patches you do are worth talking about upstreaming (since we would like to actually just have a Go API where you do not need to do any of this at some point in the future).

[camchenry]

Of course, all of the patches you do are worth talking about upstreaming (since we would like to actually just have a Go API where you do not need to do any of this at some point in the future).

Yes, that will be great! I'd like to try and reduce the number of patches we have, and we'll definitely try out the Go API when it's available. In the meantime, I will see if I can upstream more of this code. For this PR specifically, I'll try to benchmark it in typescript-go and see if it has any impact.

[andrewbranch]

Ironically, we used to have basically this, and @auvred showed that it was worse than not having any caching: microsoft/typescript-go#673

[camchenry]

Ironically, we used to have basically this, and @auvred showed that it was worse than not having any caching: microsoft/typescript-go#673

Oh wow, that is really interesting. I wonder if this patch is correct then, given I'm not using any mutexes or anything, just depending on the SyncMap implementation to handle that? Maybe this is a lighter version of the original module resolution cache?

[andrewbranch]

I haven't looked too closely at the differences yet, and the original cache implementation is one of the first bits of Go code I ever wrote, so it's highly possible I did something dumb!

[camchenry]

I haven't looked too closely at the differences yet, and the original cache implementation is one of the first bits of Go code I ever wrote, so it's highly possible I did something dumb!

I'm not too familiar with Go either, so this has been a good learning opportunity for me! When I get back from being out-of-office, I can definitely try benchmarking this. It seems like if we had this before, then it might be worth trying this new implementation and see how it compares. I'm sure a lot has changed in the codebase since April that might change the calculus of whether this is worth it or not.