Skip to content

Commit

Permalink
refactor(compiler): improve perf
Browse files Browse the repository at this point in the history 
This refactor improves perf significantly when transform output is not cached, by reducing the scope of each ts.Program.

It also adds a default of tsconfig.incremental = true (if not specified), which seems to help a little.
  • Loading branch information
@johncrim
johncrim committed Jan 30, 2021
1 parent 39eb72d commit 23da68d
Showing 1 changed file with 39 additions and 24 deletions.
63 changes: 39 additions & 24 deletions src/compiler/ng-jest-compiler.ts
View file
@@ -1,3 +1,5 @@
import { normalize } from 'path';

import { CompilerHost, CompilerOptions } from '@angular/compiler-cli';
import { createCompilerHost } from '@angular/compiler-cli/src/transformers/compiler_host';
import type { Logger } from 'bs-logger';
Expand Down Expand Up @@ -71,27 +73,24 @@ export class NgJestCompiler implements CompilerInstance {
module: moduleKind,
};

if (this._program) {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
if (!this.ngJestConfig.isolatedModules) {
/* eslint-disable @typescript-eslint/no-non-null-assertion */
this._tsHost!.updateMemoryHost(fileName, fileContent);

let sourceFile: ts.SourceFile | undefined;
if (!this._rootNames.includes(fileName)) {
this._logger.debug({ fileName }, 'getCompiledOutput: adding file to rootNames and updating Program');
this._rootNames = [...this._rootNames, fileName];
this._createOrUpdateProgram();
sourceFile = this._program.getSourceFile(fileName);
} else {
sourceFile = this._program.getSourceFile(fileName);
if (sourceFile) {
const replaceSpan: ts.TextSpan = { start: 0, length: sourceFile.text.length };
sourceFile.update(fileContent, { span: replaceSpan, newLength: fileContent.length });
}
let sourceFile = this._program?.getSourceFile(fileName);
if (!sourceFile) {
this._logger.debug({ fileName }, 'getCompiledOutput: updating Program');
this._createOrUpdateProgram(fileName);
sourceFile = this._program!.getSourceFile(fileName);
} else if (fileContent !== sourceFile.text) {
// Update source content
const replaceSpan: ts.TextSpan = { start: 0, length: sourceFile.text.length };
sourceFile.update(fileContent, { span: replaceSpan, newLength: fileContent.length });
}

this._logger.debug({ fileName }, 'getCompiledOutput: compiling using Program');

const emitResult = this._program.emit(sourceFile, undefined, undefined, undefined, {
const emitResult = this._program!.emit(sourceFile, undefined, undefined, undefined, {
...customTransformers,
before: [
...(customTransformers.before as Array<ts.TransformerFactory<ts.SourceFile>>),
Expand All @@ -101,21 +100,20 @@ export class NgJestCompiler implements CompilerInstance {
* the transformers are created. Also because program can be updated so we can't push this transformer in
* _createCompilerHost
*/
constructorParametersDownlevelTransform(this._program),
replaceResources(this.isAppPath, this._program.getTypeChecker),
constructorParametersDownlevelTransform(this._program!),
replaceResources(this.isAppPath, this._program!.getTypeChecker),
],
});

// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
const compiledOutput: [string, string] = this._tsHost!.getEmittedResult();
const allDiagnostics: ts.Diagnostic[] = [];
if (this.ngJestConfig.shouldReportDiagnostics(fileName)) {
this._logger.debug({ fileName }, 'getCompiledOutput: getting diagnostics');

allDiagnostics.push(
...emitResult.diagnostics,
...this._program.getSyntacticDiagnostics(sourceFile),
...this._program.getSemanticDiagnostics(sourceFile),
...this._program!.getSyntacticDiagnostics(sourceFile),
...this._program!.getSemanticDiagnostics(sourceFile),
);
}
if (!allDiagnostics.length) {
Expand All @@ -127,6 +125,7 @@ export class NgJestCompiler implements CompilerInstance {

return '';
}
/* eslint-enable @typescript-eslint/no-non-null-assertion */
} else {
this._logger.debug({ fileName }, 'getCompiledOutput: compiling as isolated module');

Expand All @@ -151,7 +150,20 @@ export class NgJestCompiler implements CompilerInstance {
this._logger.debug({ parsedTsConfig }, '_setupOptions: initializing compiler config');

this._compilerOptions = { ...this._initialCompilerOptions };
this._rootNames = parsedTsConfig.rootNames.filter((rootName) => !this.ngJestConfig.isTestFile(rootName));
if (this._compilerOptions.incremental === undefined) {
this._compilerOptions.incremental = true;
}

this._rootNames = parsedTsConfig.rootNames
.filter((rootName) => !this.ngJestConfig.isTestFile(rootName))
.map((rootName) => normalize(rootName));

This comment has been minimized.

Sign in to view

Copy link
@ahnpnl

ahnpnl Jan 30, 2021 •
edited

I think this might be critical point since normalize is related to Windows file path, like I asked you before whether cache is missed while reading files.

Any files passed from jest-runtime is always consistent in path regardless OS, but for TypeScript API it is OS-senstive so I was suspecting about cached file missed.

This comment has been minimized.

Sign in to view

Copy link
@johncrim

johncrim Jan 30, 2021 •
edited

Author Owner

In my project this results in only setup-jest.ts in the rootNames, and its dependencies get pulled into the Program but nothing more. I added the normalize() b/c I saw that setup-jest.ts was getting added 2x, once with forward slashes (ts) and once with backslashes (jest). I used normalize() bc it looks like it's used elsewhere, and I didn't see an internal API to use instead. So I think it's important, but it didn't make a perf difference for me, b/c just a single root.

Our tsconfig.spec.json contains:

  "include": [
    "**/*.spec.ts"
  ],
  "exclude": [
    "node_modules",
    "tmp",
    "karma",
    "**/*.karma-spec.ts",
  ]

Thus parsedTsConfig.rootNames contains test files that are all filtered out here. So I'm pretty sure it didn't make a perf difference in my testing.
if (this.ngJestConfig.jestConfig.setupFiles) {

This comment has been minimized.

Sign in to view

Copy link
@ahnpnl

ahnpnl Jan 30, 2021

Im not sure this gonna help since Jest will just send these files to transformers, the same applied to setupFilesAfterEnv. In general, it's better IMO to not touch files which are defined in jest config

This comment has been minimized.

Sign in to view

Copy link
@johncrim

johncrim Jan 30, 2021

Author Owner

ok - makes sense. I'll try removing them.
this._rootNames.push(...this.ngJestConfig.jestConfig.setupFiles);
}
if (this.ngJestConfig.jestConfig.setupFilesAfterEnv) {
this._rootNames.push(...this.ngJestConfig.jestConfig.setupFilesAfterEnv);
}

if (this._compilerOptions.strictMetadataEmit) {
this._logger.warn(
`Using Angular compiler option 'strictMetadataEmit' for applications might cause undefined behavior.`,
Expand All @@ -168,16 +180,19 @@ export class NgJestCompiler implements CompilerInstance {
options: this._compilerOptions,
tsHost: this._tsHost,
});
this._createOrUpdateProgram();

This comment has been minimized.

Sign in to view

Copy link
@ahnpnl

ahnpnl Jan 30, 2021

I was thinking about removing this too so it won't create Program at startup until process is called. One issue with this is watch mode requires module resolution to be ready therefore https://github.com/thymikee/jest-preset-angular/blob/master/src/compiler/ng-jest-compiler.ts#L42 is there and it requires a Program to be ready to use.
}
}

private _createOrUpdateProgram(): void {
private _createOrUpdateProgram(filePath: string): void {
const oldTsProgram = this._program;

this._logger.debug(`_createOrUpdateProgram: ${oldTsProgram ? 'update' : 'create'} TypeScript Program`);

this._program = this._ts.createProgram(this._rootNames, this._compilerOptions, this._compilerHost, oldTsProgram);
let rootNames = this._rootNames;
if (!rootNames.includes(filePath)) {
rootNames = [...this._rootNames, filePath];
}
this._program = this._ts.createProgram(rootNames, this._compilerOptions, this._compilerHost, oldTsProgram);
}

/**
Expand Down

0 comments on commit 23da68d

Please sign in to comment.