From 0c2ed4c3e5a2d7ef67f893e29bebab7e17b12007 Mon Sep 17 00:00:00 2001 From: Pete Bacon Darwin Date: Fri, 17 Apr 2020 14:19:31 +0100 Subject: [PATCH] fix(ngcc): do not use cached file-system (#36687) The cached file-system was implemented to speed up ngcc processing, but in reality most files are not accessed many times and there is no noticeable degradation in speed by removing it. Benchmarking `ngcc -l debug` for AIO on a local machine gave a range of 196-236 seconds with the cache and 197-224 seconds without the cache. Moreover, when running in parallel mode, ngcc has a separate file cache for each process. This results in excess memory usage. Notably the master process, which only does analysis of entry-points holds on to up to 500Mb for AIO when using the cache compared to only around 30Mb when not using the cache. Finally, the file-system cache being incorrectly primed with file contents before being processed has been the cause of a number of bugs. For example https://github.com/angular/angular-cli/issues/16860#issuecomment-614694269. PR Close #36687 --- integration/ngcc/test.sh | 3 +-- packages/compiler-cli/ngcc/index.ts | 5 ++--- packages/compiler-cli/ngcc/main-ngcc.ts | 2 -- .../compiler-cli/ngcc/src/command_line_options.ts | 4 ++-- .../ngcc/src/execution/cluster/worker.ts | 3 --- .../locking/lock_file_with_child_process/index.ts | 10 ++-------- .../lockfile_with_child_process/index_spec.ts | 14 +------------- 7 files changed, 8 insertions(+), 33 deletions(-) diff --git a/integration/ngcc/test.sh b/integration/ngcc/test.sh index d4729a2dc3..b750c55aaa 100755 --- a/integration/ngcc/test.sh +++ b/integration/ngcc/test.sh @@ -173,8 +173,7 @@ assertSucceeded "Expected 'ngcc' to log 'Compiling'." assertEquals 1 `cat node_modules/@angular/material/button/button.d.ts | grep 'import \* as ɵngcc0' | wc -l` # Re-compile packages (which requires cleaning up those compiled by a different ngcc version). - # (Use sync mode to ensure all tasks share the same `CachedFileSystem` instance.) - ngcc --no-async --properties main + ngcc --properties main assertSucceeded "Expected 'ngcc' to successfully re-compile the packages." # Ensure previously compiled packages were correctly cleaned up (i.e. no multiple diff --git a/packages/compiler-cli/ngcc/index.ts b/packages/compiler-cli/ngcc/index.ts index 693b16438d..c508021afc 100644 --- a/packages/compiler-cli/ngcc/index.ts +++ b/packages/compiler-cli/ngcc/index.ts @@ -5,7 +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 */ -import {CachedFileSystem, NodeJSFileSystem, setFileSystem} from '../src/ngtsc/file_system'; +import {NodeJSFileSystem, setFileSystem} from '../src/ngtsc/file_system'; import {mainNgcc} from './src/main'; import {AsyncNgccOptions, NgccOptions, SyncNgccOptions} from './src/ngcc_options'; @@ -17,7 +17,6 @@ export {AsyncNgccOptions, NgccOptions, PathMappings, SyncNgccOptions} from './sr export function process(options: AsyncNgccOptions): Promise; export function process(options: SyncNgccOptions): void; export function process(options: NgccOptions): void|Promise { - // Recreate the file system on each call to reset the cache - setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); + setFileSystem(new NodeJSFileSystem()); return mainNgcc(options); } diff --git a/packages/compiler-cli/ngcc/main-ngcc.ts b/packages/compiler-cli/ngcc/main-ngcc.ts index e2ba2091bb..2afc9481ab 100644 --- a/packages/compiler-cli/ngcc/main-ngcc.ts +++ b/packages/compiler-cli/ngcc/main-ngcc.ts @@ -6,7 +6,6 @@ * 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 {setFileSystem, CachedFileSystem, NodeJSFileSystem} from '../src/ngtsc/file_system'; import {mainNgcc} from './src/main'; import {parseCommandLineOptions} from './src/command_line_options'; @@ -14,7 +13,6 @@ import {parseCommandLineOptions} from './src/command_line_options'; if (require.main === module) { process.title = 'ngcc'; const startTime = Date.now(); - setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); const options = parseCommandLineOptions(process.argv.slice(2)); (async () => { try { diff --git a/packages/compiler-cli/ngcc/src/command_line_options.ts b/packages/compiler-cli/ngcc/src/command_line_options.ts index 826538584a..65ca7fac61 100644 --- a/packages/compiler-cli/ngcc/src/command_line_options.ts +++ b/packages/compiler-cli/ngcc/src/command_line_options.ts @@ -8,7 +8,7 @@ */ import * as yargs from 'yargs'; -import {resolve, setFileSystem, CachedFileSystem, NodeJSFileSystem} from '../../src/ngtsc/file_system'; +import {resolve, setFileSystem, NodeJSFileSystem} from '../../src/ngtsc/file_system'; import {ConsoleLogger} from './logging/console_logger'; import {LogLevel} from './logging/logger'; import {NgccOptions} from './ngcc_options'; @@ -105,7 +105,7 @@ export function parseCommandLineOptions(args: string[]): NgccOptions { process.exit(1); } - setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); + setFileSystem(new NodeJSFileSystem()); const baseSourcePath = resolve(options['s'] || './node_modules'); const propertiesToConsider: string[] = options['p']; diff --git a/packages/compiler-cli/ngcc/src/execution/cluster/worker.ts b/packages/compiler-cli/ngcc/src/execution/cluster/worker.ts index a570494345..ca0e57aec5 100644 --- a/packages/compiler-cli/ngcc/src/execution/cluster/worker.ts +++ b/packages/compiler-cli/ngcc/src/execution/cluster/worker.ts @@ -9,7 +9,6 @@ import * as cluster from 'cluster'; -import {CachedFileSystem, NodeJSFileSystem, setFileSystem} from '../../../../src/ngtsc/file_system'; import {parseCommandLineOptions} from '../../command_line_options'; import {ConsoleLogger} from '../../logging/console_logger'; import {Logger, LogLevel} from '../../logging/logger'; @@ -28,8 +27,6 @@ if (require.main === module) { process.title = 'ngcc (worker)'; try { - setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); - const { createNewEntryPointFormats = false, logger = new ConsoleLogger(LogLevel.info), diff --git a/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts index e85ffca92b..ba2aaf487a 100644 --- a/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts +++ b/packages/compiler-cli/ngcc/src/locking/lock_file_with_child_process/index.ts @@ -5,10 +5,9 @@ * 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 {ChildProcess, ChildProcessByStdio, fork} from 'child_process'; -import {Readable, Writable} from 'stream'; +import {ChildProcess, fork} from 'child_process'; -import {AbsoluteFsPath, CachedFileSystem, FileSystem} from '../../../../src/ngtsc/file_system'; +import {AbsoluteFsPath, FileSystem} from '../../../../src/ngtsc/file_system'; import {Logger, LogLevel} from '../../logging/logger'; import {getLockFilePath, LockFile} from '../lock_file'; @@ -58,11 +57,6 @@ export class LockFileWithChildProcess implements LockFile { read(): string { try { - if (this.fs instanceof CachedFileSystem) { - // The lock-file file is "volatile", it might be changed by an external process, - // so we must not rely upon the cached value when reading it. - this.fs.invalidateCaches(this.path); - } return this.fs.readFile(this.path); } catch { return '{unknown}'; diff --git a/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts b/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts index 049660ba94..1f1c85720b 100644 --- a/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts +++ b/packages/compiler-cli/ngcc/test/locking/lockfile_with_child_process/index_spec.ts @@ -8,7 +8,7 @@ import {ChildProcess} from 'child_process'; import * as process from 'process'; -import {CachedFileSystem, FileSystem, getFileSystem} from '../../../../src/ngtsc/file_system'; +import {FileSystem, getFileSystem} from '../../../../src/ngtsc/file_system'; import {runInEachFileSystem} from '../../../../src/ngtsc/file_system/testing'; import {getLockFilePath} from '../../../src/locking/lock_file'; import {LockFileWithChildProcess} from '../../../src/locking/lock_file_with_child_process'; @@ -97,18 +97,6 @@ runInEachFileSystem(() => { const lockFile = new LockFileUnderTest(fs); expect(lockFile.read()).toEqual('{unknown}'); }); - - it('should not read file from the cache, since the file may have been modified externally', - () => { - const rawFs = getFileSystem(); - const fs = new CachedFileSystem(rawFs); - const lockFile = new LockFileUnderTest(fs); - rawFs.writeFile(lockFile.path, '' + process.pid); - expect(lockFile.read()).toEqual('' + process.pid); - // We need to write to the rawFs to ensure that we don't update the cache at this point - rawFs.writeFile(lockFile.path, '444'); - expect(lockFile.read()).toEqual('444'); - }); }); describe('remove()', () => {