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

Watch improvements in tsserver #17269

Merged
merged 128 commits into from
Oct 3, 2017
Merged

Watch improvements in tsserver #17269

merged 128 commits into from
Oct 3, 2017

Conversation

sheetalkamat
Copy link
Member

@sheetalkamat sheetalkamat commented Jul 18, 2017

This is the first stage of pulling out builder api. It factors out a lot of things that project did, when it was reloaded, when/how we lookup for the tsconfig files, and what we cache.
It will fix many bugs and I will include that list as I add the test scenarios for them.

Here is detailed list of changes done as part of this PR:

  • When opening a file, if it is using existing project, there is no need to update the project by re-reading the config file This will improve the opening file perf for file opens from same config project
  • Instead of doing disk IO on file watcher callback invoke, use the FileWatcherStatus to determine the action to take
  • Do not re-read and reload the project from disk if files are added/removed from watched directories. Instead just recreate the file list using pattern matching and update the program
  • Configured project and external projects now have the root file names even if they are missing on the disk. This helps in reporting errors as well as syncing the projects when files are created.
  • When reloading configured project from disk because of change in config file or say reload request received, reload configured project once even if there are multiple files open from that project
  • Updates and corrects all file/directory callback actions. Eg. when config file is deleted, remove the configured project, but also reload the project for the open files from this project so it can be picked up by the parent config file if any
  • Optimizes, wild card watchers and config directory watching since we watch missing files. We dont need to explicitly watch config file directory as it will be watched:
    • if there was no files specified, in wild card directories
    • if there were files specified as missing file (if the file wasnt present)
  • Handles deleted file in the resolution cache a little better, so that the update graph can be postponed, instead of doing it right away
  • When compiling/having project with config file open, cache the directory structure/file existence so that when calculating file names for the project, we dont end up doing file IO all the time
  • We now delay the project updates and batch them together when there are changes because of watcher invoked callbacks.
  • Better logging when file/ directories are watched
  • Config files are watched for inferred project instead of watching their directories.
  • We also cache that result/use configured project to determine config file existence on each file open. This helps in fast open of files from same project
  • Also the config files are watched as the files are opened so this avoids the reiterating over the folder structure to create those watches
  • Move the builder to compiler so that it can be used by tsc --watch as well.
  • Resolution cache and file system cache is also reused by tsserver as well as tsc --watch
  • In tsserver, do not update graph in builder if compile on save is not on
  • Simple optimization, to not update fileNames when program is reused just so we can update list of missing files
  • tsc --watch uses builder to emit files
  • Watch resolved module names - failed lookup location so as to update the graph on their creation
  • We now invalidate source file even if it didnt change, but its module resolution might have changed, so that we can re-evaluate those resolutions
  • tsc --watch also gets the semantic diagnostics from the builder so that it stores the error list per file and updates it only for the changed files. (we could technically report only changed errors but thats not done as part of this change)
  • Since the closed script info's that are orphan arent removed immediately but on next open request, treat the orphan script infos as if they are not present in the session
  • For directory watchers - previously if the directory didnt exist it created no-op watcher whose callback would never be created. Replaced this by adding watcher to parent directory and switching it back to directory once the directory is created.
  • Script info while opening or closing shouldnt mark project dirty if their contents hasnt changed
  • Watch config file directory if the failed lookup is in that tree so as to optimize the directories being watched
  • Resolve the module name only once in the directory and reuse the result for the module name in same directory
  • Configured project is not removed when closing the last file it contains.. Instead it is still kept alive and reused if next file open is from the same project. We will close all the config files with no open files on the next file open request
  • Resolved module name result is reused through next program structure compute unless the resolution was invalidated (because the failed lookup location file was added/removed, file it resolved to was deleted)
  • Update the way we get project/script info for operation so that project graph isnt updated unnecessarily
  • Event "projectsUpdatedInBackground" is sent with current list of open files, so that error check can be scheduled for the open files when there is update in background (due to watch callbacks and not user initiated)

…d to update the project by re-reading the config file

