Optimization: cache results between TypeScript projects #182

olafurpg · 2022-10-17T20:41:36Z

Towards #175

Previously, scip-typescript didn't cache anything at all between TypeScript projects. This commit implements an optimization so that we now cache the results of loading source files and parsing options. Benchmarks against the sourcegraph/sourcegraph repo indicate this optimization consistently speeds up the index command in all three multi-project repositories that I tested it with.

sourcegraph/sourcegraph: ~30% from ~100s to ~70s
nextautjs/next-auth: ~40% from 6.5s to 3.9
xtermjs/xterm.js: ~45% from 7.3s to 4.1s

For every repo, I additionally validated that the resulting index.scip has identical checksum before and after applying this optimization. Given these promising results, this new optimization is enabled by default, but can be disabled with the option --no-global-cache.

Test plan

Manually tested by running scip-typescript index tsconfig.all.json in the sourcegraph/sourcegraph repository. To benchmark the difference for this PR:

Checkout the code
Run yarn tsc -b
Go to the directory of your project
Run node PATH_TO_SCIP_TYPESCRIPT/dist/src/main.js
Copy the "optimized" index.scip with cp index.scip index-withcache.scip
Run node PATH_TO_SCIP_TYPESCRIPT/dist/src/main.js --no-global-caches
Validate the checksum is identical from the optimized output shasum -a 256 *.scip

Towards #175 Previously, scip-typescript didn't cache anything at all between TypeScript projects. This commit implements an optimization so that we now cache the results of loading source files and parsing options. Benchmarks against the sourcegraph/sourcegraph repo indicate this optimization consistently speeds up the `index` command in all three multi-project repositories that I tested it with. - sourcegraph/sourcegraph: ~30% from ~100s to ~70s - nextautjs/next-auth: ~40% from 6.5s to 3.9 - xtermjs/xterm.js: ~45% from 7.3s to 4.1s For every repo, I additionally validated that the resulting index.scip has identical checksum before and after applying this optimization. Given these promising results, this new optimization is enabled by default, but can be disabled with the option `--no-global-cache`. *Test plan* Manually tested by running `scip-typescript index tsconfig.all.json` in the sourcegraph/sourcegraph repository. To benchmark the difference for this PR: - Checkout the code - Run `yarn tsc -b` - Go to the directory of your project - Run `node PATH_TO_SCIP_TYPESCRIPT/dist/src/main.js` - Copy the "optimized" index.scip with `cp index.scip index-withcache.scip` - Run `node PATH_TO_SCIP_TYPESCRIPT/dist/src/main.js --no-global-caches` - Validate the checksum is identical from the optimized output `shasum -a 256 *.scip`

varungandhi-src · 2022-10-17T23:21:08Z

src/ProjectIndexer.ts

+    if (!hostCopy.getParsedCommandLine) {
+      return undefined
+    }
+    if (cache.parsedCommandLines.has(fileName)) {
+      return cache.parsedCommandLines.get(fileName)
+    }


I don't fully understand all the cases here; is it possible that the cache has a hit for this even though hostCopy.getParsedCommandLine is missing? (when is getParsedCommandLine missing?)

I may be misunderstanding your comment, but we guard against hostCopy.getParsedCommandLine at the top, so even if the cache has a hit we return undefined when hostCopy.getParsedCommandLine is undefined.

varungandhi-src · 2022-10-17T23:24:43Z

src/ProjectIndexer.ts

+    const fromCache = cache.sources.get(fileName)
+    if (fromCache !== undefined) {
+      return fromCache
+    }
+    const result = hostCopy.getSourceFile(fileName, languageVersion)
+    if (result !== undefined) {
+      cache.sources.set(fileName, result)
+    }
+    return result


suggestion: Maybe add a comment here or near the cache definition which says "It is safe use the file name as a key here because we assume that a source file only ever belongs to a single project, and is only ever type-checked with a single language version".

Good call! I added an additional check to additionally only cache when languageVersion is unchanged (excluding a setExternalModuleIndicator field that is a function that is never the same. I also added a comment explaining in more details. I also discovered that getSourceFile has two optional parameters that we're now properly forwarding.

varungandhi-src · 2022-10-17T23:26:27Z

Very nice! ~~How did you recognize that this approach of caching the SourceFile values would help speed up type-checking?~~ (Nvm, saw that you wrote this in the issue.)

varungandhi-src · 2022-10-17T23:27:47Z

src/CommandLineOptions.test.ts

@@ -24,7 +24,6 @@ function checkIndexParser(

 // defaults
 checkIndexParser([], {
-  progressBar: true,


Why did this get removed? Shouldn't there be an extra globalCaches: true here instead?

I forgot to mention it in the description but this PR also disables the fancy progress bar by default. I didn't measure any performance difference with it enabled/disabled, but I found it distracting while testing projects. scip-typescript runs >99% of the time in CI where you should disable it by default anyways and I'm guessing most people are not doing it.

olafurpg

Thank you for the review! 🙏🏻

olafurpg · 2022-10-18T06:39:16Z

src/CommandLineOptions.test.ts

@@ -24,7 +24,6 @@ function checkIndexParser(

 // defaults
 checkIndexParser([], {
-  progressBar: true,


I forgot to mention it in the description but this PR also disables the fancy progress bar by default. I didn't measure any performance difference with it enabled/disabled, but I found it distracting while testing projects. scip-typescript runs >99% of the time in CI where you should disable it by default anyways and I'm guessing most people are not doing it.

olafurpg · 2022-10-18T06:40:42Z

src/ProjectIndexer.ts

+    if (!hostCopy.getParsedCommandLine) {
+      return undefined
+    }
+    if (cache.parsedCommandLines.has(fileName)) {
+      return cache.parsedCommandLines.get(fileName)
+    }


I may be misunderstanding your comment, but we guard against hostCopy.getParsedCommandLine at the top, so even if the cache has a hit we return undefined when hostCopy.getParsedCommandLine is undefined.

olafurpg · 2022-10-18T06:42:32Z

src/ProjectIndexer.ts

+    const fromCache = cache.sources.get(fileName)
+    if (fromCache !== undefined) {
+      return fromCache
+    }
+    const result = hostCopy.getSourceFile(fileName, languageVersion)
+    if (result !== undefined) {
+      cache.sources.set(fileName, result)
+    }
+    return result


Good call! I added an additional check to additionally only cache when languageVersion is unchanged (excluding a setExternalModuleIndicator field that is a function that is never the same. I also added a comment explaining in more details. I also discovered that getSourceFile has two optional parameters that we're now properly forwarding.

olafurpg requested a review from varungandhi-src October 17, 2022 20:41

olafurpg mentioned this pull request Oct 17, 2022

Process packages in parallel #175

Open

Fix failing tests

d7329f6

varungandhi-src reviewed Oct 17, 2022

View reviewed changes

varungandhi-src approved these changes Oct 17, 2022

View reviewed changes

Address review comments

e732a03

olafurpg commented Oct 18, 2022

View reviewed changes

olafurpg merged commit 21f18f1 into main Oct 18, 2022

olafurpg deleted the olafurpg/perf branch October 18, 2022 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimization: cache results between TypeScript projects #182

Optimization: cache results between TypeScript projects #182

olafurpg commented Oct 17, 2022

varungandhi-src Oct 17, 2022

olafurpg Oct 18, 2022

varungandhi-src Oct 17, 2022

olafurpg Oct 18, 2022

varungandhi-src commented Oct 17, 2022 •

edited

Loading

varungandhi-src Oct 17, 2022

olafurpg Oct 18, 2022

olafurpg left a comment

olafurpg Oct 18, 2022

olafurpg Oct 18, 2022

olafurpg Oct 18, 2022

Optimization: cache results between TypeScript projects #182

Optimization: cache results between TypeScript projects #182

Conversation

olafurpg commented Oct 17, 2022

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varungandhi-src commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olafurpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varungandhi-src commented Oct 17, 2022 •

edited

Loading