From ee8c28331ba955783ad322c4f8d10cbf1610efe8 Mon Sep 17 00:00:00 2001 From: Evan Date: Fri, 5 Jun 2026 17:34:46 -0700 Subject: [PATCH] Perf: Maintain a per-player alliance list (#4172) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Maintain a per-player alliance list (drop O(all-alliances) scan) ## Summary `PlayerImpl.alliances()` was implemented as a full scan of the global alliance list on every call: ```ts alliances(): MutableAlliance[] { return this.mg.alliances_.filter( (a) => a.requestor() === this || a.recipient() === this, ); } ``` This is O(all-alliances-in-game) **per call**, and it's called a lot — most notably twice per player per tick from `PlayerImpl.toFullUpdate()` (once for `allies`, once for `alliances`), which runs for every player every tick on the worker/core thread. This PR makes each player own its alliance list: a per-player `_alliances` array (mirroring the existing `_incomingAttacks` / `_outgoingAttacks` pattern), maintained incrementally as alliances form/break/expire, so `alliances()` becomes an O(1) field read. It turned out the global `mg.alliances_` list was only ever read by this scan — the `Game`-level `alliances()` getter had **zero callers** (all 17 `.alliances()` callsites use the player-level accessor), and the list isn't used in serialization. So rather than keep two structures in sync, this removes the global list entirely and makes the per-player lists the single source of truth. ## Motivation Profiling the worker/core thread showed `player.toFullUpdate` at ~**4% of CPU**. Breaking down where that time goes (microbenchmark, 100 players, ~100 alliances): | Component | µs/tick | Share | | --- | --- | --- | | FULL (current: alliance scan ×2 + allocate collections) | 61.5 | 100% | | Alliance scan only (the two global `.filter()`s) | 41.7 | **~68%** | | Allocation only (build arrays/objects, per-player list, no scan) | 6.4 | ~10% | The global alliance scan — not the object allocation — is the dominant cost, and it gets *worse* with game size: the scan is O(players × total-alliances) while allocation is only O(players × own-alliances). Removing the scan targets the dominant ~2/3 of `toFullUpdate`'s cost. It also speeds up `alliances()` everywhere, not just `toFullUpdate` — it's called in **17 places**, including AI hot paths (`NationAllianceBehavior`, `PlayerExecution`). > Note: this builds on the already-merged `diffPlayerUpdate` typed-comparison > change (commit `be87c76`), which addressed the diff/serialization cost. This PR > addresses the snapshot-construction cost. ## Changes - **`PlayerImpl`**: add `public _alliances: MutableAlliance[]`; `alliances()` returns it directly. - **`GameImpl`**: remove the global `alliances_` field and the unused `alliances()` getter. Maintain the per-player lists at the mutation sites: - **add** — `acceptAllianceRequest` pushes the new alliance onto both participants. - **remove** — `breakAlliance`, `expireAlliance`, and `removeAlliancesByPlayerSilently` all funnel through a small `detachAlliance()` helper that removes the alliance from both participants. - **`Game` interface**: drop `alliances(): MutableAlliance[]` (no callers). ## Correctness notes - `alliances()` now returns the internal array by reference. This matches the existing `outgoingAttacks()` / `incomingAttacks()` accessors, which already do the same. All 17 callsites were checked — none mutate the returned array. - `detachAlliance` reassigns the array (`filter`) rather than splicing in place, so the `for (const alliance of player.alliances())` loop in `PlayerExecution` (which can expire alliances mid-iteration) iterates a stable snapshot and is safe. `removeAlliancesByPlayerSilently` likewise snapshots the player's list before detaching. ## Tests New `tests/PlayerAllianceList.test.ts` asserts both participants' lists stay in sync through every mutation path: - forming an alliance adds it to both lists - `alliances()` agrees with `isAlliedWith` / `allianceWith` - breaking removes it from both lists - expiring removes it from both lists - a player tracks multiple alliances independently (breaking one keeps the other) - `removeAllAlliances` clears the player and every partner Full suite green: **1360 tests / 120 files**. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- src/core/game/Game.ts | 1 - src/core/game/GameImpl.ts | 26 ++++--- src/core/game/PlayerImpl.ts | 6 +- tests/PlayerAllianceList.test.ts | 130 +++++++++++++++++++++++++++++++ 4 files changed, 148 insertions(+), 15 deletions(-) create mode 100644 tests/PlayerAllianceList.test.ts diff --git a/src/core/game/Game.ts b/src/core/game/Game.ts index 63b71fe8c..e6d5c710b 100644 --- a/src/core/game/Game.ts +++ b/src/core/game/Game.ts @@ -890,7 +890,6 @@ export interface Game extends GameMap { teamSpawnArea(team: Team): SpawnArea | undefined; // Alliances - alliances(): MutableAlliance[]; expireAlliance(alliance: Alliance): void; // Immunity timer diff --git a/src/core/game/GameImpl.ts b/src/core/game/GameImpl.ts index 465996049..6670d8584 100644 --- a/src/core/game/GameImpl.ts +++ b/src/core/game/GameImpl.ts @@ -89,7 +89,6 @@ export class GameImpl implements Game { _terraNullius: TerraNulliusImpl; allianceRequests: AllianceRequestImpl[] = []; - alliances_: AllianceImpl[] = []; private nextPlayerID = 1; private _nextUnitID = 1; @@ -227,10 +226,6 @@ export class GameImpl implements Game { return this.playerBySmallID(this.ownerID(ref)); } - alliances(): MutableAlliance[] { - return this.alliances_; - } - playerBySmallID(id: number): Player | TerraNullius { if (id === 0) { return this.terraNullius(); @@ -367,7 +362,8 @@ export class GameImpl implements Game { this._ticks, this.nextAllianceID++, ); - this.alliances_.push(alliance); + (alliance.requestor() as PlayerImpl)._alliances.push(alliance); + (alliance.recipient() as PlayerImpl)._alliances.push(alliance); (request.requestor() as PlayerImpl).pastOutgoingAllianceRequests.push( request, ); @@ -794,7 +790,7 @@ export class GameImpl implements Game { breaker.markTraitor(); } - this.alliances_ = this.alliances_.filter((a) => a !== alliance); + this.detachAlliance(alliance); this.addUpdate({ type: GameUpdateType.BrokeAlliance, @@ -815,7 +811,7 @@ export class GameImpl implements Game { `cannot expire alliance: must have exactly one alliance, have ${alliances.length}`, ); } - this.alliances_ = this.alliances_.filter((a) => a !== alliances[0]); + this.detachAlliance(alliances[0]); this.addUpdate({ type: GameUpdateType.AllianceExpired, player1ID: alliance.requestor().smallID(), @@ -824,9 +820,17 @@ export class GameImpl implements Game { } public removeAlliancesByPlayerSilently(player: Player): void { - this.alliances_ = this.alliances_.filter( - (a) => a.requestor() !== player && a.recipient() !== player, - ); + // Snapshot — detachAlliance reassigns the player's _alliances as it goes. + const removed = [...(player as PlayerImpl)._alliances]; + for (const alliance of removed) this.detachAlliance(alliance); + } + + /** Remove an alliance from both participants' per-player alliance lists. */ + private detachAlliance(alliance: Alliance): void { + const requestor = alliance.requestor() as PlayerImpl; + const recipient = alliance.recipient() as PlayerImpl; + requestor._alliances = requestor._alliances.filter((a) => a !== alliance); + recipient._alliances = recipient._alliances.filter((a) => a !== alliance); } public isSpawnImmunityActive(): boolean { diff --git a/src/core/game/PlayerImpl.ts b/src/core/game/PlayerImpl.ts index 5ac30a257..44ba4efb6 100644 --- a/src/core/game/PlayerImpl.ts +++ b/src/core/game/PlayerImpl.ts @@ -102,6 +102,8 @@ export class PlayerImpl implements Player { public _outgoingAttacks: Attack[] = []; public _outgoingLandAttacks: Attack[] = []; + public _alliances: MutableAlliance[] = []; + private _spawnTile: TileRef | undefined; private _isDisconnected = false; @@ -487,9 +489,7 @@ export class PlayerImpl implements Player { } alliances(): MutableAlliance[] { - return this.mg.alliances_.filter( - (a) => a.requestor() === this || a.recipient() === this, - ); + return this._alliances; } expiredAlliances(): Alliance[] { diff --git a/tests/PlayerAllianceList.test.ts b/tests/PlayerAllianceList.test.ts new file mode 100644 index 000000000..fc1166f25 --- /dev/null +++ b/tests/PlayerAllianceList.test.ts @@ -0,0 +1,130 @@ +import { AllianceRequestExecution } from "../src/core/execution/alliance/AllianceRequestExecution"; +import { BreakAllianceExecution } from "../src/core/execution/alliance/BreakAllianceExecution"; +import { Game, Player, PlayerType } from "../src/core/game/Game"; +import { playerInfo, setup } from "./util/Setup"; + +/** + * Tests for the per-player alliance list maintained on PlayerImpl + * (player.alliances()). It is updated incrementally as alliances form, break, + * and expire instead of scanning the global mg.alliances_ list, so the key + * invariant is that both participants' lists stay in sync with reality. + */ + +let game: Game; +let player1: Player; +let player2: Player; +let player3: Player; + +/** Form a mutual alliance via counter-requests, then tick to apply. */ +function ally(a: Player, b: Player): void { + game.addExecution(new AllianceRequestExecution(a, b.id())); + game.executeNextTick(); + game.addExecution(new AllianceRequestExecution(b, a.id())); + game.executeNextTick(); +} + +/** + * Break an alliance. Executions are inited on the tick they're added and only + * run on the following tick, so tick twice. + */ +function breakAlliance(a: Player, b: Player): void { + game.addExecution(new BreakAllianceExecution(a, b.id())); + game.executeNextTick(); + game.executeNextTick(); +} + +describe("per-player alliance list", () => { + beforeEach(async () => { + game = await setup( + "plains", + { infiniteGold: true, instantBuild: true, infiniteTroops: true }, + [ + playerInfo("player1", PlayerType.Human), + playerInfo("player2", PlayerType.Human), + playerInfo("player3", PlayerType.Human), + ], + ); + + player1 = game.player("player1"); + player1.conquer(game.ref(0, 0)); + player2 = game.player("player2"); + player2.conquer(game.ref(0, 1)); + player3 = game.player("player3"); + player3.conquer(game.ref(0, 2)); + }); + + test("forming an alliance adds it to both participants' lists", () => { + expect(player1.alliances()).toHaveLength(0); + expect(player2.alliances()).toHaveLength(0); + + ally(player1, player2); + + expect(player1.alliances()).toHaveLength(1); + expect(player2.alliances()).toHaveLength(1); + // Same underlying alliance object on both sides. + expect(player1.alliances()[0]).toBe(player2.alliances()[0]); + expect(player1.alliances()[0].other(player1)).toBe(player2); + expect(player2.alliances()[0].other(player2)).toBe(player1); + }); + + test("alliances() agrees with isAlliedWith / allianceWith", () => { + ally(player1, player2); + + expect(player1.isAlliedWith(player2)).toBe(true); + expect(player1.allianceWith(player2)).toBe(player1.alliances()[0]); + expect(player1.isAlliedWith(player3)).toBe(false); + expect(player3.alliances()).toHaveLength(0); + }); + + test("breaking an alliance removes it from both lists", () => { + ally(player1, player2); + expect(player1.alliances()).toHaveLength(1); + + breakAlliance(player1, player2); + + expect(player1.alliances()).toHaveLength(0); + expect(player2.alliances()).toHaveLength(0); + expect(player1.isAlliedWith(player2)).toBe(false); + }); + + test("expiring an alliance removes it from both lists", () => { + ally(player1, player2); + expect(player1.alliances()).toHaveLength(1); + + player1.alliances()[0].expire(); + + expect(player1.alliances()).toHaveLength(0); + expect(player2.alliances()).toHaveLength(0); + expect(player1.isAlliedWith(player2)).toBe(false); + }); + + test("a player tracks multiple alliances independently", () => { + ally(player1, player2); + ally(player1, player3); + + expect(player1.alliances()).toHaveLength(2); + const others = player1.alliances().map((a) => a.other(player1)); + expect(others).toContain(player2); + expect(others).toContain(player3); + + // Breaking one leaves the other intact. + breakAlliance(player1, player2); + + expect(player1.alliances()).toHaveLength(1); + expect(player1.alliances()[0].other(player1)).toBe(player3); + expect(player2.alliances()).toHaveLength(0); + expect(player3.alliances()).toHaveLength(1); + }); + + test("removeAllAlliances clears the player and every partner", () => { + ally(player1, player2); + ally(player1, player3); + expect(player1.alliances()).toHaveLength(2); + + player1.removeAllAlliances(); + + expect(player1.alliances()).toHaveLength(0); + expect(player2.alliances()).toHaveLength(0); + expect(player3.alliances()).toHaveLength(0); + }); +});