Skip to content

Reuse program structure #3616

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
wants to merge 12 commits into from
Closed

Reuse program structure #3616

wants to merge 12 commits into from

Conversation

vladima
Copy link
Contributor

@vladima vladima commented Jun 24, 2015

Currently on every edit createProgram revisits all source files in order to discover all dependencies that are absent in root files and reconstruct the proper sequence of files. This is right thing to do however usually most of edits don't modify imports and tripleslash references so set and order of files remains the same. This PR tries to detect such cases: if edit does not affect the structure of the program then we'll just reuse nonchanged files from the old program keeping the same order.

@@ -5634,7 +5634,6 @@ namespace ts {
// will immediately bail out of walking any subtrees when we can see that their parents
// are already correct.
let result = Parser.parseSourceFile(sourceFile.fileName, newText, sourceFile.languageVersion, syntaxCursor, /* setParentNode */ true)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unnecessary change, will remove it from the PR


// if old program was provided by has different target module kind - assume that it cannot be reused
// different module kind can lead to different way of resolving modules
if (oldProgram && oldProgram.getCompilerOptions().module !== options.module) {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not move this to tryReuseStructureFromOldProgram and just return false if this true.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because in case if structure cannot be reused we still want to utilize previous resolutions, see processImports. However if old version of the program targeted different module kind - then resolutions should not be shared (not true currently but will be true after we incorporate different module resolution strategies). Presence or absence of old program acts as a marker if it can be reused

Copy link
Contributor

Choose a reason for hiding this comment

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

also noResolve can not change between the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

if you want to be conservative and check the whole compilerOptions properties i am fine with that.

Copy link
Contributor

Choose a reason for hiding this comment

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

and target.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 24, 2015

We should also put this in tsc.ts in the --watch implementation. it should simplify it a bit.

@@ -2471,6 +2472,8 @@ namespace ts {
let newSettings = hostCache.compilationSettings();
let changesInCompilationSettingsAffectSyntax = oldSettings && oldSettings.target !== newSettings.target;

let reusableOldProgram = changesInCompilationSettingsAffectSyntax ? undefined : program;
Copy link
Contributor

Choose a reason for hiding this comment

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

we can move the target check to create program right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@mhegazy
Copy link
Contributor

mhegazy commented Jun 25, 2015

👍

if (!skipDefaultLib) {
processRootFile(host.getDefaultLibFileName(options), /*isDefaultLib:*/ true);

if (oldProgram) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just add an explanatory comment. Something to give hte intuition for why you are checking these specific options, and the types of options you'd put here in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mhegazy
Copy link
Contributor

mhegazy commented Aug 5, 2015

this change moved to #4154

@mhegazy
Copy link
Contributor

mhegazy commented Aug 6, 2015

@vladima can we close this now

@vladima
Copy link
Contributor Author

vladima commented Aug 6, 2015

subsumed by #4154

@vladima vladima closed this Aug 6, 2015
@mhegazy mhegazy deleted the reuseProgramStructure branch August 6, 2015 23:52
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants