From 7f3fb9d58d4e268cff054a5d0bccb132ffcd0210 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Fri, 10 Jan 2025 13:42:58 +0100 Subject: [PATCH] watcher - more perf improvements for large workspaces --- src/vs/base/node/extpath.ts | 44 +--------- src/vs/base/test/node/extpath.test.ts | 26 +----- .../node/watcher/parcel/parcelWatcher.ts | 57 ++++++------ .../files/test/node/nodejsWatcher.test.ts | 6 +- .../files/test/node/parcelWatcher.test.ts | 86 ++++++++++--------- 5 files changed, 85 insertions(+), 134 deletions(-) diff --git a/src/vs/base/node/extpath.ts b/src/vs/base/node/extpath.ts index 4fa8d7a0b2b11..60fbdd6e4c7c4 100644 --- a/src/vs/base/node/extpath.ts +++ b/src/vs/base/node/extpath.ts @@ -8,7 +8,7 @@ import { CancellationToken } from '../common/cancellation.js'; import { basename, dirname, join, normalize, sep } from '../common/path.js'; import { isLinux } from '../common/platform.js'; import { rtrim } from '../common/strings.js'; -import { Promises, readdirSync } from './pfs.js'; +import { Promises } from './pfs.js'; /** * Copied from: https://github.com/microsoft/vscode-node-debug/blob/master/src/node/pathUtilities.ts#L83 @@ -17,48 +17,8 @@ import { Promises, readdirSync } from './pfs.js'; * On a case insensitive file system, the returned path might differ from the original path by character casing. * On a case sensitive file system, the returned path will always be identical to the original path. * In case of errors, null is returned. But you cannot use this function to verify that a path exists. - * realcaseSync does not handle '..' or '.' path segments and it does not take the locale into account. + * realcase does not handle '..' or '.' path segments and it does not take the locale into account. */ -export function realcaseSync(path: string): string | null { - if (isLinux) { - // This method is unsupported on OS that have case sensitive - // file system where the same path can exist in different forms - // (see also https://github.com/microsoft/vscode/issues/139709) - return path; - } - - const dir = dirname(path); - if (path === dir) { // end recursion - return path; - } - - const name = (basename(path) /* can be '' for windows drive letters */ || path).toLowerCase(); - try { - const entries = readdirSync(dir); - const found = entries.filter(e => e.toLowerCase() === name); // use a case insensitive search - if (found.length === 1) { - // on a case sensitive filesystem we cannot determine here, whether the file exists or not, hence we need the 'file exists' precondition - const prefix = realcaseSync(dir); // recurse - if (prefix) { - return join(prefix, found[0]); - } - } else if (found.length > 1) { - // must be a case sensitive $filesystem - const ix = found.indexOf(name); - if (ix >= 0) { // case sensitive - const prefix = realcaseSync(dir); // recurse - if (prefix) { - return join(prefix, found[ix]); - } - } - } - } catch (error) { - // silently ignore error - } - - return null; -} - export async function realcase(path: string, token?: CancellationToken): Promise { if (isLinux) { // This method is unsupported on OS that have case sensitive diff --git a/src/vs/base/test/node/extpath.test.ts b/src/vs/base/test/node/extpath.test.ts index 9dbdf1ac87e3b..c86f3cea0d1e2 100644 --- a/src/vs/base/test/node/extpath.test.ts +++ b/src/vs/base/test/node/extpath.test.ts @@ -6,7 +6,7 @@ import * as fs from 'fs'; import assert from 'assert'; import { tmpdir } from 'os'; -import { realcase, realcaseSync, realpath, realpathSync } from '../../node/extpath.js'; +import { realcase, realpath, realpathSync } from '../../node/extpath.js'; import { Promises } from '../../node/pfs.js'; import { ensureNoDisposablesAreLeakedInTestSuite } from '../common/utils.js'; import { flakySuite, getRandomTestPath } from './testUtils.js'; @@ -24,30 +24,6 @@ flakySuite('Extpath', () => { return Promises.rm(testDir); }); - test('realcaseSync', async () => { - - // assume case insensitive file system - if (process.platform === 'win32' || process.platform === 'darwin') { - const upper = testDir.toUpperCase(); - const real = realcaseSync(upper); - - if (real) { // can be null in case of permission errors - assert.notStrictEqual(real, upper); - assert.strictEqual(real.toUpperCase(), upper); - assert.strictEqual(real, testDir); - } - } - - // linux, unix, etc. -> assume case sensitive file system - else { - let real = realcaseSync(testDir); - assert.strictEqual(real, testDir); - - real = realcaseSync(testDir.toUpperCase()); - assert.strictEqual(real, testDir.toUpperCase()); - } - }); - test('realcase', async () => { // assume case insensitive file system diff --git a/src/vs/platform/files/node/watcher/parcel/parcelWatcher.ts b/src/vs/platform/files/node/watcher/parcel/parcelWatcher.ts index f6eea7fb0e527..c778b998140b3 100644 --- a/src/vs/platform/files/node/watcher/parcel/parcelWatcher.ts +++ b/src/vs/platform/files/node/watcher/parcel/parcelWatcher.ts @@ -4,7 +4,7 @@ *--------------------------------------------------------------------------------------------*/ import parcelWatcher from '@parcel/watcher'; -import { statSync, unlinkSync } from 'fs'; +import { promises } from 'fs'; import { tmpdir, homedir } from 'os'; import { URI } from '../../../../../base/common/uri.js'; import { DeferredPromise, RunOnceScheduler, RunOnceWorker, ThrottledWorker } from '../../../../../base/common/async.js'; @@ -18,7 +18,7 @@ import { TernarySearchTree } from '../../../../../base/common/ternarySearchTree. import { normalizeNFC } from '../../../../../base/common/normalization.js'; import { normalize, join } from '../../../../../base/common/path.js'; import { isLinux, isMacintosh, isWindows } from '../../../../../base/common/platform.js'; -import { realcaseSync, realpathSync } from '../../../../../base/node/extpath.js'; +import { realcase, realpath } from '../../../../../base/node/extpath.js'; import { FileChangeType, IFileChange } from '../../../common/files.js'; import { coalesceEvents, IRecursiveWatchRequest, parseWatcherPatterns, IRecursiveWatcherWithSubscribe, isFiltered, IWatcherErrorEvent } from '../../../common/watcher.js'; import { Disposable, DisposableStore, IDisposable, toDisposable } from '../../../../../base/common/lifecycle.js'; @@ -201,7 +201,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS protected override async doWatch(requests: IRecursiveWatchRequest[]): Promise { // Figure out duplicates to remove from the requests - requests = this.removeDuplicateRequests(requests); + requests = await this.removeDuplicateRequests(requests); // Figure out which watchers to start and which to stop const requestsToStart: IRecursiveWatchRequest[] = []; @@ -232,7 +232,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS // Start watching as instructed for (const request of requestsToStart) { if (request.pollingInterval) { - this.startPolling(request, request.pollingInterval); + await this.startPolling(request, request.pollingInterval); } else { await this.startWatching(request); } @@ -247,7 +247,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS return isLinux ? path : path.toLowerCase() /* ignore path casing */; } - private startPolling(request: IRecursiveWatchRequest, pollingInterval: number, restarts = 0): void { + private async startPolling(request: IRecursiveWatchRequest, pollingInterval: number, restarts = 0): Promise { const cts = new CancellationTokenSource(); const instance = new DeferredPromise(); @@ -268,13 +268,13 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS watcher.worker.dispose(); pollingWatcher.dispose(); - unlinkSync(snapshotFile); + await promises.unlink(snapshotFile); } ); this._watchers.set(this.requestToWatcherKey(request), watcher); // Path checks for symbolic links / wrong casing - const { realPath, realPathDiffers, realPathLength } = this.normalizePath(request); + const { realPath, realPathDiffers, realPathLength } = await this.normalizePath(request); this.trace(`Started watching: '${realPath}' with polling interval '${pollingInterval}'`); @@ -343,7 +343,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS this._watchers.set(this.requestToWatcherKey(request), watcher); // Path checks for symbolic links / wrong casing - const { realPath, realPathDiffers, realPathLength } = this.normalizePath(request); + const { realPath, realPathDiffers, realPathLength } = await this.normalizePath(request); try { const parcelWatcherLib = parcelWatcher; @@ -471,7 +471,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS } } - private normalizePath(request: IRecursiveWatchRequest): { realPath: string; realPathDiffers: boolean; realPathLength: number } { + private async normalizePath(request: IRecursiveWatchRequest): Promise<{ realPath: string; realPathDiffers: boolean; realPathLength: number }> { let realPath = request.path; let realPathDiffers = false; let realPathLength = request.path.length; @@ -479,12 +479,12 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS try { // First check for symbolic link - realPath = realpathSync(request.path); + realPath = await realpath(request.path); // Second check for casing difference // Note: this will be a no-op on Linux platforms if (request.path === realPath) { - realPath = realcaseSync(request.path) ?? request.path; + realPath = await realcase(request.path) ?? request.path; } // Correct watch path as needed @@ -615,7 +615,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS // Start watcher again counting the restarts if (watcher.request.pollingInterval) { - this.startPolling(watcher.request, watcher.request.pollingInterval, watcher.restarts + 1); + await this.startPolling(watcher.request, watcher.request.pollingInterval, watcher.restarts + 1); } else { await this.startWatching(watcher.request, watcher.restarts + 1); } @@ -640,7 +640,7 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS } } - protected removeDuplicateRequests(requests: IRecursiveWatchRequest[], validatePaths = true): IRecursiveWatchRequest[] { + protected async removeDuplicateRequests(requests: IRecursiveWatchRequest[], validatePaths = true): Promise { // Sort requests by path length to have shortest first // to have a way to prevent children to be watched if @@ -686,26 +686,29 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS for (const request of requestsForCorrelation.values()) { - // Check for overlapping requests + // Check for overlapping request paths (but preserve symbolic links) if (requestTrie.findSubstr(request.path)) { - try { - const realpath = realpathSync(request.path); - if (realpath === request.path) { - this.trace(`ignoring a request for watching who's parent is already watched: ${this.requestToString(request)}`); + if (requestTrie.has(request.path)) { + this.trace(`ignoring a request for watching who's path is already watched: ${this.requestToString(request)}`); + } else { + try { + if (!(await promises.lstat(request.path)).isSymbolicLink()) { + this.trace(`ignoring a request for watching who's parent is already watched: ${this.requestToString(request)}`); - continue; - } - } catch (error) { - this.trace(`ignoring a request for watching who's realpath failed to resolve: ${this.requestToString(request)} (error: ${error})`); + continue; + } + } catch (error) { + this.trace(`ignoring a request for watching who's lstat failed to resolve: ${this.requestToString(request)} (error: ${error})`); - this._onDidWatchFail.fire(request); + this._onDidWatchFail.fire(request); - continue; + continue; + } } } // Check for invalid paths - if (validatePaths && !this.isPathValid(request.path)) { + if (validatePaths && !(await this.isPathValid(request.path))) { this._onDidWatchFail.fire(request); continue; @@ -720,9 +723,9 @@ export class ParcelWatcher extends BaseWatcher implements IRecursiveWatcherWithS return normalizedRequests; } - private isPathValid(path: string): boolean { + private async isPathValid(path: string): Promise { try { - const stat = statSync(path); + const stat = await promises.stat(path); if (!stat.isDirectory()) { this.trace(`ignoring a path for watching that is a file and not a folder: ${path}`); diff --git a/src/vs/platform/files/test/node/nodejsWatcher.test.ts b/src/vs/platform/files/test/node/nodejsWatcher.test.ts index 9a234b80cf57a..2e748a7c53222 100644 --- a/src/vs/platform/files/test/node/nodejsWatcher.test.ts +++ b/src/vs/platform/files/test/node/nodejsWatcher.test.ts @@ -72,7 +72,11 @@ suite.skip('File Watcher (node.js)', function () { setup(async () => { await createWatcher(undefined); - testDir = URI.file(getRandomTestPath(tmpdir(), 'vsctests', 'filewatcher')).fsPath; + // Rule out strange testing conditions by using the realpath + // here. for example, on macOS the tmp dir is potentially a + // symlink in some of the root folders, which is a rather + // unrealisic case for the file watcher. + testDir = URI.file(getRandomTestPath(fs.realpathSync(tmpdir()), 'vsctests', 'filewatcher')).fsPath; const sourceDir = FileAccess.asFileUri('vs/platform/files/test/node/fixtures/service').fsPath; diff --git a/src/vs/platform/files/test/node/parcelWatcher.test.ts b/src/vs/platform/files/test/node/parcelWatcher.test.ts index 8b1824af3bd83..e2038a17f50ed 100644 --- a/src/vs/platform/files/test/node/parcelWatcher.test.ts +++ b/src/vs/platform/files/test/node/parcelWatcher.test.ts @@ -32,14 +32,14 @@ export class TestParcelWatcher extends ParcelWatcher { readonly onWatchFail = this._onDidWatchFail.event; - testRemoveDuplicateRequests(paths: string[], excludes: string[] = []): string[] { + async testRemoveDuplicateRequests(paths: string[], excludes: string[] = []): Promise { // Work with strings as paths to simplify testing const requests: IRecursiveWatchRequest[] = paths.map(path => { return { path, excludes, recursive: true }; }); - return this.removeDuplicateRequests(requests, false /* validate paths skipped for tests */).map(request => request.path); + return (await this.removeDuplicateRequests(requests, false /* validate paths skipped for tests */)).map(request => request.path); } protected override getUpdateWatchersDelay(): number { @@ -97,7 +97,11 @@ suite.skip('File Watcher (parcel)', function () { } }); - testDir = URI.file(getRandomTestPath(tmpdir(), 'vsctests', 'filewatcher')).fsPath; + // Rule out strange testing conditions by using the realpath + // here. for example, on macOS the tmp dir is potentially a + // symlink in some of the root folders, which is a rather + // unrealisic case for the file watcher. + testDir = URI.file(getRandomTestPath(realpathSync(tmpdir()), 'vsctests', 'filewatcher')).fsPath; const sourceDir = FileAccess.asFileUri('vs/platform/files/test/node/fixtures/service').fsPath; @@ -636,34 +640,34 @@ suite.skip('File Watcher (parcel)', function () { return basicCrudTest(join(testDir, 'newFile.txt'), correlationId); }); - test('should not exclude roots that do not overlap', () => { + test('should not exclude roots that do not overlap', async () => { if (isWindows) { - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\a']), ['C:\\a']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\b']), ['C:\\a', 'C:\\b']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\b', 'C:\\c\\d\\e']), ['C:\\a', 'C:\\b', 'C:\\c\\d\\e']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\a']), ['C:\\a']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\b']), ['C:\\a', 'C:\\b']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\b', 'C:\\c\\d\\e']), ['C:\\a', 'C:\\b', 'C:\\c\\d\\e']); } else { - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/a']), ['/a']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/a', '/b']), ['/a', '/b']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/a', '/b', '/c/d/e']), ['/a', '/b', '/c/d/e']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/a']), ['/a']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/a', '/b']), ['/a', '/b']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/a', '/b', '/c/d/e']), ['/a', '/b', '/c/d/e']); } }); - test('should remove sub-folders of other paths', () => { + test('should remove sub-folders of other paths', async () => { if (isWindows) { - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\a\\b']), ['C:\\a']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\b', 'C:\\a\\b']), ['C:\\a', 'C:\\b']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\b\\a', 'C:\\a', 'C:\\b', 'C:\\a\\b']), ['C:\\a', 'C:\\b']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\a\\b', 'C:\\a\\c\\d']), ['C:\\a']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\a\\b']), ['C:\\a']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\b', 'C:\\a\\b']), ['C:\\a', 'C:\\b']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\b\\a', 'C:\\a', 'C:\\b', 'C:\\a\\b']), ['C:\\a', 'C:\\b']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['C:\\a', 'C:\\a\\b', 'C:\\a\\c\\d']), ['C:\\a']); } else { - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/a', '/a/b']), ['/a']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/a', '/b', '/a/b']), ['/a', '/b']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/b/a', '/a', '/b', '/a/b']), ['/a', '/b']); - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/a', '/a/b', '/a/c/d']), ['/a']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/a', '/a/b']), ['/a']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/a', '/b', '/a/b']), ['/a', '/b']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/b/a', '/a', '/b', '/a/b']), ['/a', '/b']); + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/a', '/a/b', '/a/c/d']), ['/a']); } }); - test('should ignore when everything excluded', () => { - assert.deepStrictEqual(watcher.testRemoveDuplicateRequests(['/foo/bar', '/bar'], ['**', 'something']), []); + test('should ignore when everything excluded', async () => { + assert.deepStrictEqual(await watcher.testRemoveDuplicateRequests(['/foo/bar', '/bar'], ['**', 'something']), []); }); test('watching same or overlapping paths supported when correlation is applied', async () => { @@ -786,17 +790,19 @@ suite.skip('File Watcher (parcel)', function () { const filePath = join(folderPath, 'newFile.txt'); await basicCrudTest(filePath); - onDidWatchFail = Event.toPromise(watcher.onWatchFail); - await Promises.rm(folderPath); - await onDidWatchFail; + if (!reuseExistingWatcher) { + onDidWatchFail = Event.toPromise(watcher.onWatchFail); + await Promises.rm(folderPath); + await onDidWatchFail; - changeFuture = awaitEvent(watcher, folderPath, FileChangeType.ADDED); - onDidWatch = Event.toPromise(watcher.onDidWatch); - await promises.mkdir(folderPath); - await changeFuture; - await onDidWatch; + changeFuture = awaitEvent(watcher, folderPath, FileChangeType.ADDED); + onDidWatch = Event.toPromise(watcher.onDidWatch); + await promises.mkdir(folderPath); + await changeFuture; + await onDidWatch; - await basicCrudTest(filePath); + await basicCrudTest(filePath); + } } (isWindows /* Windows: times out for some reason */ ? test.skip : test)('watch requests support suspend/resume (folder, exist in beginning, not reusing watcher)', async () => { @@ -820,17 +826,19 @@ suite.skip('File Watcher (parcel)', function () { const filePath = join(folderPath, 'newFile.txt'); await basicCrudTest(filePath); - const onDidWatchFail = Event.toPromise(watcher.onWatchFail); - await Promises.rm(folderPath); - await onDidWatchFail; + if (!reuseExistingWatcher) { + const onDidWatchFail = Event.toPromise(watcher.onWatchFail); + await Promises.rm(folderPath); + await onDidWatchFail; - const changeFuture = awaitEvent(watcher, folderPath, FileChangeType.ADDED); - const onDidWatch = Event.toPromise(watcher.onDidWatch); - await promises.mkdir(folderPath); - await changeFuture; - await onDidWatch; + const changeFuture = awaitEvent(watcher, folderPath, FileChangeType.ADDED); + const onDidWatch = Event.toPromise(watcher.onDidWatch); + await promises.mkdir(folderPath); + await changeFuture; + await onDidWatch; - await basicCrudTest(filePath); + await basicCrudTest(filePath); + } } test('watch request reuses another recursive watcher even when requests are coming in at the same time', async function () {