From 7f2330a968d69291579edff84ca163b21a79e0cd Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Wed, 15 May 2019 13:38:19 +0100 Subject: [PATCH] perf(ivy): ngcc - add a cache to the FileSystem (#30525) When profiling ngcc it is notable that a large amount of time is spent dealing with an exception that is thrown (and handled internally by fs) when checking the existence of a file. We check file existence a lot in both finding entry-points and when TS is compiling code. This commit adds a simple cached `FileSystem`, which wraps a real `FileSystem` delegate. This will reduce the number of calls through to `fs.exists()` and `fs.readFile()` on the delegate. Initial benchmarks indicate that the cache is miss to hit ratio for `exists()` is about 2:1, which means that we save about 1/3 of the calls to `fs.existsSync()`. Note that this implements a "non-expiring" cache, so it is not suitable for a long lived `FileSystem`, where files may be modified externally. The cache will be updated if a file is changed or moved via calls to `FileSystem` methods but it will not be aware of changes to the files system from outside the `FileSystem` service. For ngcc we must create a new `FileSystem` service for each run of `mainNgcc` and ensure that all file operations (including TS compilation) use the `FileSystem` service. This ensures that it is very unlikely that a file will change externally during `mainNgcc` processing. PR Close #30525 --- packages/compiler-cli/ngcc/index.ts | 15 +- packages/compiler-cli/ngcc/main-ngcc.ts | 4 +- .../src/ngtsc/file_system/index.ts | 1 + .../file_system/src/cached_file_system.ts | 125 ++++++++ .../test/cached_file_system_spec.ts | 271 ++++++++++++++++++ 5 files changed, 409 insertions(+), 7 deletions(-) create mode 100644 packages/compiler-cli/src/ngtsc/file_system/src/cached_file_system.ts create mode 100644 packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts diff --git a/packages/compiler-cli/ngcc/index.ts b/packages/compiler-cli/ngcc/index.ts index cbdb7db690..b5fb624b18 100644 --- a/packages/compiler-cli/ngcc/index.ts +++ b/packages/compiler-cli/ngcc/index.ts @@ -5,20 +5,25 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +import {CachedFileSystem, NodeJSFileSystem, setFileSystem} from '../src/ngtsc/file_system'; -import {NodeJSFileSystem, setFileSystem} from '../src/ngtsc/file_system'; +import {mainNgcc} from './src/main'; import {hasBeenProcessed as _hasBeenProcessed} from './src/packages/build_marker'; import {EntryPointJsonProperty, EntryPointPackageJson} from './src/packages/entry_point'; export {ConsoleLogger, LogLevel} from './src/logging/console_logger'; export {Logger} from './src/logging/logger'; -export {NgccOptions, mainNgcc as process} from './src/main'; +export {NgccOptions} from './src/main'; export {PathMappings} from './src/utils'; export function hasBeenProcessed(packageJson: object, format: string) { - // We are wrapping this function to hide the internal types. + // Recreate the file system on each call to reset the cache + setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); return _hasBeenProcessed(packageJson as EntryPointPackageJson, format as EntryPointJsonProperty); } -// Configure the file-system for external users. -setFileSystem(new NodeJSFileSystem()); +export function process(...args: Parameters) { + // Recreate the file system on each call to reset the cache + setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); + return mainNgcc(...args); +} diff --git a/packages/compiler-cli/ngcc/main-ngcc.ts b/packages/compiler-cli/ngcc/main-ngcc.ts index 140a91746c..665200a6a5 100644 --- a/packages/compiler-cli/ngcc/main-ngcc.ts +++ b/packages/compiler-cli/ngcc/main-ngcc.ts @@ -8,7 +8,7 @@ */ import * as yargs from 'yargs'; -import {resolve, setFileSystem, NodeJSFileSystem} from '../src/ngtsc/file_system'; +import {resolve, setFileSystem, CachedFileSystem, NodeJSFileSystem} from '../src/ngtsc/file_system'; import {mainNgcc} from './src/main'; import {ConsoleLogger, LogLevel} from './src/logging/console_logger'; @@ -57,7 +57,7 @@ if (require.main === module) { process.exit(1); } - setFileSystem(new NodeJSFileSystem()); + setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); const baseSourcePath = resolve(options['s'] || './node_modules'); const propertiesToConsider: string[] = options['p']; diff --git a/packages/compiler-cli/src/ngtsc/file_system/index.ts b/packages/compiler-cli/src/ngtsc/file_system/index.ts index e0cd876adb..594af21923 100644 --- a/packages/compiler-cli/src/ngtsc/file_system/index.ts +++ b/packages/compiler-cli/src/ngtsc/file_system/index.ts @@ -5,6 +5,7 @@ * Use of this source code is governed by an MIT-style license that can be * found in the LICENSE file at https://angular.io/license */ +export {CachedFileSystem} from './src/cached_file_system'; export {NgtscCompilerHost} from './src/compiler_host'; export {absoluteFrom, absoluteFromSourceFile, basename, dirname, getFileSystem, isRoot, join, relative, relativeFrom, resolve, setFileSystem} from './src/helpers'; export {LogicalFileSystem, LogicalProjectPath} from './src/logical'; diff --git a/packages/compiler-cli/src/ngtsc/file_system/src/cached_file_system.ts b/packages/compiler-cli/src/ngtsc/file_system/src/cached_file_system.ts new file mode 100644 index 0000000000..a141dc5550 --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/file_system/src/cached_file_system.ts @@ -0,0 +1,125 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {AbsoluteFsPath, FileStats, FileSystem, PathSegment, PathString} from './types'; + + +/** + * A wrapper around `FileSystem` that caches hits to `exists()` and + * `readFile()` to improve performance. + * + * Be aware that any changes to the file system from outside of this + * class could break the cache, leaving it with stale values. + */ +export class CachedFileSystem implements FileSystem { + private existsCache = new Map(); + private readFileCache = new Map(); + + constructor(private delegate: FileSystem) {} + + exists(path: AbsoluteFsPath): boolean { + if (!this.existsCache.has(path)) { + this.existsCache.set(path, this.delegate.exists(path)); + } + return this.existsCache.get(path) !; + } + + readFile(path: AbsoluteFsPath): string { + if (!this.readFileCache.has(path)) { + try { + if (this.lstat(path).isSymbolicLink()) { + // don't cache the value of a symbolic link + return this.delegate.readFile(path); + } + this.readFileCache.set(path, this.delegate.readFile(path)); + } catch (e) { + this.readFileCache.set(path, e); + } + } + const result = this.readFileCache.get(path); + if (typeof result === 'string') { + return result; + } else { + throw result; + } + } + + writeFile(path: AbsoluteFsPath, data: string): void { + this.delegate.writeFile(path, data); + this.readFileCache.set(path, data); + this.existsCache.set(path, true); + } + + symlink(target: AbsoluteFsPath, path: AbsoluteFsPath): void { + this.delegate.symlink(target, path); + this.existsCache.set(path, true); + } + + copyFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { + this.delegate.copyFile(from, to); + this.existsCache.set(to, true); + } + + moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void { + this.delegate.moveFile(from, to); + this.existsCache.set(from, false); + if (this.readFileCache.has(from)) { + this.readFileCache.set(to, this.readFileCache.get(from)); + this.readFileCache.delete(from); + } + this.existsCache.set(to, true); + } + + mkdir(path: AbsoluteFsPath): void { + this.delegate.mkdir(path); + this.existsCache.set(path, true); + } + + ensureDir(path: AbsoluteFsPath): void { + this.delegate.ensureDir(path); + while (!this.isRoot(path)) { + this.existsCache.set(path, true); + path = this.dirname(path); + } + } + + lstat(path: AbsoluteFsPath): FileStats { + const stat = this.delegate.lstat(path); + // if the `path` does not exist then `lstat` will thrown an error. + this.existsCache.set(path, true); + return stat; + } + + stat(path: AbsoluteFsPath): FileStats { + const stat = this.delegate.stat(path); + // if the `path` does not exist then `stat` will thrown an error. + this.existsCache.set(path, true); + return stat; + } + + // The following methods simply call through to the delegate. + readdir(path: AbsoluteFsPath): PathSegment[] { return this.delegate.readdir(path); } + pwd(): AbsoluteFsPath { return this.delegate.pwd(); } + extname(path: AbsoluteFsPath|PathSegment): string { return this.delegate.extname(path); } + isCaseSensitive(): boolean { return this.delegate.isCaseSensitive(); } + isRoot(path: AbsoluteFsPath): boolean { return this.delegate.isRoot(path); } + isRooted(path: string): boolean { return this.delegate.isRooted(path); } + resolve(...paths: string[]): AbsoluteFsPath { return this.delegate.resolve(...paths); } + dirname(file: T): T { return this.delegate.dirname(file); } + join(basePath: T, ...paths: string[]): T { + return this.delegate.join(basePath, ...paths); + } + relative(from: T, to: T): PathSegment { + return this.delegate.relative(from, to); + } + basename(filePath: string, extension?: string|undefined): PathSegment { + return this.delegate.basename(filePath, extension); + } + realpath(filePath: AbsoluteFsPath): AbsoluteFsPath { return this.delegate.realpath(filePath); } + getDefaultLibLocation(): AbsoluteFsPath { return this.delegate.getDefaultLibLocation(); } + normalize(path: T): T { return this.delegate.normalize(path); } +} diff --git a/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts b/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts new file mode 100644 index 0000000000..9bf2a7ccfa --- /dev/null +++ b/packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts @@ -0,0 +1,271 @@ +/** + * @license + * Copyright Google Inc. All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ +import {CachedFileSystem} from '../src/cached_file_system'; +import {absoluteFrom, setFileSystem} from '../src/helpers'; +import {NodeJSFileSystem} from '../src/node_js_file_system'; +import {AbsoluteFsPath, FileSystem} from '../src/types'; + +describe('CachedFileSystem', () => { + let delegate: FileSystem; + let fs: CachedFileSystem; + let abcPath: AbsoluteFsPath; + let xyzPath: AbsoluteFsPath; + + beforeEach(() => { + delegate = new NodeJSFileSystem(); + fs = new CachedFileSystem(delegate); + // Set the file-system so that calls like `absoluteFrom()` + // and `PathSegment.fromFsPath()` work correctly. + setFileSystem(fs); + abcPath = absoluteFrom('/a/b/c'); + xyzPath = absoluteFrom('/x/y/z'); + }); + + describe('exists()', () => { + it('should call delegate if not in cache', () => { + const spy = spyOn(delegate, 'exists').and.returnValue(true); + expect(fs.exists(abcPath)).toBe(true); + expect(spy).toHaveBeenCalledWith(abcPath); + }); + + it('should take from the cache if available', () => { + const spy = spyOn(delegate, 'exists').and.returnValue(true); + fs.exists(abcPath); // Call once to fill the cache + spy.calls.reset(); + + expect(fs.exists(abcPath)).toBe(true); + expect(spy).not.toHaveBeenCalled(); + }); + }); + + describe('readFile()', () => { + let lstatSpy: jasmine.Spy; + beforeEach(() => { + // For most of the tests the files are not symbolic links. + lstatSpy = spyOn(delegate, 'lstat').and.returnValue({isSymbolicLink: () => false}); + }); + + it('should call delegate if not in cache', () => { + const spy = spyOn(delegate, 'readFile').and.returnValue('Some contents'); + expect(fs.readFile(abcPath)).toBe('Some contents'); + expect(spy).toHaveBeenCalledWith(abcPath); + }); + + it('should take from the cache if available', () => { + const spy = spyOn(delegate, 'readFile').and.returnValue('Some contents'); + fs.readFile(abcPath); // Call once to fill the cache + spy.calls.reset(); + + expect(fs.readFile(abcPath)).toBe('Some contents'); + expect(spy).not.toHaveBeenCalled(); + }); + + it('should cache the exception if it originally threw', () => { + const spy = spyOn(delegate, 'readFile').and.throwError('Some error'); + expect(() => fs.readFile(abcPath)).toThrowError('Some error'); + spy.calls.reset(); + expect(() => fs.readFile(abcPath)).toThrowError('Some error'); + expect(spy).not.toHaveBeenCalled(); + }); + + it('should always call delegate (and not cache) if the path is a symbolic link', () => { + const readFileSpy = spyOn(delegate, 'readFile').and.returnValue('Some contents'); + // Simulate a symlink by overriding `lstat` + lstatSpy.and.returnValue({isSymbolicLink: () => true}); + + // Read the symlink target file contents + expect(fs.readFile(abcPath)).toEqual('Some contents'); + expect(lstatSpy).toHaveBeenCalledWith(abcPath); + + // Now read it again and check that the cache was not hit + lstatSpy.calls.reset(); + readFileSpy.calls.reset(); + expect(fs.readFile(abcPath)).toEqual('Some contents'); + expect(lstatSpy).toHaveBeenCalledWith(abcPath); + }); + }); + + describe('writeFile()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'writeFile'); + fs.writeFile(abcPath, 'Some contents'); + expect(spy).toHaveBeenCalledWith(abcPath, 'Some contents'); + }); + + it('should update the exists and "readFile" caches', () => { + spyOn(delegate, 'writeFile'); + const existsSpy = spyOn(delegate, 'exists'); + const readFileSpy = spyOn(delegate, 'readFile'); + + fs.writeFile(abcPath, 'Some contents'); + expect(fs.readFile(abcPath)).toEqual('Some contents'); + expect(fs.exists(abcPath)).toBe(true); + expect(existsSpy).not.toHaveBeenCalled(); + expect(readFileSpy).not.toHaveBeenCalled(); + }); + }); + + describe('readdir()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'readdir'); + fs.readdir(abcPath); + expect(spy).toHaveBeenCalledWith(abcPath); + }); + }); + + describe('lstat()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'lstat'); + fs.lstat(abcPath); + expect(spy).toHaveBeenCalledWith(abcPath); + }); + + it('should update the "exists" cache', () => { + spyOn(delegate, 'lstat'); + const existsSpy = spyOn(delegate, 'exists'); + fs.lstat(abcPath); + expect(fs.exists(abcPath)).toBe(true); + expect(existsSpy).not.toHaveBeenCalled(); + }); + }); + + describe('stat()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'stat'); + fs.stat(abcPath); + expect(spy).toHaveBeenCalledWith(abcPath); + }); + + it('should update the "exists" cache', () => { + spyOn(delegate, 'stat'); + const existsSpy = spyOn(delegate, 'exists'); + fs.stat(abcPath); + expect(fs.exists(abcPath)).toBe(true); + expect(existsSpy).not.toHaveBeenCalled(); + }); + }); + + describe('pwd()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'pwd'); + fs.pwd(); + expect(spy).toHaveBeenCalledWith(); + }); + }); + + describe('copyFile()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'copyFile'); + fs.copyFile(abcPath, xyzPath); + expect(spy).toHaveBeenCalledWith(abcPath, xyzPath); + }); + + it('should update the "exists" cache', () => { + spyOn(delegate, 'copyFile'); + const existsSpy = spyOn(delegate, 'exists').and.returnValue(false); + fs.copyFile(abcPath, xyzPath); + expect(fs.exists(xyzPath)).toEqual(true); + expect(existsSpy).not.toHaveBeenCalled(); + }); + }); + + describe('moveFile()', () => { + beforeEach(() => { + // `moveFile()` relies upon `readFile` which calls through to `lstat()`, so stub it out. + spyOn(delegate, 'lstat').and.returnValue({isSymbolicLink: () => false}); + }); + + it('should call delegate', () => { + const spy = spyOn(delegate, 'moveFile'); + fs.moveFile(abcPath, xyzPath); + expect(spy).toHaveBeenCalledWith(abcPath, xyzPath); + }); + + it('should update the "exists" cache', () => { + spyOn(delegate, 'moveFile'); + const existsSpy = spyOn(delegate, 'exists'); + + fs.moveFile(abcPath, xyzPath); + + expect(fs.exists(abcPath)).toEqual(false); + expect(fs.exists(xyzPath)).toEqual(true); + expect(existsSpy).not.toHaveBeenCalled(); + }); + + it('should delete the `from` "readFile" cache', () => { + spyOn(delegate, 'moveFile'); + const readFileSpy = spyOn(delegate, 'readFile'); + + // Fill the abc "readFile" cache + readFileSpy.and.returnValue('abc content'); + fs.readFile(abcPath); + + // Move the file + fs.moveFile(abcPath, xyzPath); + + // Emulate an error now that the file has been moved. + readFileSpy.and.throwError('no file'); + + // Show that asking for the abc file does not read from the cache + expect(() => fs.readFile(abcPath)).toThrowError('no file'); + expect(readFileSpy).toHaveBeenCalledWith(abcPath); + }); + + it('should update the `to` "readFile" cache', () => { + spyOn(delegate, 'moveFile'); + const readFileSpy = spyOn(delegate, 'readFile'); + + // Fill the abc "readFile" cache + readFileSpy.and.returnValue('abc content'); + fs.readFile(abcPath); + readFileSpy.calls.reset(); + + // Move the file + fs.moveFile(abcPath, xyzPath); + + // Show that the cache was hit for the xyz file + expect(fs.readFile(xyzPath)).toEqual('abc content'); + expect(readFileSpy).not.toHaveBeenCalled(); + }); + }); + + describe('mkdir()', () => { + it('should call delegate', () => { + const spy = spyOn(delegate, 'mkdir'); + fs.mkdir(xyzPath); + expect(spy).toHaveBeenCalledWith(xyzPath); + }); + + it('should update the "exists" cache', () => { + spyOn(delegate, 'mkdir'); + const existsSpy = spyOn(delegate, 'exists'); + fs.mkdir(xyzPath); + expect(fs.exists(xyzPath)).toEqual(true); + expect(existsSpy).not.toHaveBeenCalled(); + }); + }); + + describe('ensureDir()', () => { + it('should call delegate', () => { + const ensureDirSpy = spyOn(delegate, 'ensureDir'); + fs.ensureDir(abcPath); + expect(ensureDirSpy).toHaveBeenCalledWith(abcPath); + }); + + it('should update the "exists" cache', () => { + spyOn(delegate, 'ensureDir'); + const existsSpy = spyOn(delegate, 'exists'); + fs.ensureDir(abcPath); + existsSpy.calls.reset(); + expect(fs.exists(abcPath)).toEqual(true); + expect(fs.exists(absoluteFrom('/a/b'))).toEqual(true); + expect(fs.exists(absoluteFrom('/a'))).toEqual(true); + expect(existsSpy).not.toHaveBeenCalled(); + }); + }); +});