This will improve the opening file perf for file opens from same config project
…ect update

Also reload the projects when extra extension in the host change
…t on the disk

This helps in reporting errors as well as syncing of the configured/external project when the files are created
…update and delete

This will ensure that the calling of watches doesnt rely on writing test correctly
… the configured projects for open files from that project to ensure to pick them by another config file that can be present in parent directory
…ve missing file watching as well

We dont need to explicitly watch config file directory as it will be watched:
- if there was no files specified, in wild card directories
- if there were files specified as missing file (if the file wasnt present)
…e it is possible to schedule the update graph of project
…batching the updates when there are multiple files added/removed/changed
This should speed up the file open scenarios where the file belongs to same configured project as we would use cache to answer those fileExists answers
}

// Return array of values that needs emit
return arrayFrom(seenFileNamesMap.values());
Copy link

Choose a reason for hiding this comment

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

Should this filter out just the non-undefined values? seenFileNamesMap will have an undefined entry for anything that shouldn't be emitted.

signature: string;
}

export function createBuilder(
Copy link

Choose a reason for hiding this comment

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

Do we want this to be a new public API?
(If so it would be a good idea to take an options object instead of taking many positional arguments.)

getFilesAffectedByUpdatedShape
};

function getFilesAffectedByUpdatedShape(program: Program, _sourceFile: SourceFile, singleFileResult: string[]): string[] {
Copy link

Choose a reason for hiding this comment

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

It looks like this would be simpler if it took and returned SourceFile[] instead of string[].
Then you can call onAffectedFile(file.fileName, file); instead of onAffectedFile(file, program.getSourceFile(file));
(Also, since file.fileName can trivially be derived from file, you could skip passing that into the callback too.)
You might similarly change the return type of getFilesAffectedBy. to SourceFile[].

Copy link
Member Author

Choose a reason for hiding this comment

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

getFilesAffectedByUpdatedShape is used by project to get affected file names, so this works out better than sending source files so that project doesnt need to iterate on the list to convert that to file names.. In rest of the cases, you can always get the source file from file name

Copy link

Choose a reason for hiding this comment

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

But, it's simpler to get the filename from a source file (sf.fileName) than it is to get the source file from a file name (requires a lookup table).

@ghost ghost mentioned this pull request Oct 2, 2017
@sheetalkamat
Copy link
Member Author

@andy-ms and @mhegazy Please let me know if there is any other feedback

text: string;
}

export interface ChangedProgramFiles {
Copy link
Contributor

Choose a reason for hiding this comment

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

do you still need this?

/**
* This is the callback when file infos in the builder are updated
*/
onProgramUpdateGraph(program: Program, hasInvalidatedResolution: HasInvalidatedResolution): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

i do not think we need graph in the name of this function, users do not need to know how we represnet the files.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, i would call program as newProgram.

clear(): void;
}

interface EmitHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be intrnal too

Copy link
Contributor

Choose a reason for hiding this comment

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

consider spiting this into two namespace ts { declarations

Copy link
Member Author

Choose a reason for hiding this comment

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

This isnt public interface anyways ?

@mhegazy
Copy link
Contributor

mhegazy commented Oct 2, 2017

let's get the functional changes in, i would like to make some changes to the API, but we can make these in a separate PR.

@sheetalkamat sheetalkamat merged commit 6997e9b into master Oct 3, 2017
@sheetalkamat sheetalkamat deleted the watchImprovements branch October 3, 2017 00:38
* Tests whether a value is string
*/
export function isString(text: any): text is string {
return typeof text === "string";
Copy link
Contributor

Choose a reason for hiding this comment

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

This is likely a minor slow-down, for no benefit to readability.

Functions that are called with polymorphic arguments are quickly de-optimised. This one is specifically intended to be called with polymorphic arguments, so it's very likely to be de-optimised and become slower than conventional idiomatic typeof === 'string'.

Worth checking with a timed run.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

Expose ContextEvent in TS Server Protocol TSServer High CPU Usage Optimize tsc --w using the CoS builder
8 participants