Watch improvements in tsserver #17269

sheetalkamat · 2017-07-18T17:47:48Z

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.

Add tests and bug ids corresponding to this: Fixes TSServer High CPU Usage #14655, Resolving modules and Intellisense Autocompletion pretty slow #16329, Typescript service is often stopping working randomly: Cannot read property 'charCount' of undefined #16456, ConfigFileDiagnosticEvent fires for files not selected by tsconfig #16635, NPM Install fails when tsconfig is present in VSCode workspace #16955, Log all 'watch' events #17177, Log reason refreshInferredProjects was called #17178, TSServer Reference requests taking a long time #17385, TypeScript watcher consumes large amount of CPU when idle. #17506, Probably FATAL ERROR: CALL_AND_RETRY_LAST Allocation failed - JavaScript heap out of memory #18411 since it talks about leak with tsc --watch
Separate PR of this branch to pull out builder API into compiler so that --watch can use it as well - Improvements to tsc --watch #17669 Fixes: Optimize tsc --w using the CoS builder #10879
Work out if we should watch tsconfig files/directory instead for (configured project as well as inferred root)
Event on project graph update or such so clients can query the info/errors etc - Event to notify project changes #17820. Fixes Expose ContextEvent in TS Server Protocol #17659
We should potentially watch extended config file as well - Filed bug Apart from watching config file for the project updates, we probably should also watch extended config file #17753

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

…n reloading FS

…e files open from that project

… 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

…all the time

…nce we watch the needed files

…t read directory or getDirectories

…batching the updates when there are multiple files added/removed/changed

…ject root

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

…in case of inferred root

…em as CachedDirectoryStructureHost

ghost · 2017-09-29T18:45:27Z

src/compiler/builder.ts

+                }
+
+                // Return array of values that needs emit
+                return arrayFrom(seenFileNamesMap.values());


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

ghost · 2017-09-29T18:47:50Z

src/compiler/builder.ts

+        signature: string;
+    }
+
+    export function createBuilder(


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.)

ghost · 2017-09-29T18:49:55Z

src/compiler/builder.ts

+                getFilesAffectedByUpdatedShape
+            };
+
+            function getFilesAffectedByUpdatedShape(program: Program, _sourceFile: SourceFile, singleFileResult: string[]): string[] {


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[].

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

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).

sheetalkamat · 2017-10-02T19:35:30Z

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

mhegazy · 2017-10-02T20:11:17Z

src/compiler/builder.ts

