Skip to content

Commit abae1c0

Browse files
CopilotNullVoxPopuli
authored andcommitted
fix: make perf benchmark script more reliable
- Fix lsof() bug: .toString (property ref) -> .toString() (method call) and wrap in try/catch for non-zero exit codes - Replace fixed sleep(5000) + broken lsof check with waitForServer() that polls with HTTP fetch until servers actually respond - Store startVitePreview child processes and add .catch() error handlers Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/abc30b6b-e3d1-44c5-988e-648096650173 chore: remove unnecessary comments from benchmark scripts Co-authored-by: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Agent-Logs-Url: https://github.com/emberjs/ember.js/sessions/b3fd4afb-3714-4a3d-b7d6-acb56ba1a9dc fix: default to non-headless on macOS to fix bench trace capture Chrome's --headless=new mode on macOS (Chrome 147+) silently fails to capture performance.mark() events in traces. Tracerbench then fails immediately with 'Could not find mark renderStart in trace'. Non-headless mode (visible browser windows) works correctly on macOS. Default to non-headless on macOS, keep headless on Linux for CI. Users can still force headless with --headless, or force non-headless with --no-headless (now a no-op on macOS since it's already the default). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix: make headless work on macOS via tracerbench patches Root cause: Chrome 147 (headless=new) on macOS emits an initial blank navigationStart (documentLoaderURL: '') before the real page navigation. Tracerbench's findPhases() locked onto the wrong navigationId, so all performance.mark() events failed the ID check and 'renderStart' was never found in the trace. Fix in extract-navigation-sample.js: when a second navigationStart with a non-empty documentLoaderURL is encountered, update navigationStartArgs so subsequent markers match the correct navigationId. Additional improvements to the tracerbench patch: - inject-mark-observer.js: remove self===top guard and add buffered:true to PerformanceObserver so pre-existing marks are captured reliably - run-trace.js: use traceConfig.includedCategories (modern CDP API) with recordAsMuchAsPossible to avoid trace buffer overflow Also reverts the earlier macOS non-headless workaround in benchmark.mjs — headless mode now works on macOS too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> fix: correct hunk line count in tracerbench patch After manually editing the run-trace.js comment from 3 lines to 2, the @@ hunk header still said +10,13 instead of +10,12, breaking pnpm install with 'hunk header integrity check failed'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Add lockfile
1 parent 0531e62 commit abae1c0

File tree

5 files changed

+108
-23
lines changed

5 files changed

+108
-23
lines changed

bin/benchmark.mjs

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@ Options:
1717
--force delete cached directories before running
1818
--reuse reuse existing apps and tarballs, if available (by default only the control app/tarball is reused)
1919
--no-headless run Chrome without headless mode (opens visible browser windows)
20-
On macOS, benchmarks will run sequentially (one browser at a time)
2120
2221
Notes:
2322
- This script runs \`pnpm install\` and \`node ./bin/build-for-publishing.js\` in both repos.

bin/benchmark/run.mjs

Lines changed: 16 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import fs from 'fs-extra';
66

77
import { getOrBuildControlTarball } from './control.mjs';
88
import { buildExperimentTarball } from './experiment.mjs';
9-
import { run, prepareApp, sleep, startVitePreview, lsof } from './utils.mjs';
9+
import { run, prepareApp, startVitePreview, waitForServer } from './utils.mjs';
1010

1111
const { ensureDir, remove } = fs;
1212

@@ -112,12 +112,19 @@ export async function runBenchmark({ force = false, reuse = false, headless = tr
112112
]);
113113

114114
// These will error if the parts are occupied (--strict-port)
115-
startVitePreview({ appDir: CONTROL_DIRS.app, port: DEFAULT_CONTROL_PORT });
116-
startVitePreview({
115+
const controlServer = startVitePreview({ appDir: CONTROL_DIRS.app, port: DEFAULT_CONTROL_PORT });
116+
const experimentServer = startVitePreview({
117117
appDir: EXPERIMENT_DIRS.app,
118118
port: DEFAULT_EXPERIMENT_PORT,
119119
});
120120

121+
controlServer.catch((err) => {
122+
console.error('Control server exited unexpectedly:', err.message);
123+
});
124+
experimentServer.catch((err) => {
125+
console.error('Experiment server exited unexpectedly:', err.message);
126+
});
127+
121128
try {
122129
await bootAndRun({ headless });
123130
} finally {
@@ -132,20 +139,12 @@ async function bootAndRun({ headless = true } = {}) {
132139
const experimentUrl = `http://127.0.0.1:${DEFAULT_EXPERIMENT_PORT}`;
133140
const markersString = buildMarkersString(DEFAULT_MARKERS);
134141

135-
// give servers a moment to start
136-
await sleep(5000);
137-
138-
/**
139-
* We need to make sure both servers are running before starting the benchmark.
140-
*/
141-
let controlLsof = await lsof(DEFAULT_CONTROL_PORT);
142-
let experimentLsof = await lsof(DEFAULT_EXPERIMENT_PORT);
143-
144-
if (!controlLsof || !experimentLsof) {
145-
throw new Error(
146-
`One of the servers failed to start. Control server lsof:\n${controlLsof}\n\nExperiment server lsof:\n${experimentLsof}`
147-
);
148-
}
142+
console.log('\n\tWaiting for servers to be ready...');
143+
await Promise.all([
144+
waitForServer(controlUrl, { timeout: 30_000 }),
145+
waitForServer(experimentUrl, { timeout: 30_000 }),
146+
]);
147+
console.log('\tBoth servers are ready.\n');
149148

