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
This commit is contained in:
Pete Bacon Darwin 2020-04-17 14:19:31 +01:00 committed by Matias Niemelä
parent 37bfb14956
commit 0c2ed4c3e5
7 changed files with 8 additions and 33 deletions

View File

@ -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` 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). # 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 --properties main
ngcc --no-async --properties main
assertSucceeded "Expected 'ngcc' to successfully re-compile the packages." assertSucceeded "Expected 'ngcc' to successfully re-compile the packages."
# Ensure previously compiled packages were correctly cleaned up (i.e. no multiple # Ensure previously compiled packages were correctly cleaned up (i.e. no multiple

View File

@ -5,7 +5,7 @@
* Use of this source code is governed by an MIT-style license that can be * 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 * 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 {mainNgcc} from './src/main';
import {AsyncNgccOptions, NgccOptions, SyncNgccOptions} from './src/ngcc_options'; 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<void>; export function process(options: AsyncNgccOptions): Promise<void>;
export function process(options: SyncNgccOptions): void; export function process(options: SyncNgccOptions): void;
export function process(options: NgccOptions): void|Promise<void> { export function process(options: NgccOptions): void|Promise<void> {
// Recreate the file system on each call to reset the cache setFileSystem(new NodeJSFileSystem());
setFileSystem(new CachedFileSystem(new NodeJSFileSystem()));
return mainNgcc(options); return mainNgcc(options);
} }

View File

@ -6,7 +6,6 @@
* Use of this source code is governed by an MIT-style license that can be * 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 * 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 {mainNgcc} from './src/main';
import {parseCommandLineOptions} from './src/command_line_options'; import {parseCommandLineOptions} from './src/command_line_options';
@ -14,7 +13,6 @@ import {parseCommandLineOptions} from './src/command_line_options';
if (require.main === module) { if (require.main === module) {
process.title = 'ngcc'; process.title = 'ngcc';
const startTime = Date.now(); const startTime = Date.now();
setFileSystem(new CachedFileSystem(new NodeJSFileSystem()));
const options = parseCommandLineOptions(process.argv.slice(2)); const options = parseCommandLineOptions(process.argv.slice(2));
(async () => { (async () => {
try { try {

View File

@ -8,7 +8,7 @@
*/ */
import * as yargs from 'yargs'; 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 {ConsoleLogger} from './logging/console_logger';
import {LogLevel} from './logging/logger'; import {LogLevel} from './logging/logger';
import {NgccOptions} from './ngcc_options'; import {NgccOptions} from './ngcc_options';
@ -105,7 +105,7 @@ export function parseCommandLineOptions(args: string[]): NgccOptions {
process.exit(1); process.exit(1);
} }
setFileSystem(new CachedFileSystem(new NodeJSFileSystem())); setFileSystem(new NodeJSFileSystem());
const baseSourcePath = resolve(options['s'] || './node_modules'); const baseSourcePath = resolve(options['s'] || './node_modules');
const propertiesToConsider: string[] = options['p']; const propertiesToConsider: string[] = options['p'];

View File

@ -9,7 +9,6 @@
import * as cluster from 'cluster'; import * as cluster from 'cluster';
import {CachedFileSystem, NodeJSFileSystem, setFileSystem} from '../../../../src/ngtsc/file_system';
import {parseCommandLineOptions} from '../../command_line_options'; import {parseCommandLineOptions} from '../../command_line_options';
import {ConsoleLogger} from '../../logging/console_logger'; import {ConsoleLogger} from '../../logging/console_logger';
import {Logger, LogLevel} from '../../logging/logger'; import {Logger, LogLevel} from '../../logging/logger';
@ -28,8 +27,6 @@ if (require.main === module) {
process.title = 'ngcc (worker)'; process.title = 'ngcc (worker)';
try { try {
setFileSystem(new CachedFileSystem(new NodeJSFileSystem()));
const { const {
createNewEntryPointFormats = false, createNewEntryPointFormats = false,
logger = new ConsoleLogger(LogLevel.info), logger = new ConsoleLogger(LogLevel.info),

View File

@ -5,10 +5,9 @@
* Use of this source code is governed by an MIT-style license that can be * 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 * found in the LICENSE file at https://angular.io/license
*/ */
import {ChildProcess, ChildProcessByStdio, fork} from 'child_process'; import {ChildProcess, fork} from 'child_process';
import {Readable, Writable} from 'stream';
import {AbsoluteFsPath, CachedFileSystem, FileSystem} from '../../../../src/ngtsc/file_system'; import {AbsoluteFsPath, FileSystem} from '../../../../src/ngtsc/file_system';
import {Logger, LogLevel} from '../../logging/logger'; import {Logger, LogLevel} from '../../logging/logger';
import {getLockFilePath, LockFile} from '../lock_file'; import {getLockFilePath, LockFile} from '../lock_file';
@ -58,11 +57,6 @@ export class LockFileWithChildProcess implements LockFile {
read(): string { read(): string {
try { 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); return this.fs.readFile(this.path);
} catch { } catch {
return '{unknown}'; return '{unknown}';

View File

@ -8,7 +8,7 @@
import {ChildProcess} from 'child_process'; import {ChildProcess} from 'child_process';
import * as process from '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 {runInEachFileSystem} from '../../../../src/ngtsc/file_system/testing';
import {getLockFilePath} from '../../../src/locking/lock_file'; import {getLockFilePath} from '../../../src/locking/lock_file';
import {LockFileWithChildProcess} from '../../../src/locking/lock_file_with_child_process'; import {LockFileWithChildProcess} from '../../../src/locking/lock_file_with_child_process';
@ -97,18 +97,6 @@ runInEachFileSystem(() => {
const lockFile = new LockFileUnderTest(fs); const lockFile = new LockFileUnderTest(fs);
expect(lockFile.read()).toEqual('{unknown}'); 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()', () => { describe('remove()', () => {