Fix too many watcher instances issue #6026

zhengbli · 2015-12-10T00:28:03Z

Instead of creating a watcher for every single file, I create watchers only for the directory that the files are in, and matches the file change event with a list of registered files.

vladima · 2015-12-10T17:08:26Z

src/compiler/sys.ts

@@ -293,6 +293,50 @@ namespace ts {
                    removeFile: removeFile
                };
            }
+
+            function createWatchedFileSet() {
+                let watchedDirectories: { [path: string]: FileWatcher } = {};


watchedFiles and watchedDirectories do not take case sensitivity\slashes into account.

use FileMap instead.

zhengbli · 2015-12-11T02:02:42Z

@mhegazy @vladima due to the approaching release deadline and concerns from @bpasero, I'll revert the polling watchers for all cases and send another PR to address the CR feedbacks after the release.

DanielRosenwasser · 2015-12-11T04:51:00Z

src/compiler/sys.ts

+                    const file: WatchedFile = { fileName, callback };
+                    const watchedPaths = Object.keys(watchedDirectories);
+                    // Try to find parent paths that are already watched. If found, don't add directory watchers
+                    const watchedParentPaths = watchedPaths.filter(path => fileName.indexOf(path) === 0);


Use ts.filter

bpasero · 2015-12-11T06:37:06Z

Btw I don't really have concerns about this change it is totally the right way of doing it, but I am a bit nervous because VS Code is shipping next week with TS 1.7 and I want to make sure we are not introducing a regression late in the game. Back to polling for 1.7.x and then this right fix for 1.8 would be nice.

vladima · 2015-12-11T06:47:32Z

src/compiler/sys.ts

+                    // If adding new watchers, try to find children paths that are already watched. If found, close them. 
+                    if (watchedParentPaths.length === 0) {
+                        const pathToWatch = ts.getDirectoryPath(fileName);
+                        for (const watchedPath in watchedDirectories) {


this part is interesting.correct me please if I'm wrong, here you are saying: "As I see nobody is watching folder 'pathToWatch' so I'm going to install watcher for it. However it is possible that somebody is already watching 'pathToWatch/'. If yes - I'm going to shutdown this watcher since watcher for 'pathToWatch' should get the same notifications".

If the answer is 'yes' then I'm curious how it works since by default fs.watch is not recursive so if initially you had watcher for '/a/b/c/d' and then you install one for '/a/b' then after closing watcher for '/a/b/c/d' you'll receive no notifications about changes in this file

You are right, I was trying to use the recursive fs.watch but forgot to specify the {recursive: true} option.

Though on a second thought, this logic may be too complex when I need to close a unused directory watcher later. For non-recursive watchers I can just check if the direct parent folder has any files in the watching list, while for recursive watchers it's more complex. It might make more sense to check to the non-recursive version.

I would think twice before completely switching to recursive watchers or at least test it in different scenarios. Effectively it means that if user adds some file that is close to the root of the disk then watcher will receive tons of notifications about files he doesn't care which might (and will) affect performance

Good point, noted. Thanks!

zhengbli · 2015-12-11T07:32:26Z

@bpasero @vladima @DanielRosenwasser for the approaching release 1.7.5 I sent another PR (#6066) that rolls back to polling watchers. This PR will be updated according to code reviews and tested after the release to reduce risks.

bpasero · 2015-12-11T08:32:53Z

👍

mihailik · 2015-12-11T10:02:45Z

src/compiler/sys.ts

+                                delete watchedDirectories[watchedPath];
+                            }
+                        }
+                        watchedDirectories[pathToWatch] = _fs.watch(


Isn't it sloppy in regards of hasOwnProperty, toString that may exist as files and clash with members on watchDirectories object?

true and this is one more argument to switch to FileMap which handles this case correctly

… i6016

zhengbli · 2015-12-15T16:50:27Z

@vladima @DanielRosenwasser @mhegazy Updated this PR using FileMap, please let me know if you have further comments. Thanks!

billti · 2015-12-15T17:32:38Z

src/compiler/sys.ts

+
+                function addFile(fileName: string, callback: (fileName: string, removed?: boolean) => void): WatchedFile {
+                    const path = toPath(fileName, currentDirectory, getCanonicalPath);
+                    const parentDirPath = toPath(ts.getDirectoryPath(fileName), currentDirectory, getCanonicalPath);


Shouldn't you getDirectoryPath on the path you just created in the line above, rather than the fileName which may not be a (normalized) full path?

toPath should take care of it, however the valid point here is that separators in fileName might still vary and getDirectoryPath expects them to be only forward slashes. I agree with @billti that it will be simplier to just call const parentDirPath = getDirectoryPath(path)

also given that getDirectoryPath does not reshape its argument so Path should stay Path I think we can add an overload to getDirectoryPath to avoid redundant conversions: smth like this:

export function getDirectoryPath(path: Path): Path; export function getDirectoryPath(path: string): string; export function getDirectoryPath(path: string): any { return path.substr(0, Math.max(getRootLength(path), path.lastIndexOf(directorySeparator))); }

In this case you can just can call getDirectoryPath(path) and immediately get parentDirPath

billti · 2015-12-15T18:00:56Z

src/compiler/sys.ts

                    return {
-                        close: () => watchedFileSet.removeFile(watchedFile)
+                        close: () => watchSet.removeFile(watchedFile)


"watchDirectory" (line 474 below) also talks about Node 4 or later and uses the 'recursive' option, but doesn't actually check for Node 4 or later.

I can see in "tsc.ts" and "editorServices.ts" it is called with "recursive: true". Are there potentially issues here if not on Node v4?

I tested before that if not supported then the recursive: true would simply be ignored. The recursive directory watcher is used with tsconfig.json without files array, so that adding/removing files in the background would be detected. The plan for this feature at the time was to support Node v4 or later only, so I didn't differentiate with the cases with older Node version.

I'm not sure i follow about the plan to use the feature. watchDirectory is a public API (exported on interface System). If when called, it's not going to be able to do what the API claims it does (via the recursive parameter`), then it should error.

Even if not a public API, it's a future bug waiting to happen if it ignores known failure scenarios based on assumptions about how it will be used. Please protect against it.

… i6016

vladima · 2015-12-23T00:43:11Z

src/compiler/sys.ts

+            function createWatchedFileSet() {
+                const dirWatchers = createFileMap<DirWatcher>();
+                const recursiveDirWatchers = createFileMap<DirWatcher>();
+                const fileWatcherCallbacks = createFileMap<FileWatcherCallback>();


as I see now we store one callback per file, a question: will this work?

const fileName = "/some/file"; ts.sys.watchFile(fileName, () => console.log(1)); ts.sys.watchFile(fileName, () => console.log(2));

Ah it wouldn't work now. Maybe a file needs a list of callbacks. Will update later.

mhegazy · 2016-01-08T00:55:26Z

src/compiler/sys.ts

@@ -1,6 +1,9 @@
 /// <reference path="core.ts"/>

 namespace ts {
+    export type FileWatcherCallback = (path: string, removed?: boolean) => void;
+    export type DirWatcherCallback = (path: string) => void;


spell out "Dir" into "DirectoryWatcherCallback"

…omplexity, because reference counting is a lot easier in this case

zhengbli · 2016-01-11T19:40:00Z

After some offline discussion with @mhegazy, we decided that it would be easier not to mix the recursive watchers and non-recursive watchers together, because the logic for reference counting would be quite complex in that case. Therefore, I rolled back to the implementation with separate directory watchers and file watchers, which might have duplicated watchers for the same folder in some case, but the compatibility across platforms would be better, and the code would be more maintainable.

mhegazy · 2016-01-12T21:47:24Z

src/compiler/sys.ts

+                            const watchedDir = findWatchedDirForFile(filePath);
+                            if (watchedDir) {
+                                reduceDirWatcherRefCount(watchedDir);
+                            }


consider replacing these four lines with:

removeDirWatcher(getDirectoryPath(filePath));

mhegazy · 2016-01-14T01:28:48Z

👍

Fix too many watcher instances issue

Fix too many watcher instances issue

e675744

msftclas added the cla-not-required label Dec 10, 2015

zhengbli mentioned this pull request Dec 10, 2015

1.7: TS Server opens a file handle for watching for each file in the project results in EMFILE error #6016

Closed

vladima reviewed Dec 10, 2015
View reviewed changes

zhengbli added 2 commits December 10, 2015 17:54

revert back to polling watching for approaching release

5fa7bec

fix linter errors

36cc0e0

DanielRosenwasser reviewed Dec 11, 2015
View reviewed changes

vladima reviewed Dec 11, 2015
View reviewed changes

zhengbli mentioned this pull request Dec 11, 2015

Roll back to polling file watching for 1.7 #6066

Merged

mihailik reviewed Dec 11, 2015
View reviewed changes

zhengbli added 2 commits December 15, 2015 08:39

Use FileMap instead of string array

d419015

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

602cde4

… i6016

billti reviewed Dec 15, 2015
View reviewed changes

zhengbli added 4 commits December 21, 2015 17:02

Add type alias for filewatching callbacks

178b2da

Reuse watchers between 'watchDirectory' and 'watchFile'

caa6eb4

Fix lint issues

631363f

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

56a7217

… i6016

vladima reviewed Dec 23, 2015
View reviewed changes

enable more than one callbacks for a watched file

8cf1a34

mhegazy mentioned this pull request Jan 8, 2016

tsc -w is unacceptably slow #6178

Closed

mhegazy reviewed Jan 8, 2016
View reviewed changes

zhengbli added 2 commits January 7, 2016 22:48

spell our dir to directory

697644c

Separate directory watching and file watching again to reduce logic c…

114d2bd

…omplexity, because reference counting is a lot easier in this case

Fix lint issue

d22626f

mhegazy reviewed Jan 12, 2016
View reviewed changes

zhengbli added 2 commits January 13, 2016 13:18

Update the watchedFileSet to use Path instead of string for file names

02531af

cr feedback: simplify the removeFile function

067573b

zhengbli force-pushed the i6016 branch from 24d9940 to 067573b Compare January 13, 2016 22:03

not casting relative filenames in 'tsc watch' to Path

dbfe862

zhengbli added a commit that referenced this pull request Jan 14, 2016

Merge pull request #6026 from zhengbli/i6016

d5d7a2a

Fix too many watcher instances issue

zhengbli merged commit d5d7a2a into microsoft:master Jan 14, 2016

microsoft locked and limited conversation to collaborators Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix too many watcher instances issue #6026

Fix too many watcher instances issue #6026

zhengbli commented Dec 10, 2015

vladima Dec 10, 2015

mhegazy Dec 10, 2015

zhengbli commented Dec 11, 2015

DanielRosenwasser Dec 11, 2015

bpasero commented Dec 11, 2015

vladima Dec 11, 2015

zhengbli Dec 11, 2015

vladima Dec 11, 2015

zhengbli Dec 11, 2015

zhengbli commented Dec 11, 2015

bpasero commented Dec 11, 2015

mihailik Dec 11, 2015

vladima Dec 11, 2015

zhengbli commented Dec 15, 2015

billti Dec 15, 2015

vladima Dec 15, 2015

vladima Dec 15, 2015

billti Dec 15, 2015

zhengbli Dec 16, 2015

billti Dec 16, 2015

vladima Dec 23, 2015

zhengbli Dec 23, 2015

mhegazy Jan 8, 2016

zhengbli commented Jan 11, 2016

mhegazy Jan 12, 2016

mhegazy commented Jan 14, 2016

Fix too many watcher instances issue #6026

Fix too many watcher instances issue #6026

Conversation

zhengbli commented Dec 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli commented Dec 11, 2015

Choose a reason for hiding this comment

bpasero commented Dec 11, 2015

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

zhengbli commented Dec 11, 2015

bpasero commented Dec 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli commented Dec 15, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhengbli commented Jan 11, 2016

Choose a reason for hiding this comment

mhegazy commented Jan 14, 2016