Skip to content

Commit

Permalink
fix: revert commit ad7a297 to fix perf regression
Browse files Browse the repository at this point in the history
As reported, the perf issue is caused by the introduction of an extra `Program` instance to fetch to AST transformers next to the hidden `Program` behind `ts.transpileModule`. We should only use one instance of `Program` therefore we have to manually copy the codes of `ts.transpileModule` to use for our case.

Fixes #2886
  • Loading branch information
ahnpnl committed Dec 3, 2024
1 parent 99bc772 commit b19cd6e
Show file tree
Hide file tree
Showing 17 changed files with 277 additions and 55 deletions.
7 changes: 7 additions & 0 deletions .github/workflows/source_code_ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,10 @@ jobs:
with:
os: ubuntu-latest
node: ${{ matrix.node }}

test_performance:
needs: prepare_cache_ubuntu
uses: ./.github/workflows/test_performance.yml
with:
os: ubuntu-latest
node: 20.x
55 changes: 55 additions & 0 deletions .github/workflows/test_performance.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
name: Test performance

on:
workflow_call:
inputs:
os:
required: true
type: string
node:
required: true
type: string

permissions:
contents: read

jobs:
test:
runs-on: ${{ inputs.os }}

steps:
- name: Checkout 🛎️
uses: actions/checkout@v4
with:
fetch-depth: 20
fetch-tags: false

- name: Restore cached node modules ♻️
id: cache-yarn
uses: actions/cache@v4
with:
path: |
.yarn/cache
node_modules
examples/example-app-monorepo/node_modules
examples/example-app-v17/node_modules
examples/example-app-v18/node_modules
examples/example-app-v19/node_modules
examples/example-app-yarn-workspace/node_modules
key: ${{ inputs.os }}-${{ inputs.node }}-build-${{ hashFiles('**/yarn.lock') }}
restore-keys: |
${{ inputs.os }}-${{ inputs.node }}-build
- name: Setup Node version ⚙️
uses: actions/setup-node@v4
with:
node-version: ${{ inputs.node }}

- name: Build 🏗️
run: yarn build

- name: Install dependencies 📦
run: yarn --cwd performance

