Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Performance: Cache regular expression instead of creating anew for every file in ScriptTransformer. #8235

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -46,6 +46,7 @@
- `[jest-core]` Improve performance of SearchSource.findMatchingTests by 15% ([#8184](https://github.com/facebook/jest/pull/8184))
- `[jest-resolve]` Optimize internal cache lookup performance ([#8183](https://github.com/facebook/jest/pull/8183))
- `[jest-core]` Dramatically improve watch mode performance ([#8201](https://github.com/facebook/jest/pull/8201))
- `[jest-transform]` Cache regular expression instead of creating anew for every file in ScriptTransformer ([#8235](https://github.com/facebook/jest/pull/8235))
- `[jest-core]` Fix memory leak of source map info and minor performance improvements ([#8234](https://github.com/facebook/jest/pull/8234))
- `[jest-console]` Fix memory leak by releasing console output reference when printed to stdout ([#8233](https://github.com/facebook/jest/pull/8233))
- `[jest-runtime]` Use `Map` instead of `Object` for module registry ([#8232](https://github.com/facebook/jest/pull/8232))
Expand Down
56 changes: 38 additions & 18 deletions packages/jest-transform/src/ScriptTransformer.ts
Expand Up @@ -31,7 +31,8 @@ import enhanceUnexpectedTokenMessage from './enhanceUnexpectedTokenMessage';

type ProjectCache = {
configString: string;
ignorePatternsRegExp: RegExp | null;
ignorePatternsRegExp?: RegExp;
transformRegExp?: Array<[RegExp, string]>;
transformedFiles: Map<string, TransformResult>;
};

Expand Down Expand Up @@ -64,7 +65,8 @@ export default class ScriptTransformer {
if (!projectCache) {
projectCache = {
configString: stableStringify(this._config),
ignorePatternsRegExp: calcIgnorePatternRegexp(this._config),
ignorePatternsRegExp: calcIgnorePatternRegExp(this._config),
transformRegExp: calcTransformRegExp(this._config),
transformedFiles: new Map(),
};

Expand Down Expand Up @@ -131,12 +133,18 @@ export default class ScriptTransformer {
}

private _getTransformPath(filename: Config.Path) {
for (let i = 0; i < this._config.transform.length; i++) {
if (new RegExp(this._config.transform[i][0]).test(filename)) {
return this._config.transform[i][1];
const transformRegExp = this._cache.transformRegExp;
if (!transformRegExp) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this be falsy?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the array passed from config with transforms is 0-length.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, transform: {}? Gotcha

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

calcTransformRegExp returns undefined if config.transform.length === 0.

I try not to return empty arrays to loop through when the function should actually be telling you "nothing to do here". I personally find it makes the code more readable and sometimes make optimizations in the future possible.

return undefined;
}

for (let i = 0; i < transformRegExp.length; i++) {
if (transformRegExp[i][0].test(filename)) {
return transformRegExp[i][1];
}
}
return null;

return undefined;
}

private _getTransformer(filename: Config.Path) {
Expand Down Expand Up @@ -371,21 +379,19 @@ export default class ScriptTransformer {
options: Options,
fileSource?: string,
): TransformResult {
let scriptCacheKey = null;
let scriptCacheKey = undefined;
let instrument = false;
let result: TransformResult | undefined;

if (!options.isCoreModule) {
instrument = shouldInstrument(filename, options, this._config);
scriptCacheKey = getScriptCacheKey(filename, instrument);
result = this._cache.transformedFiles.get(scriptCacheKey);
}

if (result) {
return result;
const result = this._cache.transformedFiles.get(scriptCacheKey);
if (result) {
return result;
}
}

result = this._transformAndBuildScript(
const result = this._transformAndBuildScript(
filename,
options,
instrument,
Expand Down Expand Up @@ -539,19 +545,33 @@ const getScriptCacheKey = (filename: Config.Path, instrument: boolean) => {
return filename + '_' + mtime.getTime() + (instrument ? '_instrumented' : '');
};

const calcIgnorePatternRegexp = (
config: Config.ProjectConfig,
): RegExp | null => {
const calcIgnorePatternRegExp = (config: Config.ProjectConfig) => {
if (
!config.transformIgnorePatterns ||
config.transformIgnorePatterns.length === 0
) {
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this changed introduced a lint warning

image

return;
}

return new RegExp(config.transformIgnorePatterns.join('|'));
};

const calcTransformRegExp = (config: Config.ProjectConfig) => {
if (!config.transform.length) {
return;
}

const transformRegexp: Array<[RegExp, string]> = [];
for (let i = 0; i < config.transform.length; i++) {
transformRegexp.push([
new RegExp(config.transform[i][0]),
config.transform[i][1],
]);
}

return transformRegexp;
};

const wrap = (content: string, ...extras: Array<string>) => {
const globals = new Set([
'module',
Expand Down