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

Optimize compiler filesystem caching #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

markerikson
Copy link
Contributor

This PR:

  • Optimizes the compiler wrapping by caching results that were getting re-requested numerous times:
    • Resolved TS modules
    • File source contents
  • Tweaks the module resolution logic for "files" that are part of the snippet codeblocks with relative imports - specifying an absolute path seemed to help

Copy link
Owner

@phryneas phryneas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly nitpicking because a lot of these changes are functionally not doing anything and I assume they were at some point introduced during debugging.

The only thing that I'd ask you to look into would be if this would affect watch mode, and if it does we probably need to make the caching configurable.

Comment on lines +61 to +62
const filenames = Object.keys(files);
this.compilerHost.setScriptFileNames(filenames);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a bunch of these going on for logging purposes - could you undo them so we have the diff a little bit smaller?

Comment on lines +127 to +129
const result =
!!virtualFiles[normalize(fileName)] || ts.sys.fileExists(fileName);
return result;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also pulled apart for logging purposes I guess?

Comment on lines +139 to +151
if (normalized in cachedFiles) {
const cacheResult = cachedFiles[fileName];

if (cacheResult) {
return cacheResult;
}
}

const contents = ts.sys.readFile(fileName);

if (contents) {
cachedFiles[normalized] = contents;
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you verify that this doesn't cause problems in watch mode?
If it does, we might need a flag to swap behaviours here.

@@ -166,7 +190,7 @@ function createCompilerHost(
);
},
resolveModuleNames(moduleNames, containingFile) {
return moduleNames.map((moduleName) => {
const mappedModules = moduleNames.map((moduleName) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also stay a direct return and was only pulled apart for logging I think?

Comment on lines +220 to +227
const resolvedModule = ts.resolveModuleName(
newModuleName,
containingFile,
compilerOptions,
this
).resolvedModule;

return resolvedModule;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const resolvedModule = ts.resolveModuleName(
newModuleName,
containingFile,
compilerOptions,
this
).resolvedModule;
return resolvedModule;
return ts.resolveModuleName(
newModuleName,
containingFile,
compilerOptions,
this
).resolvedModule;


let cachedFiles: Record<string, string | undefined> = {};

const host: ModifiedCompilerHost = {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this could also stay a direct return?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants