Add a main-thread perf harness for the worker → client update pipeline#4243
Merged
Conversation
The sim runs at 10Hz in a worker; every tick the main thread synchronously runs structuredClone deserialization, GameView.update() (tile apply + FrameData derivations), and WebGLFrameBuilder.update(). On low-end devices that burst exceeds the 16.7ms frame budget and causes a visible stutter every 100ms. npm run perf:client measures the burst headlessly per stage (clone / view / builder), reports payload sizes, writes a .cpuprofile, and prints a deterministic View hash so client-side optimizations can be verified to not change view behavior. WebGLFrameBuilder's import of ./render/gl is made type-only so the module loads under Node (a value import pulls GPURenderer and its .glsl?raw shader imports); no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
WalkthroughThis pull request adds a comprehensive Node-based performance harness for measuring the client-side game update pipeline. It includes a localStorage shim for Node, a WebGL stub that tracks GPU-layer calls without rendering, precise timing measurement across clone/view/builder stages, optional CPU profiling, and deterministic view state hashing to validate optimizations. ChangesClient Performance Harness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed due to a network error. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/perf/client/ClientUpdatePerf.ts`:
- Around line 374-394: The test collects payload/GL metrics (stats,
tilePairsByTick, totalTilePairs, maxTilePairs, unitUpdatesTotal,
playerUpdatesTotal and the glStub/WebGLFrameBuilder usage) before the spawn loop
so the "Update payload (game phase)" report includes spawn-phase traffic; to
fix, introduce a second set of counters (e.g. gameStats, gameTilePairsByTick,
gameTotalTilePairs, gameMaxTilePairs, gameUnitUpdatesTotal,
gamePlayerUpdatesTotal) or snapshot/reset the existing counters immediately
after the spawn loop and use those snapshots for the game-phase report, keeping
the original counters if you still want overall totals—apply the same change
where similar reports are generated around the other ranges mentioned.
- Around line 118-135: The CLI parsing accepts invalid numeric input silently;
update the handlers for the "--ticks", "--bots", "--nations" (when not
"default"/"disabled"), and "--top" cases to validate parseInt(next(), 10)
results: after parsing, check for Number.isInteger(value) and that value >= 0
(or >0 if zero is invalid), and if validation fails throw an informative Error
or exit with a nonzero code so the process fails fast; refer to the case labels
"--ticks", "--bots", "--nations", and "--top" and the
opts.{ticks,bots,nations,top} assignments to locate where to add these checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0900901b-1711-4862-8f5a-4056762a4f74
📒 Files selected for processing (4)
package.jsonsrc/client/WebGLFrameBuilder.tstests/perf/client/ClientUpdatePerf.tstests/perf/client/Shims.ts
Comment on lines
+118
to
+135
| case "--ticks": | ||
| opts.ticks = parseInt(next(), 10); | ||
| break; | ||
| case "--bots": | ||
| opts.bots = parseInt(next(), 10); | ||
| break; | ||
| case "--nations": { | ||
| const v = next(); | ||
| opts.nations = | ||
| v === "default" || v === "disabled" ? v : parseInt(v, 10); | ||
| break; | ||
| } | ||
| case "--seed": | ||
| opts.seed = next(); | ||
| break; | ||
| case "--top": | ||
| opts.top = parseInt(next(), 10); | ||
| break; |
Contributor
There was a problem hiding this comment.
Reject invalid numeric CLI values early.
parseInt() here accepts bad input too quietly. For example, --ticks nope becomes NaN and the main loop runs zero iterations, while negative values also slip through. That makes the harness print misleading results instead of failing fast.
Suggested fix
+function parsePositiveInt(flag: string, raw: string): number {
+ const value = Number(raw);
+ if (!Number.isInteger(value) || value < 0) {
+ throw new Error(`invalid value for ${flag}: ${raw}`);
+ }
+ return value;
+}
+
function parseArgs(argv: string[]): Options {
const opts: Options = {
map: GameMapType.World,
@@
case "--ticks":
- opts.ticks = parseInt(next(), 10);
+ opts.ticks = parsePositiveInt("--ticks", next());
break;
case "--bots":
- opts.bots = parseInt(next(), 10);
+ opts.bots = parsePositiveInt("--bots", next());
break;
case "--nations": {
const v = next();
opts.nations =
- v === "default" || v === "disabled" ? v : parseInt(v, 10);
+ v === "default" || v === "disabled"
+ ? v
+ : parsePositiveInt("--nations", v);
break;
}
@@
case "--top":
- opts.top = parseInt(next(), 10);
+ opts.top = parsePositiveInt("--top", next());
break;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "--ticks": | |
| opts.ticks = parseInt(next(), 10); | |
| break; | |
| case "--bots": | |
| opts.bots = parseInt(next(), 10); | |
| break; | |
| case "--nations": { | |
| const v = next(); | |
| opts.nations = | |
| v === "default" || v === "disabled" ? v : parseInt(v, 10); | |
| break; | |
| } | |
| case "--seed": | |
| opts.seed = next(); | |
| break; | |
| case "--top": | |
| opts.top = parseInt(next(), 10); | |
| break; | |
| function parsePositiveInt(flag: string, raw: string): number { | |
| const value = Number(raw); | |
| if (!Number.isInteger(value) || value < 0) { | |
| throw new Error(`invalid value for ${flag}: ${raw}`); | |
| } | |
| return value; | |
| } | |
| case "--ticks": | |
| opts.ticks = parsePositiveInt("--ticks", next()); | |
| break; | |
| case "--bots": | |
| opts.bots = parsePositiveInt("--bots", next()); | |
| break; | |
| case "--nations": { | |
| const v = next(); | |
| opts.nations = | |
| v === "default" || v === "disabled" | |
| ? v | |
| : parsePositiveInt("--nations", v); | |
| break; | |
| } | |
| case "--seed": | |
| opts.seed = next(); | |
| break; | |
| case "--top": | |
| opts.top = parsePositiveInt("--top", next()); | |
| break; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/perf/client/ClientUpdatePerf.ts` around lines 118 - 135, The CLI
parsing accepts invalid numeric input silently; update the handlers for the
"--ticks", "--bots", "--nations" (when not "default"/"disabled"), and "--top"
cases to validate parseInt(next(), 10) results: after parsing, check for
Number.isInteger(value) and that value >= 0 (or >0 if zero is invalid), and if
validation fails throw an informative Error or exit with a nonzero code so the
process fails fast; refer to the case labels "--ticks", "--bots", "--nations",
and "--top" and the opts.{ticks,bots,nations,top} assignments to locate where to
add these checks.
Comment on lines
+374
to
+394
| const glStub = createGlStub(); | ||
| const builder = new WebGLFrameBuilder( | ||
| glStub.view as unknown as ConstructorParameters< | ||
| typeof WebGLFrameBuilder | ||
| >[0], | ||
| ); | ||
|
|
||
| // Per-stage stats. "total" is clone+view+builder — the main-thread burst. | ||
| const stats = { | ||
| sim: new TickStats(), | ||
| clone: new TickStats(), | ||
| view: new TickStats(), | ||
| builder: new TickStats(), | ||
| total: new TickStats(), | ||
| }; | ||
| const tilePairsByTick = new Map<number, number>(); | ||
| let totalTilePairs = 0; | ||
| let maxTilePairs = 0; | ||
| let unitUpdatesTotal = 0; | ||
| let playerUpdatesTotal = 0; | ||
|
|
Contributor
There was a problem hiding this comment.
Keep spawn metrics out of the “game phase” payload report.
The payload counters and GL stub start collecting before the spawn loop, but the final section says Update payload (game phase). Right now those numbers include spawn-phase traffic too, so the reported tile/unit/player/GPU totals are overstated for the phase this harness says it is measuring.
Suggested fix
const spawnTurns = turnNumber;
const spawnClientMs = spawnStats.total.summarize(FRAME_BUDGET_MS).totalMs;
console.log(
`Spawn phase done: ${spawnTurns} turns in ` +
`${fmtMs(performance.now() - spawnStart)}ms wall ` +
`(${fmtMs(spawnClientMs)}ms client-side), ` +
`${runner.game.players().filter((p) => p.isAlive()).length} players spawned.`,
);
// Main game phase, under the CPU profiler.
+ tilePairsByTick.clear();
+ totalTilePairs = 0;
+ maxTilePairs = 0;
+ unitUpdatesTotal = 0;
+ playerUpdatesTotal = 0;
+ const glStub = createGlStub();
+ const builder = new WebGLFrameBuilder(
+ glStub.view as unknown as ConstructorParameters<
+ typeof WebGLFrameBuilder
+ >[0],
+ );
+
const cpuProfiler = opts.cpuProfile ? new CpuProfiler() : null;If you want spawn + game totals too, keep a second set of counters instead of reusing the game-phase ones.
Also applies to: 396-420, 447-471, 567-586
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/perf/client/ClientUpdatePerf.ts` around lines 374 - 394, The test
collects payload/GL metrics (stats, tilePairsByTick, totalTilePairs,
maxTilePairs, unitUpdatesTotal, playerUpdatesTotal and the
glStub/WebGLFrameBuilder usage) before the spawn loop so the "Update payload
(game phase)" report includes spawn-phase traffic; to fix, introduce a second
set of counters (e.g. gameStats, gameTilePairsByTick, gameTotalTilePairs,
gameMaxTilePairs, gameUnitUpdatesTotal, gamePlayerUpdatesTotal) or
snapshot/reset the existing counters immediately after the spawn loop and use
those snapshots for the game-phase report, keeping the original counters if you
still want overall totals—apply the same change where similar reports are
generated around the other ranges mentioned.
github-project-automation
Bot
moved this from Triage
to Development
in OpenFront Release Management
github-project-automation
Bot
moved this from Development
to Complete
in OpenFront Release Management
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
npm run perf:client— a headless harness (companion tonpm run perf:gamefrom #4228) that measures the main-thread burst the client runs every simulation tick. The sim ticks at 10Hz in a worker; each tick the main thread synchronously runs deserialization →GameView.update()→WebGLFrameBuilder.update()→ HUD ticks. On low-end devices that burst exceeds the 16.7ms frame budget and shows up as a stutter every 100ms. Before optimizing that path, this gives us numbers.Per tick it runs the real pipeline end to end and times three stages:
structuredCloneof theGameUpdateViewDatawith the same transfer listWorker.worker.tsuses (serialize+deserialize, an upper bound on the main-thread share of the realpostMessage)GameView.update(), including allpopulateFrame()derivationsWebGLFrameBuilder.update()against a no-op GL stub that counts payload sizesIt reports mean/p50/p95/p99/max per stage, slowest bursts with their tile counts, payload stats, a filtered V8 CPU profile table, and writes a
.cpuprofile. Not covered (browser-only): CPU inside the WebGL view'supdate*()methods and HUD layer ticks.Same flags as
perf:game:--map --ticks --bots --nations --seed --top --no-cpu-profile.Determinism
perf:gamereferences on all three standard configs (world/200t/100b →5607618202213430, default →29309648281599524, giantworldmap/600t →39945089450032050) — the harness's worker side is faithful.Baseline (this machine; low-end devices are ~5–20× slower)
Default run (world, 400 bots, 1800 ticks):
giantworldmap/600t: TOTAL mean 2.54ms, p99 5.65ms, max 6.42ms.
Notable: the clone is the largest stage (~60%) — the packed tile/motion buffers transfer for free, so the cost is structured-cloning the
updatesobject (~278 partial player updates/tick on world, ~508 on giantworldmap). Insideview, the recurring cost ispopulateFrame's derivations (computePlayerStatus, the O(players²) relation matrix, alliance clusters); tile apply dominates the land-grab spikes.Code changes outside the harness
WebGLFrameBuilder: the./render/glimport is nowimport typeso the module loads under Node — a value import pullsGPURendererand its.glsl?rawshader imports. No behavior change (the symbols were only used in type positions).tests/perf/client/Shims.ts: an in-memorylocalStorageshim soUserSettings/theme code runs under Node (all settings resolve to defaults, which is also the deterministic choice).Verification
npm test(1474 tests),eslint,prettier --check,tsc --noEmitall pass.🤖 Generated with Claude Code