Add a reusable Graphile performance harness - #1716
Conversation
2f912bd to
45cbb07
Compare
45cbb07 to
658736e
Compare
|
Review complete. 🟡 1 medium 💬 Inline comments (1)
🧹 Nitpicks (1) — 🟢 1 low
This PR introduces
Reviewed commit: 658736e |
658736e to
f7c63ec
Compare
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a new packages/perf-harness package: a performance benchmarking harness that spawns worker processes against a seeded PostgreSQL schema and reports timing/memory metrics, wired into CI.
Key findings
- 🟡 Add a timeout to
runWorkerProcess— process.ts:70
| export const runWorkerProcess = ( | ||
| workerPath: string, | ||
| databaseUrl: string, | ||
| definition: BenchmarkCaseDefinition | ||
| ): Promise<SpawnedWorkerResult> => | ||
| new Promise((resolve, reject) => { | ||
| const config: WorkerConfigEnvelope = { | ||
| caseName: definition.name, | ||
| workerConfig: definition.workerConfig, | ||
| }; | ||
| const child = spawn( | ||
| process.execPath, | ||
| [ | ||
| '--expose-gc', | ||
| workerPath, | ||
| `--${DATABASE_URL_ARGUMENT}`, | ||
| databaseUrl, | ||
| `--${WORKER_CONFIG_ARGUMENT}`, | ||
| Buffer.from(JSON.stringify(config)).toString('base64url'), | ||
| ], | ||
| { | ||
| env: { | ||
| ...process.env, | ||
| NODE_ENV: 'production', | ||
| GRAPHILE_ENV: 'production', | ||
| }, | ||
| stdio: ['ignore', 'pipe', 'pipe'], | ||
| } | ||
| ); | ||
| const pid = child.pid; | ||
| let stdout = ''; | ||
| let stderr = ''; | ||
| child.stdout.setEncoding('utf8'); | ||
| child.stderr.setEncoding('utf8'); | ||
| child.stdout.on('data', (chunk: string) => { | ||
| stdout += chunk; | ||
| }); | ||
| child.stderr.on('data', (chunk: string) => { | ||
| stderr += chunk; | ||
| }); | ||
| child.once('error', reject); | ||
| child.once('close', (code, signal) => { | ||
| const resultLine = stdout | ||
| .split('\n') | ||
| .reverse() | ||
| .find((line) => line.startsWith(WORKER_RESULT_PREFIX)); | ||
| if (!resultLine) { | ||
| reject( | ||
| new Error( | ||
| redactSecret( | ||
| `benchmark worker ${pid ?? 'unknown'} exited without a result ` + | ||
| `(code=${String(code)}, signal=${String(signal)})` + | ||
| (stderr.trim() ? `\n${lastLines(stderr)}` : ''), | ||
| databaseUrl | ||
| ) | ||
| ) | ||
| ); | ||
| return; | ||
| } | ||
| try { | ||
| const result = JSON.parse( | ||
| resultLine.slice(WORKER_RESULT_PREFIX.length) | ||
| ) as WorkerResult; | ||
| if (typeof pid !== 'number' || result.pid !== pid) { | ||
| throw new Error( | ||
| `worker PID mismatch: spawned ${String(pid)}, reported ${String( | ||
| result.pid | ||
| )}` | ||
| ); | ||
| } | ||
| if (result.caseName !== definition.name) { | ||
| throw new Error( | ||
| `worker case mismatch: expected ${definition.name}, reported ${result.caseName}` | ||
| ); | ||
| } | ||
| if (result.status === 'ok' && code !== 0) { | ||
| throw new Error(`successful worker exited with code ${String(code)}`); | ||
| } | ||
| resolve({ pid, result }); | ||
| } catch (error) { | ||
| reject( | ||
| new Error( | ||
| redactSecret( | ||
| `invalid result from benchmark worker ${String(pid)}: ${String( | ||
| error instanceof Error ? error.message : error | ||
| )}`, | ||
| databaseUrl | ||
| ) | ||
| ) | ||
| ); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🟡 bug · medium
Add a timeout to runWorkerProcess
runWorkerProcess (packages/perf-harness/src/process.ts:70-162) spawns a child and only settles its promise on the child's error or close events, with no timeout or child.kill() path. A worker that hangs (an infinite schema-build loop or a stalled DB connection) leaves the promise pending forever, and runBenchmarkSuite awaits each worker sequentially, so the entire cperf run hangs with no report written and no way to recover.
📋 Prompt for AI Agents
In packages/perf-harness/src/process.ts, runWorkerProcess (lines 70-162): add a configurable timeout that races the spawned child. On expiry call child.kill() (e.g. SIGKILL) and reject the promise with a clear, redacted 'worker timed out' error; clear the timer in both the error and close handlers so a normal completion does not reject after the fact. This prevents a hung worker from leaving runBenchmarkSuite pending forever.
2 participants
Responsibility
Adds reusable fresh-process Graphile benchmarking infrastructure. The runner accepts arbitrary serializable case definitions and delegates case-specific build and lifecycle validation to a dedicated worker entry.
Included here:
--expose-gcwith deterministic GC calls--database-urland--worker-configworker argumentsStack
This is the first PR in the new performance stack and targets
main. Later stacked PRs can add their own benchmark suites without changing the core runner.Not included
No product-specific benchmark suite, application preset, application plugin integration, dependency patch, or optimization conclusion is included.
Validation
3477f9371996: 17/17 jobs passed (Linux, Windows, PostgreSQL, MinIO integration, and Ollama).git diff --check: passed