Skip to content

perf(ivy): ngcc - exit early if the targeted package has been compiled #29740

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.

Sign up for GitHub

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

Closed
petebacondarwin wants to merge 2 commits into from
Closed

perf(ivy): ngcc - exit early if the targeted package has been compiled #29740

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c718ce9
test(ivy): ngcc - improve and use the `MockLogger`
petebacondarwin Apr 6, 2019
86c941c
perf(ivy): ngcc - exit early if the targeted package has been compiled
petebacondarwin Apr 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions packages/compiler-cli/ngcc/src/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,14 @@ export function mainNgcc({basePath, targetEntryPointPath,
const absoluteTargetEntryPointPath = targetEntryPointPath ?
AbsoluteFsPath.from(resolve(basePath, targetEntryPointPath)) :
undefined;

if (absoluteTargetEntryPointPath &&
hasProcessedTargetEntryPoint(
absoluteTargetEntryPointPath, propertiesToConsider, compileAllFormats)) {
logger.info('The target entry-point has already been processed');
return;
}

const {entryPoints} =
finder.findEntryPoints(AbsoluteFsPath.from(basePath), absoluteTargetEntryPointPath);

Expand Down Expand Up @@ -167,3 +175,28 @@ export function mainNgcc({basePath, targetEntryPointPath,
function getFileWriter(createNewEntryPointFormats: boolean): FileWriter {
return createNewEntryPointFormats ? new NewEntryPointFileWriter() : new InPlaceFileWriter();
}

function hasProcessedTargetEntryPoint(
Copy link
Copy Markdown
Contributor

@alan-agius4 alan-agius4 Apr 6, 2019

Choose a reason for hiding this comment

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

This looks good

targetPath: AbsoluteFsPath, propertiesToConsider: string[], compileAllFormats: boolean) {
const packageJsonPath = AbsoluteFsPath.from(resolve(targetPath, 'package.json'));
Copy link
Copy Markdown
Member

@gkalpak gkalpak Apr 9, 2019

Choose a reason for hiding this comment

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

Since you are concerned with speed. fromUnchecked() should be (negligibly) faster (and sufficient) 😁

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Apr 9, 2019

Choose a reason for hiding this comment

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

I'll fix that up next time I am touching this code.

const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8'));

for (const property of propertiesToConsider) {
if (packageJson[property]) {
// Here is a property that should be processed
if (hasBeenProcessed(packageJson, property as EntryPointJsonProperty)) {
if (!compileAllFormats) {
// It has been processed and we only need one, so we are done.
return true;
}
} else {
// It has not been processed but we need all of them, so we are done.
return false;
}
}
}
// Either all formats need to be compiled and there were none that were unprocessed,
// Or only the one matching format needs to be compiled but there was at least one matching
Copy link
Copy Markdown
Member

@gkalpak gkalpak Apr 9, 2019

Choose a reason for hiding this comment

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

Or --> or

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Apr 9, 2019

Choose a reason for hiding this comment

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

😱

// property before the first processed format that was unprocessed.
return true;
}
2 changes: 2 additions & 0 deletions packages/compiler-cli/ngcc/test/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ ts_library(
),
deps = [
"//packages/compiler-cli/ngcc",
"//packages/compiler-cli/ngcc/test/helpers",
"//packages/compiler-cli/src/ngtsc/imports",
"//packages/compiler-cli/src/ngtsc/partial_evaluator",
"//packages/compiler-cli/src/ngtsc/path",
Expand Down Expand Up @@ -46,6 +47,7 @@ ts_library(
),
deps = [
"//packages/compiler-cli/ngcc",
"//packages/compiler-cli/ngcc/test/helpers",
"//packages/compiler-cli/src/ngtsc/path",
"//packages/compiler-cli/test:test_utils",
"@npm//@types/mock-fs",
Expand Down
16 changes: 16 additions & 0 deletions packages/compiler-cli/ngcc/test/helpers/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
load("//tools:defaults.bzl", "ts_library")

package(default_visibility = ["//packages/compiler-cli/ngcc:__subpackages__"])

ts_library(
name = "helpers",
testonly = True,
srcs = glob([
"*.ts",
]),
deps = [
"//packages/compiler-cli/ngcc",
"//packages/compiler-cli/src/ngtsc/path",
"//packages/compiler-cli/src/ngtsc/testing",
],
)
15 changes: 10 additions & 5 deletions packages/compiler-cli/ngcc/test/helpers/mock_logger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,14 @@
import {Logger} from '../../src/logging/logger';

export class MockLogger implements Logger {
logs: string[][] = [];
debug(...args: string[]) { this.logs.push(args); }
info(...args: string[]) { this.logs.push(args); }
warn(...args: string[]) { this.logs.push(args); }
error(...args: string[]) { this.logs.push(args); }
logs: {[P in keyof Logger]: string[][]} = {
debug: [],
info: [],
warn: [],
error: [],
};
debug(...args: string[]) { this.logs.debug.push(args); }
info(...args: string[]) { this.logs.info.push(args); }
warn(...args: string[]) { this.logs.warn.push(args); }
error(...args: string[]) { this.logs.error.push(args); }
}
123 changes: 109 additions & 14 deletions packages/compiler-cli/ngcc/test/integration/ngcc_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,19 @@
* found in the LICENSE file at https://angular.io/license
*/

import {existsSync, readFileSync, readdirSync, statSync} from 'fs';
import {AbsoluteFsPath} from '@angular/compiler-cli/src/ngtsc/path';
import {existsSync, readFileSync, readdirSync, statSync, writeFileSync} from 'fs';
import * as mockFs from 'mock-fs';
import {join} from 'path';
const Module = require('module');

import {mainNgcc} from '../../src/main';
import {getAngularPackagesFromRunfiles, resolveNpmTreeArtifact} from '../../../test/runfile_helpers';
import {EntryPointPackageJson} from '../../src/packages/entry_point';
import {Logger} from '../../src/logging/logger';
import {mainNgcc} from '../../src/main';
import {markAsProcessed} from '../../src/packages/build_marker';
import {EntryPointJsonProperty, EntryPointPackageJson, SUPPORTED_FORMAT_PROPERTIES} from '../../src/packages/entry_point';
import {MockLogger} from '../helpers/mock_logger';

const _ = AbsoluteFsPath.from;
Copy link
Copy Markdown
Contributor

@alan-agius4 alan-agius4 Apr 6, 2019

Choose a reason for hiding this comment

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

Why naming this _?

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Apr 6, 2019

Choose a reason for hiding this comment

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

In unit tests I have to create AbsoluteFsPaths from strings all over the place, so I got into a habit of aliasing AbsoluteFsPath.from() to _() to make the tests less verbose.

I realise that in this file I only use it once so perhaps that isn't necessary. :-)

Copy link
Copy Markdown
Contributor

@alan-agius4 alan-agius4 Apr 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I typically associate _ with lodash or underscore 😜.

Copy link
Copy Markdown
Contributor Author

@petebacondarwin petebacondarwin Apr 6, 2019

Choose a reason for hiding this comment

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

That's so 1990s :-P

Check out https://github.com/angular/angular/blob/master/packages/compiler-cli/ngcc/test/packages/dependency_host_spec.ts for an example of this being actually useful.


describe('ngcc main()', () => {
beforeEach(createMockFileSystem);
Expand All @@ -26,7 +30,11 @@ describe('ngcc main()', () => {
});

it('should run ngcc without errors for esm5', () => {
expect(() => mainNgcc({basePath: '/node_modules', propertiesToConsider: ['esm5']}))
expect(() => mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['esm5'],
logger: new MockLogger(),
}))
.not.toThrow();
});

Expand Down Expand Up @@ -82,12 +90,86 @@ describe('ngcc main()', () => {
});
});

describe('early skipping of target entry-point', () => {
describe('[compileAllFormats === true]', () => {
it('should skip all processing if all the properties are marked as processed', () => {
const logger = new MockLogger();
markPropertiesAsProcessed('@angular/common/http/testing', SUPPORTED_FORMAT_PROPERTIES);
mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: '@angular/common/http/testing', logger,
});
expect(logger.logs.info).toContain(['The target entry-point has already been processed']);
});

it('should process the target if any `propertyToConsider` is not marked as processed', () => {
const logger = new MockLogger();
markPropertiesAsProcessed('@angular/common/http/testing', ['esm2015', 'fesm2015']);
mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: '@angular/common/http/testing',
propertiesToConsider: ['fesm2015', 'esm5', 'esm2015'], logger,
});
expect(logger.logs.info).not.toContain([
'The target entry-point has already been processed'
]);
});
});

describe('[compileAllFormats === false]', () => {
it('should process the target if the first matching `propertyToConsider` is not marked as processed',
() => {
const logger = new MockLogger();
markPropertiesAsProcessed('@angular/common/http/testing', ['esm2015']);
mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: '@angular/common/http/testing',
propertiesToConsider: ['esm5', 'esm2015'],
compileAllFormats: false, logger,
});

expect(logger.logs.info).not.toContain([
'The target entry-point has already been processed'
]);
});

it('should skip all processing if the first matching `propertyToConsider` is marked as processed',
() => {
const logger = new MockLogger();
markPropertiesAsProcessed('@angular/common/http/testing', ['esm2015']);
mainNgcc({
basePath: '/node_modules',
targetEntryPointPath: '@angular/common/http/testing',
// Simulate a property that does not exist on the package.json and will be ignored.
propertiesToConsider: ['missing', 'esm2015', 'esm5'],
compileAllFormats: false, logger,
});

expect(logger.logs.info).toContain([
'The target entry-point has already been processed'
]);
});
});
});