+        text: string;
+    }
+
+    export interface ChangedProgramFiles {


do you still need this?

mhegazy · 2017-10-02T20:23:26Z

src/compiler/builder.ts

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


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

Nit, i would call program as newProgram.

…red out final details

mhegazy · 2017-10-02T23:40:05Z

src/compiler/builder.ts

+        clear(): void;
+    }
+
+    interface EmitHandler {


this should be intrnal too

consider spiting this into two namespace ts { declarations

This isnt public interface anyways ?

mhegazy · 2017-10-02T23:50:05Z

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.

…terfaces and functions

mihailik · 2017-10-03T11:32:33Z

src/compiler/core.ts

+     * Tests whether a value is string
+     */
+    export function isString(text: any): text is string {
+        return typeof text === "string";


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.

sheetalkamat added 30 commits July 10, 2017 11:31

When opening a file, if it is using existing project, there is no nee…

21ad26b

…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

Restructure updating the configured project from disk and actual proj…

ae33ae8

…ect update Also reload the projects when extra extension in the host change

Add project roots in the list of root files even if they arent presen…

75698a8

…t on the disk This helps in reporting errors as well as syncing of the configured/external project when the files are created

Move the server file system to handle watches on file/folder create, …

f154910

…update and delete This will ensure that the calling of watches doesnt rely on writing test correctly

Remove the functions to trigger watch callbacks now that it is auto o…

0e44367

…n reloading FS

Update the todo list

2a63827

Reload the configured project only once even though there are multipl…

96ffd53

…e files open from that project

When config file is deleted, apart from removing the projecty, reload…

6bd42b8

… 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

Handle the deleted file in update graph better, so that in next updat…

9ff9476

…e it is possible to schedule the update graph of project

Only update file list when there are changes in the watched directories

62871cc

Update types instantly when the type root changes.

48c6513

Cache the read directory results so that it doesnt end up reading it …

19a6a00

…all the time

Use the cached file exists/directory exists for configured project si…

68def1b

…nce we watch the needed files

Fixes the fourslash runner tests by handling hosts that cannot suppor…

029b1f2

…t read directory or getDirectories

Remove the done TODO

f338a70

TODOs for the scheduling update graph and referesh projects

8fedcf7

Allows the delayed update graph and project structure which helps in …

e568976

…batching the updates when there are multiple files added/removed/changed

Merge branch 'master' into watchImprovements

048e67c

use single instance of getCanonicalFileName

0365901

Logging of the watch add/remove/event

404aa8f

Some refactoring to combine files removal from inferred project

71d79c6

Remove the duplcate error about no input files found

f12980d

Refactor root files addition/update for non inferred project

00011a5

Instead of watching directories, watch tsconfig files of inferred pro…

0572b15

…ject root

Use map for configured project instead of the array

62663a1

Combine the logic to find config file as well as the watch.

dcbd7b1

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

Added another todo as now we are watching too many files

62ef6b1

Reduce the number of watched config files by watching the chain only …

2439e7a

…in case of inferred root

Merge branch 'master' into watchImprovements

ff34a77

ghost mentioned this pull request Sep 22, 2017

Use prettier #18700

Closed

mhegazy mentioned this pull request Sep 22, 2017

Module resolution via "--paths" fails against declaration files generated alongside "--outfile:" #18311

Closed

Merge branch 'master' into watchImprovements

23acff5

msftgits removed the cla-not-required label Sep 26, 2017

sheetalkamat added 4 commits September 26, 2017 10:46

Rename watchedProgram.ts to watch.ts

14febe2

Renamed PartialSystem as DirectoryStructureHost and CachedPartialSyst…

38f3a2b

…em as CachedDirectoryStructureHost

PR feedback

68d3605

Merge branch 'master' into watchImprovements

9e08cae

ghost reviewed Sep 29, 2017

View reviewed changes

ghost mentioned this pull request Oct 2, 2017

geterr Stops Returning Results #16540

Closed

sheetalkamat added 2 commits October 2, 2017 12:22

PR feedback

835153b

Merge branch 'master' into watchImprovements

898559b

sheetalkamat force-pushed the watchImprovements branch from e38a707 to 898559b Compare October 2, 2017 19:35

ghost approved these changes Oct 2, 2017

View reviewed changes

mhegazy reviewed Oct 2, 2017

View reviewed changes

src/compiler/builder.ts Outdated

text: string;

}

export interface ChangedProgramFiles {

Copy link

Contributor

mhegazy Oct 2, 2017

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?

mhegazy reviewed Oct 2, 2017

View reviewed changes

ghost mentioned this pull request Oct 2, 2017

TSServer: Exception on executing command semantic check #15688

Closed

Making APIs as internal so that we can enable them after we have figu…

7f969e8

…red out final details

sheetalkamat force-pushed the watchImprovements branch from 6f35ad4 to 7f969e8 Compare October 2, 2017 23:29

Merge branch 'master' into watchImprovements

4bb4711

mhegazy reviewed Oct 2, 2017

View reviewed changes

Separate namespace declarations in builder of intenal and exported in…

8ac01d7

…terfaces and functions

sheetalkamat merged commit 6997e9b into master Oct 3, 2017

sheetalkamat deleted the watchImprovements branch October 3, 2017 00:38

mihailik reviewed Oct 3, 2017

View reviewed changes

mhegazy mentioned this pull request Oct 4, 2017

TypeScript watcher consumes large amount of CPU when idle. #17506

Closed

microsoft locked and limited conversation to collaborators Jun 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Watch improvements in tsserver #17269

Watch improvements in tsserver #17269

sheetalkamat commented Jul 18, 2017 •

edited

Loading

ghost Sep 29, 2017

ghost Sep 29, 2017

ghost Sep 29, 2017

sheetalkamat Oct 2, 2017

ghost Oct 2, 2017

sheetalkamat commented Oct 2, 2017

mhegazy Oct 2, 2017

mhegazy Oct 2, 2017

mhegazy Oct 2, 2017

mhegazy Oct 2, 2017

mhegazy Oct 2, 2017

sheetalkamat Oct 3, 2017

mhegazy commented Oct 2, 2017

mihailik Oct 3, 2017

Watch improvements in tsserver #17269

Watch improvements in tsserver #17269

Conversation

sheetalkamat commented Jul 18, 2017 • edited Loading

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

Choose a reason for hiding this comment

sheetalkamat commented Oct 2, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhegazy commented Oct 2, 2017

Choose a reason for hiding this comment

sheetalkamat commented Jul 18, 2017 •

edited

Loading