Speed up the core sim: inline sfc32 PRNG and allocation-free player updates#4233
Merged
Conversation
…pdates Two optimizations, both behavior-affecting in controlled ways and covered by new tests. Together: default perf run mean tick 7.98 -> 6.96ms (125 -> 144 ticks/sec, p99 21.2 -> 19.0ms, peak heap 438 -> 294 MB); giantworldmap 600-tick run mean 17.4 -> 15.2ms (58 -> 66 ticks/sec). 1. PseudoRandom: replace the seedrandom ARC4 engine with an inline sfc32 PRNG (splitmix32 seed expansion + warmup, 32-bit integer ops only, so sequences are identical across platforms). This removes the seedrandom dependency — src/core is now actually dependency-free — and was ~4% of profiled self time. The random stream differs, so the deterministic game-state hash changes; the harness still reproduces the same hash on repeated runs per seed. New tests/PseudoRandom.test.ts (15 tests) pins the engine contract: per-seed determinism, ranges, uniformity, adjacent-seed decorrelation, and all API behaviors. The stream change exposed that the "nation cannot attack allied player" test passed only by RNG luck: the difficulty random gate (not the alliance check) blocked the attack, hiding that the test's AiAttackBehavior was constructed without its emoji behavior. The test now supplies a NationEmojiBehavior and verifies the real protection layer (AttackExecution's alliance check) regardless of AI dice rolls. 2. PlayerImpl.toFullUpdate: empty collections now reuse shared frozen module-level singletons instead of allocating fresh arrays/Sets for every player every tick (~10 allocations per player-tick, 472 players; most collections are empty for most players). diffPlayerUpdate's existing reference fast paths make the compares O(1). Non-empty collections build in single passes. Frozen so accidental in-worker mutation throws loudly; cross-thread consumers get mutable structured clones. Value-identical: the game-state hash is unchanged by this commit's update-path changes. New tests/PlayerUpdateDiff.test.ts (8 tests) covers snapshot shape, null-when-unchanged, per-collection diffs, the tick pipeline, and the freeze contract. New reference hashes (npm run perf:game): - --map world --ticks 200 --bots 100: 5607618202213430 - default run: 29309648281599524 - --map giantworldmap --ticks 600: 39945089450032050 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughEliminate the external seedrandom dependency by implementing an internal 32-bit PRNG and optimize player snapshots to reuse frozen empty singleton collections; add tests for PRNG determinism/ranges and for player update diffing and snapshot immutability. ChangesPRNG Refactoring
Player Update Optimization
Sequence DiagramssequenceDiagram
participant Caller
participant PseudoRandom
participant InternalState
Caller->>PseudoRandom: constructor(seed)
PseudoRandom->>InternalState: splitmix32 mixing + warm-up
Caller->>PseudoRandom: next()
PseudoRandom->>InternalState: advance s0–s3 via bitwise ops
PseudoRandom->>Caller: return normalized [0,1) float
sequenceDiagram
participant Game
participant Alice
participant Bob
Game->>Alice: toUpdate() (first call)
Alice-->>Game: full snapshot (empty singletons)
Game->>Alice: state change (attack/embargo/alliance)
Alice->>Game: toUpdate() (diff with outgoing changes)
Game->>Bob: toUpdate()
Bob-->>Game: diff with incomingAttacks
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
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.
🧹 Nitpick comments (1)
src/core/game/PlayerImpl.ts (1)
69-75: ⚡ Quick winClarify the Set freezing comment.
The comment states "Sets cannot be frozen (Set.add ignores freeze)", but this is misleading.
Object.freeze()does work on Sets in strict mode (which TypeScript uses) — attempting to add to a frozen Set will throw a TypeError. The implementation is correct since it never mutatesEMPTY_EMBARGOES(creating a new Set when needed on lines 204-207), but the comment should be updated to reflect the actual behavior: frozen Sets throw on mutation rather than silently ignoring it.📝 Suggested comment revision
-// Shared singletons for empty collections in toFullUpdate. Sharing -// references lets diffPlayerUpdate's `a === b` fast paths skip structural -// comparison and avoids per-player-per-tick allocations. The arrays are -// frozen so accidental in-worker mutation throws instead of silently -// corrupting every player's updates; updates crossing to the main thread -// are structured-cloned (clones are mutable). Sets cannot be frozen -// (Set.add ignores freeze) — EMPTY_EMBARGOES must never be mutated. +// Shared singletons for empty collections in toFullUpdate. Sharing +// references lets diffPlayerUpdate's `a === b` fast paths skip structural +// comparison and avoids per-player-per-tick allocations. All singletons are +// frozen so accidental in-worker mutation throws instead of silently +// corrupting every player's updates. Updates crossing to the main thread +// are structured-cloned (clones are mutable). EMPTY_EMBARGOES is never +// mutated—new Sets are allocated when non-empty (lines 204-207).🤖 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 `@src/core/game/PlayerImpl.ts` around lines 69 - 75, The comment near the top of PlayerImpl about shared singletons is misleading about Set freezing; update the comment to state that Object.freeze does apply to Sets in strict/TypeScript mode and will cause a TypeError on mutation rather than silently ignoring additions, and note that EMPTY_EMBARGOES must never be mutated (the code path in toFullUpdate and the creation of a new Set around lines where EMPTY_EMBARGOES is used ensures immutability); keep references to toFullUpdate, EMPTY_EMBARGOES, and diffPlayerUpdate so readers can find the related logic.
🤖 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.
Nitpick comments:
In `@src/core/game/PlayerImpl.ts`:
- Around line 69-75: The comment near the top of PlayerImpl about shared
singletons is misleading about Set freezing; update the comment to state that
Object.freeze does apply to Sets in strict/TypeScript mode and will cause a
TypeError on mutation rather than silently ignoring additions, and note that
EMPTY_EMBARGOES must never be mutated (the code path in toFullUpdate and the
creation of a new Set around lines where EMPTY_EMBARGOES is used ensures
immutability); keep references to toFullUpdate, EMPTY_EMBARGOES, and
diffPlayerUpdate so readers can find the related logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0f864e9b-384d-4c90-83f6-182be8dd3cfb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.jsonsrc/core/PseudoRandom.tssrc/core/game/PlayerImpl.tstests/AiAttackBehavior.test.tstests/PlayerUpdateDiff.test.tstests/PseudoRandom.test.ts
💤 Files with no reviewable changes (1)
- package.json
github-project-automation
Bot
moved this from Triage
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.
Summary
Follow-up to #4230. Two more core-sim optimizations — these are behavior-affecting in controlled ways (unlike #4230, which was hash-identical), so both come with dedicated test coverage written before the change.
Combined results (
npm run perf:game, same machine, before → after):Cumulative with #4230 vs. the original baseline: default run mean 9.04 → 6.96 ms (111 → 144 ticks/sec); giantworldmap 22.5 → 15.2 ms (44 → 66 ticks/sec, max tick 52.8 → 40.1 ms).
1.
PseudoRandom: seedrandom ARC4 → inline sfc32seedrandomdependency entirely, makingsrc/coreactually dependency-free (the import was the only violation of that rule).--map world --ticks 200 --bots 100→560761820221343029309648281599524--map giantworldmap --ticks 600→39945089450032050tests/PseudoRandom.test.ts(15 tests) pins the engine-agnostic contract: per-seed determinism, ranges, uniformity, adjacent-seed decorrelation, and every API method. The tests were verified green against the old engine first, then the swap.AiAttackBehavior.test.ts, "nation cannot attack allied player" was actually being blocked by the difficulty dice gate inshouldAttack, not the alliance check — hiding that the test'sAiAttackBehaviorwas constructed without itsNationEmojiBehavior. The test now supplies one and verifies the real protection layer (AttackExecution's alliance check), robust to any dice outcome.2.
PlayerImpl.toFullUpdate: allocation-free empty collectionstoFullUpdateruns for every player every tick and allocated ~10 collections each (allies, embargoes Set, attacks, alliance views, …) even when all were empty — the common case for most of 472 players. BecauselastSentUpdateretains each snapshot for a full tick, these objects survived minor GC, got promoted, and accumulated as old-space garbage between major GCs — that's the peak-heap drop.diffPlayerUpdate's existinga === bfast paths skip structural comparison entirely. Non-empty collections build in single passes. Freezing makes accidental in-worker mutation throw loudly instead of silently corrupting every player; consumers across the worker boundary get mutable structured clones as before. (Setcannot be frozen —EMPTY_EMBARGOESis documented as never-mutate.)tests/PlayerUpdateDiff.test.ts(8 tests): full-snapshot shape, null-when-unchanged, embargo/alliance/target/attack diffs through the real tick pipeline, and the freeze contract.Verification
🤖 Generated with Claude Code