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

Reduce exceptions #44710

Merged
merged 3 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 10 additions & 2 deletions src/compiler/sys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1266,6 +1266,7 @@ namespace ts {
let activeSession: import("inspector").Session | "stopping" | undefined;
let profilePath = "./profile.cpuprofile";

let hitSystemWatcherLimit = false;

const Buffer: {
new (input: string, encoding?: string): any;
Expand Down Expand Up @@ -1619,6 +1620,12 @@ namespace ts {
options = { persistent: true };
}
}

if (hitSystemWatcherLimit) {
sysLog(`sysLog:: ${fileOrDirectory}:: Defaulting to fsWatchFile`);
return watchPresentFileSystemEntryWithFsWatchFile();
}

try {
const presentWatcher = _fs.watch(
fileOrDirectory,
Expand All @@ -1635,6 +1642,8 @@ namespace ts {
// Catch the exception and use polling instead
// Eg. on linux the number of watches are limited and one could easily exhaust watches and the exception ENOSPC is thrown when creating watcher at that point
// so instead of throwing error, use fs.watchFile
hitSystemWatcherLimit ||= e.code === "ENOSPC";
sysLog(`sysLog:: ${fileOrDirectory}:: Changing to fsWatchFile`);
return watchPresentFileSystemEntryWithFsWatchFile();
}
}
Expand All @@ -1656,7 +1665,6 @@ namespace ts {
* Eg. on linux the number of watches are limited and one could easily exhaust watches and the exception ENOSPC is thrown when creating watcher at that point
*/
function watchPresentFileSystemEntryWithFsWatchFile(): FileWatcher {
sysLog(`sysLog:: ${fileOrDirectory}:: Changing to fsWatchFile`);
return watchFile(
fileOrDirectory,
createFileWatcherCallback(callback),
Expand Down Expand Up @@ -1796,7 +1804,7 @@ namespace ts {
}

function readDirectory(path: string, extensions?: readonly string[], excludes?: readonly string[], includes?: readonly string[], depth?: number): string[] {
return matchFiles(path, extensions, excludes, includes, useCaseSensitiveFileNames, process.cwd(), depth, getAccessibleFileSystemEntries, realpath);
return matchFiles(path, extensions, excludes, includes, useCaseSensitiveFileNames, process.cwd(), depth, getAccessibleFileSystemEntries, realpath, directoryExists);
}

function fileSystemEntryExists(path: string, entryKind: FileSystemEntryKind): boolean {
Expand Down
6 changes: 4 additions & 2 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6490,7 +6490,7 @@ namespace ts {
}

/** @param path directory of the tsconfig.json */
export function matchFiles(path: string, extensions: readonly string[] | undefined, excludes: readonly string[] | undefined, includes: readonly string[] | undefined, useCaseSensitiveFileNames: boolean, currentDirectory: string, depth: number | undefined, getFileSystemEntries: (path: string) => FileSystemEntries, realpath: (path: string) => string): string[] {
export function matchFiles(path: string, extensions: readonly string[] | undefined, excludes: readonly string[] | undefined, includes: readonly string[] | undefined, useCaseSensitiveFileNames: boolean, currentDirectory: string, depth: number | undefined, getFileSystemEntries: (path: string) => FileSystemEntries, realpath: (path: string) => string, directoryExists: (path: string) => boolean): string[] {
path = normalizePath(path);
currentDirectory = normalizePath(currentDirectory);

Expand All @@ -6506,7 +6506,9 @@ namespace ts {
const visited = new Map<string, true>();
const toCanonical = createGetCanonicalFileName(useCaseSensitiveFileNames);
for (const basePath of patterns.basePaths) {
visitDirectory(basePath, combinePaths(currentDirectory, basePath), depth);
if (directoryExists(basePath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@amcasey I wonder if this check is causing a behavior change in the material-ui benchmark and changing the number of .d.ts files that get loaded or similar.

Copy link
Member

Choose a reason for hiding this comment

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

Comparing --extendedDiagnostics might be a good first step to figuring out whether there really is an observable behavior change, if you haven't already done that @amcasey

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, we'd been discussing this offline and I forgot to update the issue. The perf suite was using the wrong baseline. I'll kick off a clean run now, but local testing showed no change in the check time of material-ui.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I should also say that I don't believe it will cause a behavioral change because visitDirectory would just see an empty list of fs entries for the non-existent directory and stop.

The latest perf run shows no change, as expected. 😄

Copy link

@gretzkiy gretzkiy Sep 15, 2021

Choose a reason for hiding this comment

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

@amcasey Hi! I don't know if it the right place to ask a question but I found this thread relative to the problem (I'm sorry if it's not). Should directoryExists be called with an absolute path like directoryExists(combinePaths(currentDirectory, basePath))?

I came across the undocumented breaking change in 4.4-beta that causes (for example) ts.sys.readDirectory("", [".ts",".tsx",".d.ts"],["node_modules","dist"],["**/*"]) to return an empty array. Before 4.4-beta it has been returning the correct array of files. So I wonder if it is a bug or desired behaviour.

Thank you in advance.

visitDirectory(basePath, combinePaths(currentDirectory, basePath), depth);
}
}

return flatten(results);
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/watchUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ namespace ts {
const rootResult = tryReadDirectory(rootDir, rootDirPath);
let rootSymLinkResult: FileSystemEntries | undefined;
if (rootResult !== undefined) {
return matchFiles(rootDir, extensions, excludes, includes, useCaseSensitiveFileNames, currentDirectory, depth, getFileSystemEntries, realpath);
return matchFiles(rootDir, extensions, excludes, includes, useCaseSensitiveFileNames, currentDirectory, depth, getFileSystemEntries, realpath, directoryExists);
}
return host.readDirectory!(rootDir, extensions, excludes, includes, depth);

Expand Down
2 changes: 1 addition & 1 deletion src/harness/fakesHosts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ namespace fakes {
}

public readDirectory(path: string, extensions?: readonly string[], exclude?: readonly string[], include?: readonly string[], depth?: number): string[] {
return ts.matchFiles(path, extensions, exclude, include, this.useCaseSensitiveFileNames, this.getCurrentDirectory(), depth, path => this.getAccessibleFileSystemEntries(path), path => this.realpath(path));
return ts.matchFiles(path, extensions, exclude, include, this.useCaseSensitiveFileNames, this.getCurrentDirectory(), depth, path => this.getAccessibleFileSystemEntries(path), path => this.realpath(path), path => this.directoryExists(path));
}

public getAccessibleFileSystemEntries(path: string): ts.FileSystemEntries {
Expand Down
2 changes: 1 addition & 1 deletion src/harness/virtualFileSystemWithWatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,7 @@ interface Array<T> { length: number; [n: number]: T; }`
});
}
return { directories, files };
}, path => this.realpath(path));
}, path => this.realpath(path), path => this.directoryExists(path));
}

createHash(s: string): string {
Expand Down