diff --git a/.github/workflows/benchmark-compare.yml b/.github/workflows/benchmark-compare.yml index 70e7d4240e3..49bdb2b6a9f 100644 --- a/.github/workflows/benchmark-compare.yml +++ b/.github/workflows/benchmark-compare.yml @@ -39,17 +39,19 @@ jobs: # We cannot use `angular/dev-infra/github-actions/npm/checkout-and-setup-node` here # because it does not support checking out from a fork (as it lacks a `repository` input). # Thus, we checkout and setup Node/pnpm manually. - - name: Install pnpm - uses: pnpm/action-setup@0e279bb959325dab635dd2c09392533439d90093 # v6.0.8 - - name: Setup Node.js uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: node-version-file: '.nvmrc' - cache: 'pnpm' + + - name: Install pnpm + uses: pnpm/action-setup@0e279bb959325dab635dd2c09392533439d90093 # v6.0.8 - run: pnpm install --frozen-lockfile + - name: Setup Bazel + uses: angular/dev-infra/github-actions/bazel/setup@442c2fcbf06a321b5196b4c5fc70e78a49242958 + - uses: angular/dev-infra/github-actions/bazel/configure-remote@442c2fcbf06a321b5196b4c5fc70e78a49242958 with: bazelrc: ./.bazelrc.user @@ -61,10 +63,12 @@ jobs: COMMENT_BODY: ${{ github.event.comment.body }} run: pnpm benchmarks prepare-for-github-action "$COMMENT_BODY" - - run: pnpm benchmarks run-compare ${{steps.info.outputs.compareSha}} "${{steps.info.outputs.benchmarkTarget}}" - + - run: pnpm benchmarks run-compare "$COMPARE_SHA" "$BENCHMARK_TARGET" id: benchmark name: Running benchmark + env: + BENCHMARK_TARGET: ${{steps.info.outputs.benchmarkTarget}} + COMPARE_SHA: ${{steps.info.outputs.compareSha}} - uses: peter-evans/create-or-update-comment@e8674b075228eee787fea43ef493e45ece1004c9 # v5 with: diff --git a/packages/benchpress/package.json b/packages/benchpress/package.json index be7b8e41ba6..10b9a834136 100644 --- a/packages/benchpress/package.json +++ b/packages/benchpress/package.json @@ -4,7 +4,8 @@ "description": "Benchpress - a framework for e2e performance tests", "dependencies": { "@angular/core": "^22.0.0-next", - "reflect-metadata": "^0.2.0" + "reflect-metadata": "^0.2.0", + "tslib": "^2.3.0" }, "repository": { "type": "git", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0f66a20e069..89c621fea04 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -916,6 +916,9 @@ importers: reflect-metadata: specifier: ^0.2.0 version: 0.2.2 + tslib: + specifier: ^2.3.0 + version: 2.8.1 packages/common: dependencies: diff --git a/scripts/benchmarks/index.mts b/scripts/benchmarks/index.mts index d84d05163d3..d77d3bc00c3 100644 --- a/scripts/benchmarks/index.mts +++ b/scripts/benchmarks/index.mts @@ -6,6 +6,8 @@ * found in the LICENSE file at https://angular.dev/license */ +import fs from 'fs'; +import path from 'path'; import {setOutput} from '@actions/core'; import {GitClient, Log, bold, green, yellow} from '@angular/ng-dev'; import {select} from '@inquirer/prompts'; @@ -17,7 +19,7 @@ import { getTestlogPath, resolveTarget, } from './targets.mts'; -import {exec} from './utils.mts'; +import {exec, projectDir} from './utils.mts'; const benchmarkTestFlags = [ '--cache_test_results=no', @@ -27,6 +29,9 @@ const benchmarkTestFlags = [ // reduce fluctuation. Output streamed ensures that deps can build with RBE, but // tests run locally while also providing useful output for debugging. '--test_output=streamed', + // In the comparison run, we create a hybrid workspace (main files + PR scripts/lockfiles). + // This causes a lockfile mismatch, so we must allow Bazel to update the lockfile in memory. + '--lockfile_mode=update', ]; await yargs(process.argv.slice(2)) @@ -98,11 +103,11 @@ async function prepareForGitHubAction(commentBody: string): Promise { // Attempt to find the compare SHA. The commit may be either part of the // pull request, or might be a commit unrelated to the PR- but part of the // upstream repository. We attempt to fetch/resolve the SHA in both remotes. - const compareRefResolve = git.runGraceful(['rev-parse', compareRefRaw]); + const compareRefResolve = git.runGraceful(['rev-parse', '--', compareRefRaw]); let compareRefSha = compareRefResolve.stdout.trim(); if (compareRefSha === '' || compareRefResolve.status !== 0) { git.run(['fetch', '--depth=1', git.getRepoGitUrl(), compareRefRaw]); - compareRefSha = git.run(['rev-parse', 'FETCH_HEAD']).stdout.trim(); + compareRefSha = git.run(['rev-parse', '--', 'FETCH_HEAD']).stdout.trim(); } setOutput('compareSha', compareRefSha); @@ -126,8 +131,8 @@ async function runBenchmarkCmd(bazelTargetRaw: string | undefined): Promise { - await exec('bazel', ['test', bazelTarget, ...benchmarkTestFlags]); +async function runBenchmarkTarget(bazelTarget: ResolvedTarget, cwd?: string): Promise { + await exec('pnpm', ['bazel', 'test', bazelTarget, ...benchmarkTestFlags], cwd); } /** @@ -138,13 +143,6 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string const git = await GitClient.get(); const currentRef = git.getCurrentBranchOrRevision(); - if (git.hasUncommittedChanges()) { - Log.warn(bold('You have uncommitted changes.')); - Log.warn('The script will stash your changes and re-apply them so that'); - Log.warn('the comparison ref can be checked out.'); - Log.warn(''); - } - if (bazelTargetRaw === undefined) { bazelTargetRaw = await promptForBenchmarkTarget(); } @@ -159,29 +157,92 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string const workingDirResults = await collectBenchmarkResults(testlogPath); - // Stash working directory as we might be in the middle of developing - // and we wouldn't want to discard changes when checking out the compare SHA. - git.run(['stash']); + // Define isolated temporary workspace inside `dist/` so it is ignored by git. + const tempDir = path.join(projectDir, 'dist/benchmark-compare-temp'); + let comparisonResults: any = null; try { - Log.log(green('Fetching comparison revision.')); - // Note: Not using a shallow fetch here as that would convert the local - // user repository into an incomplete repository. - git.run(['fetch', git.getRepoGitUrl(), compareRef]); - Log.log(green('Checking out comparison revision.')); - git.run(['checkout', 'FETCH_HEAD']); + Log.log(green(`Creating isolated workspace in ${tempDir}`)); + try { + git.run(['worktree', 'remove', '--force', tempDir]); + } catch (e) { + if (fs.existsSync(tempDir)) { + fs.rmSync(tempDir, {recursive: true, force: true}); + } + try { + git.run(['worktree', 'prune']); + } catch (pruneError) { + // Ignore prune errors + } + } - await exec('pnpm', ['install', '--frozen-lockfile']); - await runBenchmarkTarget(bazelTarget); + // Ensure the comparison ref is fetched on the main repository if not already present. + const hasCommit = git.runGraceful(['cat-file', '-e', `${compareRef}^{commit}`]).status === 0; + if (!hasCommit) { + Log.log(green(`Fetching comparison revision ${compareRef}...`)); + git.run(['fetch', git.getRepoGitUrl(), compareRef]); + } else { + Log.log( + green(`Comparison revision ${compareRef} is already available locally. Skipping fetch.`), + ); + } + + // Create isolated workspace instantly using native git worktree. + Log.log(green(`Creating isolated worktree for ${compareRef} in ${tempDir}`)); + git.run(['worktree', 'add', '--detach', tempDir, compareRef]); + + // Copy the current PR's benchmark scripts and packages into the isolated workspace. + // Explicitly exclude node_modules to avoid copying broken relative symlinks. + Log.log(green('Copying PR benchmark scripts and packages into isolated workspace...')); + const dirsToCopy = ['scripts/benchmarks', 'packages/benchpress']; + for (const relDir of dirsToCopy) { + const src = path.join(projectDir, relDir); + const dest = path.join(tempDir, relDir); + fs.rmSync(dest, {recursive: true, force: true}); + fs.cpSync(src, dest, { + recursive: true, + filter: (srcPath) => !srcPath.split(path.sep).includes('node_modules'), + }); + } + + // Copy `.bazelrc.user` if it exists, otherwise create it. + const bazelrcUser = path.join(projectDir, '.bazelrc.user'); + const tempBazelrcUser = path.join(tempDir, '.bazelrc.user'); + if (fs.existsSync(bazelrcUser)) { + fs.copyFileSync(bazelrcUser, tempBazelrcUser); + } else { + fs.writeFileSync(tempBazelrcUser, ''); + } + + // Run pnpm install inside the isolated workspace. + Log.log(green('Installing dependencies in isolated workspace...')); + await exec('pnpm', ['install', '--no-frozen-lockfile', '--prefer-offline'], tempDir); + + // Run the benchmark on the comparison workspace. + Log.log(green('Running benchmark in isolated workspace...')); + await runBenchmarkTarget(bazelTarget, tempDir); + + // Resolve testlog path and collect results from the isolated workspace. + Log.log(green('Collecting comparison results...')); + const tempTestlogPath = await getTestlogPath(bazelTarget, tempDir); + comparisonResults = await collectBenchmarkResults(tempTestlogPath); } finally { - restoreWorkingStage(git, currentRef); + Log.log(green('Cleaning up isolated workspace...')); + try { + git.run(['worktree', 'remove', '--force', tempDir]); + } catch (e) { + Log.warn(`Failed to clean up isolated worktree: ${e}`); + if (fs.existsSync(tempDir)) { + fs.rmSync(tempDir, {recursive: true, force: true}); + } + try { + git.run(['worktree', 'prune']); + } catch (pruneError) { + // Ignore prune errors + } + } } - // Re-install dependencies for `HEAD`. - await exec('pnpm', ['install', '--frozen-lockfile']); - - const comparisonResults = await collectBenchmarkResults(testlogPath); - // If we are running in a GitHub action, expose the benchmark text // results as outputs. Useful if those are exposed as a GitHub comment then. if (process.env.GITHUB_ACTION !== undefined) { @@ -198,11 +259,3 @@ async function runCompare(bazelTargetRaw: string | undefined, compareRef: string Log.info(bold(yellow(`Working stage (${currentRef}) results:`)), '\n'); Log.info(workingDirResults.summaryConsoleText); } - -function restoreWorkingStage(git: GitClient, initialRef: string) { - Log.log(green('Restoring working stage')); - git.run(['checkout', '-f', initialRef]); - - // Stash apply could fail if there were not changes in the working stage. - git.runGraceful(['stash', 'apply']); -} diff --git a/scripts/benchmarks/results.mts b/scripts/benchmarks/results.mts index 288bc3cf5d2..486f227eb55 100644 --- a/scripts/benchmarks/results.mts +++ b/scripts/benchmarks/results.mts @@ -6,6 +6,7 @@ * found in the LICENSE file at https://angular.dev/license */ +import fs from 'fs'; import path from 'path'; import Zip from 'adm-zip'; @@ -16,6 +17,7 @@ interface JsonReport { metricsText: string; statsText: string; validSampleTexts: string[]; + completeSample?: any; } /** Results of an individual benchmark scenario. */ @@ -38,31 +40,80 @@ export interface OverallResult { /** Collects and parses the benchmark results of the given Bazel target testlog directory. */ export function collectBenchmarkResults(testlogDir: string): OverallResult { - const z = new Zip(path.join(testlogDir, 'test.outputs/outputs.zip')); const scenarioResults: ScenarioResult[] = []; + const zipPath = path.join(testlogDir, 'test.outputs/outputs.zip'); - for (const e of z.getEntries()) { - if (path.extname(e.entryName) !== '.json') { - continue; + if (fs.existsSync(zipPath)) { + const z = new Zip(zipPath); + for (const e of z.getEntries()) { + if (path.extname(e.entryName) !== '.json') { + continue; + } + + try { + const data = JSON.parse(z.readAsText(e.entryName)); + if (isJsonReport(data)) { + addScenarioResult(data, scenarioResults); + } + } catch (err) { + // Skip files that fail to parse + } } + } else { + const outputsDir = path.join(testlogDir, 'test.outputs'); + if (fs.existsSync(outputsDir)) { + for (const file of fs.readdirSync(outputsDir)) { + if (path.extname(file) !== '.json') { + continue; + } - const data = JSON.parse(z.readAsText(e.entryName)); + const filePath = path.join(outputsDir, file); + if (!fs.statSync(filePath).isFile()) { + continue; + } - // Skip files that do not look like benchpress reports. - if (!isJsonReport(data)) { - continue; + let data; + try { + data = JSON.parse(fs.readFileSync(filePath, 'utf-8')); + } catch (e) { + continue; + } + + if (!isJsonReport(data)) { + continue; + } + + addScenarioResult(data, scenarioResults); + } } + } - scenarioResults.push({ - id: data.description.id, - data, - // Output used for console output when running locally/CI. - summaryConsoleText: `\ + if (scenarioResults.length === 0) { + throw new Error(`No valid benchpress benchmark reports found in "${testlogDir}".`); + } + + return { + scenarios: scenarioResults, + summaryConsoleText: scenarioResults + .map((s) => `${bold(s.id)}\n\n${s.summaryConsoleText}`) + .join('\n\n'), + summaryMarkdownText: scenarioResults + .map((s) => `### ${s.id}\n\n${s.summaryMarkdownText}`) + .join('\n\n'), + }; +} + +function addScenarioResult(data: JsonReport, scenarioResults: ScenarioResult[]) { + scenarioResults.push({ + id: data.description.id, + data, + // Output used for console output when running locally/CI. + summaryConsoleText: `\ ${data.metricsText} ${data.validSampleTexts.join('\n')} ${data.statsText}`, - // Output used for e.g. GitHub actions. - summaryMarkdownText: `\ + // Output used for e.g. GitHub actions. + summaryMarkdownText: `\
Full example results \`\`\` @@ -77,21 +128,10 @@ ${data.statsText} ${data.metricsText} ${data.statsText} \`\`\``, - }); - } - - return { - scenarios: scenarioResults, - summaryConsoleText: scenarioResults - .map((s) => `${bold(s.id)}\n\n${s.summaryConsoleText}`) - .join('`\n'), - summaryMarkdownText: scenarioResults - .map((s) => `### ${s.id}\n\n${s.summaryMarkdownText}`) - .join('`\n'), - }; + }); } /** Whether the object corresponds to a benchpress JSON report. */ function isJsonReport(data: any): data is JsonReport { - return data['completeSample'] !== undefined; + return data?.completeSample !== undefined; } diff --git a/scripts/benchmarks/targets.mts b/scripts/benchmarks/targets.mts index 9699dfa2762..3eee3c529c4 100644 --- a/scripts/benchmarks/targets.mts +++ b/scripts/benchmarks/targets.mts @@ -6,8 +6,9 @@ * found in the LICENSE file at https://angular.dev/license */ +import fs from 'fs'; import path from 'path'; -import {exec} from './utils.mts'; +import {exec, projectDir} from './utils.mts'; /** Branded string representing a resolved Bazel benchmark target. */ export type ResolvedTarget = string & { @@ -17,10 +18,11 @@ export type ResolvedTarget = string & { /** Finds all benchmark Bazel targets in the project. */ export async function findBenchmarkTargets(): Promise { return ( - await exec('bazel', [ + await exec('pnpm', [ + 'bazel', 'query', '--output=label', - `'kind("^web_test", //modules/...) intersect attr("name", "perf", //modules/...)'`, + `kind("^js_test|^web_test", //modules/...) intersect attr("name", "^perf$", //modules/...)`, ]) ) .split(/\r?\n/) @@ -28,23 +30,27 @@ export async function findBenchmarkTargets(): Promise { } /** Gets the testlog path of a given Bazel target. */ -export async function getTestlogPath(target: ResolvedTarget): Promise { - return path.join(await bazelTestlogDir(), target.substring(2).replace(':', '/')); +export async function getTestlogPath( + target: ResolvedTarget, + cwd: string = projectDir, +): Promise { + const symlinkPath = path.join(cwd, 'dist/testlogs', target.substring(2).replace(':', '/')); + if (fs.existsSync(path.join(cwd, 'dist/testlogs'))) { + return symlinkPath; + } + try { + const bazelTestlogs = ( + await exec('pnpm', ['bazel', 'info', 'bazel-testlogs', '--lockfile_mode=update'], cwd) + ).trim(); + return path.join(bazelTestlogs, target.substring(2).replace(':', '/')); + } catch (e) { + return symlinkPath; + } } /** Resolves a given benchmark Bazel target to the fully expanded label. */ export async function resolveTarget(target: string): Promise { - // If the target does not specify an explicit browser test target, we attempt - // to automatically add the Chromium suffix. This is necessary for e.g. - // resolving testlogs which would reside under the actual test target. - if (!target.endsWith('_chromium')) { - target = `${target}_chromium`; - } - - return (await exec('bazel', ['query', '--output=label', target])).trim() as ResolvedTarget; -} - -let testlogDir: string | null = null; -async function bazelTestlogDir(): Promise { - return testlogDir ?? (testlogDir = (await exec('bazel', ['info', 'bazel-testlogs'])).trim()); + return ( + await exec('pnpm', ['bazel', 'query', '--output=label', '--', target]) + ).trim() as ResolvedTarget; } diff --git a/scripts/benchmarks/utils.mts b/scripts/benchmarks/utils.mts index 97654a51534..6ef7fb6272e 100644 --- a/scripts/benchmarks/utils.mts +++ b/scripts/benchmarks/utils.mts @@ -22,16 +22,16 @@ export const projectDir: string = path.join(scriptDir, '../..'); * This ensures that special shell characters within arguments are treated as * literal values and cannot be used to inject additional commands. */ -export function exec(cmd: string, args: string[] = []): Promise { +export function exec(cmd: string, args: string[] = [], cwd: string = projectDir): Promise { return new Promise((resolve, reject) => { - Log.info('Running command:', cmd, args.join(' ')); + Log.info('Running command:', cmd, args.join(' '), `(in ${cwd})`); const proc = childProcess.spawn(cmd, args, { // Do not use a shell to spawn the process. This ensures that arguments // are passed directly to the executable without shell interpretation, // preventing injection via shell metacharacters. shell: false, - cwd: projectDir, + cwd, // Only capture `stdout`. Forward the rest to the parent TTY. stdio: ['inherit', 'pipe', 'inherit'], });