function markPropertiesAsProcessed(packagePath: string, properties: EntryPointJsonProperty[]) {
const basePath = '/node_modules';
const targetPackageJsonPath = _(join(basePath, packagePath, 'package.json'));
const targetPackage = loadPackage(packagePath);
markAsProcessed(targetPackage, targetPackageJsonPath, 'typings');
properties.forEach(property => markAsProcessed(targetPackage, targetPackageJsonPath, property));
}


describe('with propertiesToConsider', () => {
it('should only compile the entry-point formats given in the `propertiesToConsider` list',
() => {
mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['main', 'esm5', 'module', 'fesm5']
propertiesToConsider: ['main', 'esm5', 'module', 'fesm5'],
logger: new MockLogger(),

});

// * the `main` property is UMD, which is not yet supported.
Expand Down Expand Up @@ -125,7 +207,9 @@ describe('ngcc main()', () => {
mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['main', 'module', 'fesm5', 'esm5'],
compileAllFormats: false
compileAllFormats: false,
logger: new MockLogger(),

});
// * The `main` is UMD, which is not yet supported, and so is not compiled.
// * In the Angular packages fesm5 and module have the same underlying format,
Expand Down Expand Up @@ -158,15 +242,21 @@ describe('ngcc main()', () => {
mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['module'],
compileAllFormats: false
compileAllFormats: false,
logger: new MockLogger(),

});
expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({
module: '0.0.0-PLACEHOLDER',
typings: '0.0.0-PLACEHOLDER',
});
// If ngcc tries to write out the typings files again, this will throw an exception.
mainNgcc(
{basePath: '/node_modules', propertiesToConsider: ['esm5'], compileAllFormats: false});
mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['esm5'],
compileAllFormats: false,
logger: new MockLogger(),
});
expect(loadPackage('@angular/core').__processed_by_ivy_ngcc__).toEqual({
esm5: '0.0.0-PLACEHOLDER',
module: '0.0.0-PLACEHOLDER',
Expand All @@ -181,7 +271,9 @@ describe('ngcc main()', () => {
mainNgcc({
basePath: '/node_modules',
createNewEntryPointFormats: true,
propertiesToConsider: ['esm5']
propertiesToConsider: ['esm5'],
logger: new MockLogger(),

});

// Updates the package.json
Expand Down Expand Up @@ -222,9 +314,12 @@ describe('ngcc main()', () => {
});

it('should use a custom logger if provided', () => {
const logger: Logger = jasmine.createSpyObj(['debug', 'info', 'warn', 'error']);
mainNgcc({basePath: '/node_modules', propertiesToConsider: ['esm2015'], logger});
expect(logger.info).toHaveBeenCalled();
const logger = new MockLogger();
mainNgcc({
basePath: '/node_modules',
propertiesToConsider: ['esm2015'], logger,
});
expect(logger.logs.info).toContain(['Compiling @angular/common/http : esm2015 as esm2015']);
});
});
});
Expand Down