- name: Run Performance Tests 🧪
run: yarn test-perf
4 changes: 1 addition & 3 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@ node_modules
.idea
*.tgz
.yarn/*
e2e/**/.yarn/*
examples/**/.yarn/*
website/.yarn/*
**/.yarn/*
tmp
.idx
.vscode
Expand Down
1 change: 1 addition & 0 deletions .npmignore
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Tests
e2e
examples
performance

# sources are inlined
src
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component } from '@angular/core';
import { DOCUMENT } from '@angular/common';
import { Component, Inject } from '@angular/core';
import { RouterLink, RouterOutlet } from '@angular/router';

import { BannerComponent } from './banner/banner.component';
Expand All @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component';
templateUrl: './app.component.html',
imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink],
})
export class AppComponent {}
export class AppComponent {
constructor(@Inject(DOCUMENT) injectedDoc: Document) {
injectedDoc.title = 'Example App';
}
}
9 changes: 7 additions & 2 deletions examples/example-app-v17/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component } from '@angular/core';
import { DOCUMENT } from '@angular/common';
import { Component, Inject } from '@angular/core';
import { RouterLink, RouterOutlet } from '@angular/router';

import { BannerComponent } from './banner/banner.component';
Expand All @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component';
templateUrl: './app.component.html',
imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink],
})
export class AppComponent {}
export class AppComponent {
constructor(@Inject(DOCUMENT) injectedDoc: Document) {
injectedDoc.title = 'Example App';
}
}
9 changes: 7 additions & 2 deletions examples/example-app-v18/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component } from '@angular/core';
import { DOCUMENT } from '@angular/common';
import { Component, Inject } from '@angular/core';
import { RouterLink, RouterOutlet } from '@angular/router';

import { BannerComponent } from './banner/banner.component';
Expand All @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component';
templateUrl: './app.component.html',
imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink],
})
export class AppComponent {}
export class AppComponent {
constructor(@Inject(DOCUMENT) injectedDoc: Document) {
injectedDoc.title = 'Example App';
}
}
9 changes: 7 additions & 2 deletions examples/example-app-v19/src/app/app.component.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component } from '@angular/core';
import { DOCUMENT } from '@angular/common';
import { Component, Inject } from '@angular/core';
import { RouterLink, RouterOutlet } from '@angular/router';

import { BannerComponent } from './banner/banner.component';
Expand All @@ -9,4 +10,8 @@ import { WelcomeComponent } from './welcome/welcome.component';
templateUrl: './app.component.html',
imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink],
})
export class AppComponent {}
export class AppComponent {
constructor(@Inject(DOCUMENT) injectedDoc: Document) {
injectedDoc.title = 'Example App';
}
}
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Component } from '@angular/core';
import { DOCUMENT } from '@angular/common';
import { Component, Inject } from '@angular/core';
import { RouterLink, RouterOutlet } from '@angular/router';

import { BannerComponent } from './banner/banner.component';
Expand All @@ -10,4 +11,8 @@ import { WelcomeComponent } from './welcome/welcome.component';
templateUrl: './app.component.html',
imports: [BannerComponent, WelcomeComponent, RouterOutlet, RouterLink],
})
export class AppComponent {}
export class AppComponent {
constructor(@Inject(DOCUMENT) injectedDoc: Document) {
injectedDoc.title = 'Example App';
}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"test-cjs": "jest -c=jest-cjs.config.ts --no-cache && jest -c=jest-transpile-cjs.config.ts --no-cache",
"test-esm": "node --experimental-vm-modules --no-warnings node_modules/jest/bin/jest.js -c=jest-esm.config.ts --no-cache && node --experimental-vm-modules --no-warnings node_modules/jest/bin/jest.js -c=jest-transpile-esm.config.ts --no-cache",
"test-examples": "yarn build && node scripts/test-examples.js",
"test-perf": "jest -c=performance/jest.config.ts --no-cache",
"doc": "cd website && yarn start",
"doc:build": "cd website && yarn build",
"changelog": "conventional-changelog -p angular -i CHANGELOG.md -s -r 1",
Expand Down
40 changes: 40 additions & 0 deletions performance/__tests__/perf.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
const testFunction = async () => {
const { camelCase } = await import('lodash-es');
camelCase('FooBar');
};

describe('Performance Measurement', () => {
it('should complete expensive operation within acceptable performance', async () => {
const runs = 5;
const times: number[] = [];

for (let i = 0; i < runs; i++) {
const start = performance.now();
await testFunction();
const end = performance.now();
times.push(end - start);
}

const average = times.reduce((a, b) => a + b, 0) / runs;
const min = Math.min(...times);
const max = Math.max(...times);
const standardDeviation = Math.sqrt(times.reduce((sq, n) => sq + Math.pow(n - average, 2), 0) / runs);

console.log('Performance Metrics:', {
average: `${average.toFixed(2)}ms`,
min: `${min.toFixed(2)}ms`,
max: `${max.toFixed(2)}ms`,
standardDeviation: `${standardDeviation.toFixed(2)}ms`,
});

const acceptableThresholds = {
averageMax: 700,
standardDeviationMax: 1500,
absoluteMax: 3300,
};

expect(average).toBeLessThan(acceptableThresholds.averageMax);
expect(standardDeviation).toBeLessThan(acceptableThresholds.standardDeviationMax);
expect(max).toBeLessThan(acceptableThresholds.absoluteMax);
});
});
16 changes: 16 additions & 0 deletions performance/jest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import type { JestConfigWithTsJest } from 'ts-jest';

const config: JestConfigWithTsJest = {
testEnvironment: 'jsdom',
transform: {
'^.+\\.(ts|mjs|js|html)$': [
'<rootDir>/../build/index.js',
{
isolatedModules: true,
},
],
},
transformIgnorePatterns: ['node_modules/(?!lodash-es)'],
};

export default config;
8 changes: 8 additions & 0 deletions performance/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"name": "performance",
"private": true,
"devDependencies": {
"@types/lodash-es": "^4.17.12",
"lodash-es": "^4.17.21"
}
}
3 changes: 3 additions & 0 deletions performance/tsconfig.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"extends": "../tsconfig.json",
}
38 changes: 38 additions & 0 deletions performance/yarn.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# This file is generated by running "yarn install" inside your project.
# Manual changes might be lost - proceed with caution!

__metadata:
version: 8
cacheKey: 10

"@types/lodash-es@npm:^4.17.12":
version: 4.17.12
resolution: "@types/lodash-es@npm:4.17.12"
dependencies:
"@types/lodash": "npm:*"
checksum: 10/56b9a433348b11c31051c6fa9028540a033a08fb80b400c589d740446c19444d73b217cf1471d4036448ef686a83e8cf2a35d1fadcb3f2105f26701f94aebb07
languageName: node
linkType: hard

"@types/lodash@npm:*":
version: 4.14.182
resolution: "@types/lodash@npm:4.14.182"
checksum: 10/6c0d3fa682331d7631676817acf4b8b74842a9df0fb63dacbbc6a31b94e266edca550ac096cec8ce95df4fc72cf550a6321322e27872d4dfa15c1003197f6c85
languageName: node
linkType: hard

"lodash-es@npm:^4.17.21":
version: 4.17.21
resolution: "lodash-es@npm:4.17.21"
checksum: 10/03f39878ea1e42b3199bd3f478150ab723f93cc8730ad86fec1f2804f4a07c6e30deaac73cad53a88e9c3db33348bb8ceeb274552390e7a75d7849021c02df43
languageName: node
linkType: hard

"performance@workspace:.":
version: 0.0.0-use.local
resolution: "performance@workspace:."
dependencies:
"@types/lodash-es": "npm:^4.17.12"
lodash-es: "npm:^4.17.21"
languageName: unknown
linkType: soft
3 changes: 2 additions & 1 deletion src/compiler/ng-jest-compiler.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -323,9 +323,10 @@ describe('NgJestCompiler', () => {
`);

expect(diagnostics.length).toBe(0);
// The `type: undefined` is ok here which is covered by example apps' tests.
expect(code).toContain(dedent`
MyDir.ctorParameters = () => [
{ type: Document, decorators: [{ type: core_1.Inject, args: [core_1.DOCUMENT,] }] }
{ type: undefined, decorators: [{ type: core_1.Inject, args: [core_1.DOCUMENT,] }] }
];
exports.MyDir = MyDir = tslib_1.__decorate([
(0, core_1.Directive)()
Expand Down
Loading

0 comments on commit b19cd6e

Please sign in to comment.