Skip to content

Fix too many watcher instances issue #6026

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.

Sign up for GitHub

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

Merged
merged 16 commits into from
Jan 14, 2016
Merged

Fix too many watcher instances issue #6026

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
e675744
Fix too many watcher instances issue
zhengbli Dec 10, 2015
5fa7bec
revert back to polling watching for approaching release
zhengbli Dec 11, 2015
36cc0e0
fix linter errors
zhengbli Dec 11, 2015
d419015
Use FileMap instead of string array
zhengbli Dec 15, 2015
602cde4
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
zhengbli Dec 15, 2015
178b2da
Add type alias for filewatching callbacks
zhengbli Dec 22, 2015
caa6eb4
Reuse watchers between 'watchDirectory' and 'watchFile'
zhengbli Dec 22, 2015
631363f
Fix lint issues
zhengbli Dec 22, 2015
56a7217
Merge branch 'master' of https://github.com/Microsoft/TypeScript into…
zhengbli Dec 23, 2015
8cf1a34
enable more than one callbacks for a watched file
zhengbli Dec 28, 2015
697644c
spell our dir to directory
zhengbli Jan 8, 2016
114d2bd
Separate directory watching and file watching again to reduce logic c…
zhengbli Jan 11, 2016
d22626f
Fix lint issue
zhengbli Jan 12, 2016
02531af
Update the watchedFileSet to use Path instead of string for file names
zhengbli Jan 13, 2016
067573b
cr feedback: simplify the removeFile function
zhengbli Jan 13, 2016
dbfe862
not casting relative filenames in 'tsc watch' to Path
zhengbli Jan 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 50 additions & 9 deletions src/compiler/sys.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ namespace ts {
interface WatchedFile {
fileName: string;
callback: (fileName: string, removed?: boolean) => void;
mtime: Date;
mtime?: Date;
}

export interface FileWatcher {
Expand Down Expand Up @@ -218,7 +218,7 @@ namespace ts {

// average async stat takes about 30 microseconds
// set chunk size to do 30 files in < 1 millisecond
function createWatchedFileSet(interval = 2500, chunkSize = 30) {
function createPollingWatchedFileSet(interval = 2500, chunkSize = 30) {
Copy link
Contributor

Choose a reason for hiding this comment

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

so why do we need this Polling one, why can not we just fall back to fs.watchFile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our polling mechanism is different from fs.watchFile. fs.watchFile polls each file independently, so if you got many files to watch, it might increase the work load a lot. In our approach, we poll a fixed number of files (chunkSize) each number, so if the chunkSize is 30 and you have 90 files to watch, the work load is still the same for each polling, only that changes will be detected after 3 interval time instead of just 1. This way we keep the polling work load consistent

let watchedFiles: WatchedFile[] = [];
let nextFileToCheck = 0;
let watchTimer: any;
Expand Down Expand Up @@ -293,6 +293,50 @@ namespace ts {
removeFile: removeFile
};
}

function createWatchedFileSet() {
let watchedDirectories: { [path: string]: FileWatcher } = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

use FileMap instead.

let watchedFiles: { [fileName: string]: (fileName: string, removed?: boolean) => void; } = {};

function addFile(fileName: string, callback: (fileName: string, removed?: boolean) => void): WatchedFile {
const file: WatchedFile = { fileName, callback };
let watchedPaths = Object.keys(watchedDirectories);
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure that I understand why do we use map-list structure if ultimately it is still doing linear scan over the array of keys (getting those also incur extra array allocation)

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 each directory path corresponds to a filewatcher that we may need to call its close method later. For better performance maybe I can use a separate string array for searching and a map to look for watcher. Are there better alternatives?

// Try to find parent paths that are already watched. If found, don't add directory watchers
let watchedParentPaths = watchedPaths.filter(path => fileName.indexOf(path) === 0);
// If adding new watchers, try to find children paths that are already watched. If found, close them.
Copy link
Member

Choose a reason for hiding this comment

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

"If adding new watchers, we need to close any existing watchers for subdirectories."

if (watchedParentPaths.length === 0) {
let pathToWatch = ts.getDirectoryPath(fileName);
for (let watchedPath in watchedDirectories) {
if (watchedPath.indexOf(pathToWatch) === 0) {
watchedDirectories[watchedPath].close();
delete watchedDirectories[watchedPath];
}
Copy link
Contributor

Choose a reason for hiding this comment

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

consider replacing these four lines with:

removeDirWatcher(getDirectoryPath(filePath));

}
watchedDirectories[pathToWatch] = _fs.watch(
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

pathToWatch,
(eventName: string, relativeFileName: string) => fileEventHandler(eventName, ts.normalizePath(ts.combinePaths(pathToWatch, relativeFileName)))
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to do this every time? is the relativeFileName expected to change between invocations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thing is we don't know what the relative file name is "relative" to. For example, we may have a watcher for C:\project\test and later on replaced by a higher level C:\project because turns out we need to watch files in that folder as well, so _fs.watch returns different relative path for the same file in the C:\project\test folder. Therefore we have to convert it to absolute path.

);
}
watchedFiles[fileName] = callback;
return { fileName, callback }
}

function removeFile(file: WatchedFile) {
delete watchedFiles[file.fileName];
Copy link
Contributor

Choose a reason for hiding this comment

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

but we are still watching the directory anyways? when does a directory watcher ever die?

}

function fileEventHandler(eventName: string, fileName: string) {
if (watchedFiles[fileName]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

hasProperty

let callback = watchedFiles[fileName];
callback(fileName);
}
}

return {
Copy link
Member

Choose a reason for hiding this comment

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

Move this prior to the function declarations

addFile: addFile,
removeFile: removeFile
Copy link
Member

Choose a reason for hiding this comment

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

Use shorthand properties to avoid repetition, or move the function declaration to use method syntax.

}
}

// REVIEW: for now this implementation uses polling.
// The advantage of polling is that it works reliably
Expand All @@ -307,6 +351,7 @@ namespace ts {
// changes for large reference sets? If so, do we want
// to increase the chunk size or decrease the interval
// time dynamically to match the large reference set?
const pollingWatchedFileSet = createPollingWatchedFileSet();
const watchedFileSet = createWatchedFileSet();

function isNode4OrLater(): Boolean {
Expand Down Expand Up @@ -411,14 +456,10 @@ namespace ts {
// and is more efficient than `fs.watchFile` (ref: https://github.com/nodejs/node/pull/2649
// and https://github.com/Microsoft/TypeScript/issues/4643), therefore
// if the current node.js version is newer than 4, use `fs.watch` instead.
if (isNode4OrLater()) {
// Note: in node the callback of fs.watch is given only the relative file name as a parameter
return _fs.watch(fileName, (eventName: string, relativeFileName: string) => callback(fileName));
}

const watchedFile = watchedFileSet.addFile(fileName, callback);
let fileSet = isNode4OrLater() ? watchedFileSet : pollingWatchedFileSet;
const watchedFile = fileSet.addFile(fileName, callback);
return {
close: () => watchedFileSet.removeFile(watchedFile)
close: () => fileSet.removeFile(watchedFile)
};
},
watchDirectory: (path, callback, recursive) => {
Expand Down