150149
const tracerbenchBin = join(REPO_ROOT, 'node_modules/tracerbench/bin/run');
151150

bin/benchmark/utils.mjs

Lines changed: 31 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,37 @@ export function sleep(ms) {
2323
return new Promise((resolve) => setTimeout(resolve, ms));
2424
}
2525

26-
export async function lsof(port) {
27-
return execSync(`lsof -i :${port} -P -n`).toString;
26+
export function lsof(port) {
27+
try {
28+
return execSync(`lsof -i :${port} -P -n`).toString();
29+
} catch {
30+
return null;
31+
}
32+
}
33+
34+
/**
35+
* Wait for a server to respond to HTTP requests.
36+
* Polls with fetch until the server returns a response (any status),
37+
* retrying on connection errors.
38+
*
39+
* @param {string} url - The URL to poll
40+
* @param {object} [options]
41+
* @param {number} [options.timeout=30000] - Max time to wait in ms
42+
* @param {number} [options.interval=500] - Time between retries in ms
43+
*/
44+
export async function waitForServer(url, { timeout = 30_000, interval = 500 } = {}) {
45+
const deadline = Date.now() + timeout;
46+
47+
while (Date.now() < deadline) {
48+
try {
49+
await fetch(url);
50+
return;
51+
} catch {
52+
await sleep(interval);
53+
}
54+
}
55+
56+
throw new Error(`Server at ${url} did not respond within ${timeout}ms`);
2857
}
2958

3059
export async function buildEmberSource(cwd) {

patches/@tracerbench__core@8.0.1.patch

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,3 +58,61 @@ index 5918e8f7665b3e796ef88283fc40c2b3286a564f..b9927ba8f639c0a9ecc70144362da439
5858
}
5959
return categories;
6060
}
61+
diff --git a/dist/metrics/extract-navigation-sample.js b/dist/metrics/extract-navigation-sample.js
62+
index 58a0330d018d00223d9cfe0cc353ee3903837df1..fa75690cd7c0f951a9106a9926caa288f6110143 100644
63+
--- a/dist/metrics/extract-navigation-sample.js
64+
+++ b/dist/metrics/extract-navigation-sample.js
65+
@@ -40,6 +40,13 @@ function findPhases(events, markers) {
66+
}
67+
}
68+
else {
69+
+ // In headless Chrome on macOS, an initial navigationStart with an empty
70+
+ // documentLoaderURL may precede the real page navigation. Update
71+
+ // navigationStartArgs if we encounter a navigationStart with a real URL
72+
+ // so subsequent markers match the correct navigationId.
73+
+ if (event.name === 'navigationStart' && event.args?.data?.documentLoaderURL) {
74+
+ navigationStartArgs = event.args;
75+
+ }
76+
if (event.name === marker.start) {
77+
const { args } = event;
78+
// ensure this belongs to this navigation
79+
diff --git a/dist/util/inject-mark-observer.js b/dist/util/inject-mark-observer.js
80+
index 0444c9911387202e156d3901e16e1ff092399f75..f7385765297aff2ff6ece7c6164dd1d3de2b0047 100644
81+
--- a/dist/util/inject-mark-observer.js
82+
+++ b/dist/util/inject-mark-observer.js
83+
@@ -54,8 +54,6 @@ function lcpObserver(variable, priorMarker) {
84+
function markObserver(mark, variable) {
85+
return `"use strict";
86+
var ${variable} =
87+
- self === top &&
88+
- opener === null &&
89+
new Promise((resolve) =>
90+
new PerformanceObserver((records, observer) => {
91+
if (records.getEntriesByName(${JSON.stringify(mark)}).length > 0) {
92+
@@ -66,7 +64,7 @@ function markObserver(mark, variable) {
93+
});
94+
observer.disconnect();
95+
}
96+
- }).observe({ type: 'mark' })
97+
+ }).observe({ type: 'mark', buffered: true })
98+
);`;
99+
}
100+
function navigationObserver(variable) {
101+
diff --git a/dist/util/run-trace.js b/dist/util/run-trace.js
102+
index 0f49cd51cf789117dd3ad9aea8754293f50a51ff..e428000306cc6553d094441ad5e17660962e06d6 100644
103+
--- a/dist/util/run-trace.js
104+
+++ b/dist/util/run-trace.js
105+
@@ -10,7 +10,12 @@ async function runTrace(page, categories, raceCancellation, usingTracing, path)
106+
page.on('Tracing.tracingComplete', complete);
107+
try {
108+
await performTrace(page, {
109+
- categories: categories.join(','),
110+
+ // Use traceConfig instead of deprecated categories string; use
111+
+ // recordAsMuchAsPossible to avoid dropping events on ring-buffer overflow.
112+
+ traceConfig: {
113+
+ recordMode: 'recordAsMuchAsPossible',
114+
+ includedCategories: categories,
115+
+ },
116+
transferMode: 'ReturnAsStream'
117+
}, usingTracing, (0, race_cancellation_1.combineRaceCancellation)(raceEarlyComplete, raceCancellation));
118+
stream = await waitForTraceStream(completed, raceCancellation);

pnpm-lock.yaml

Lines changed: 3 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)