From 23b8eaa04f7674fe295ab99075bbc1dded13d1ef Mon Sep 17 00:00:00 2001 From: Mike Zaugg Date: Wed, 6 May 2026 20:44:08 +0200 Subject: [PATCH] perf(AttackExecution): migrate hot loops to forEachNeighbor (#3820) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resolves [#3815 ](https://github.com/openfrontio/OpenFrontIO/issues/3815) ## Description: Migrates the five `mg.neighbors(tile)` call sites in `src/core/execution/AttackExecution.ts` to the existing zero-allocation `forEachNeighbor(tile, cb)` helper (`src/core/game/GameImpl.ts:1107`). `neighbors()` allocates a fresh `TileRef[]` of length up to 4 on every call; `forEachNeighbor` invokes a callback with no allocation. The helper is already used in `PlayerExecution`, `GameImpl.updateBorders`, and similar hot paths — this PR finishes that rollout in the hottest attack loop. Affected sites in `src/core/execution/AttackExecution.ts`: | Original line | Context | | --- | --- | | 277 | `tick()` border check around `tileToConquer` | | 326 | `addNeighbors()` outer neighbor loop | | 335 | `addNeighbors()` nested `numOwnedByMe` count | | 370 | `handleDeadDefender()` border test | | 375 | `handleDeadDefender()` neighbor scan | Notes: - `handleDeadDefender` previously narrowed `this.target` from `Player | TerraNullius` to `Player` via the early-return check. Inside an arrow callback that narrowing isn't preserved by TypeScript, so the function now captures `target: Player` once after the check. Same pattern as `tick()`'s `targetPlayer` cache. - For loops that previously used `break` or `.some()`, I used a small flag variable to short-circuit the callback body. With at most 4 neighbors per tile the extra callback invocations are negligible compared to the eliminated array allocation. Behavioral guarantees: - Iteration order is identical (`forEachNeighbor` enumerates the same tiles in the same order as `neighbors()`). - No RNG, no float math, no wire-format changes. - Determinism preserved. Verification: - `npm test` — all 994 tests pass across 104 files. The 22 attack-related tests in `Attack.test.ts`, `AttackStats.test.ts`, and `AiAttackBehavior.test.ts` exercise the migrated code paths end-to-end. - `npx tsc --noEmit` — clean for the changed file. - `npx prettier --check` — clean. Briefly flagged in `#development` per the contribution guidelines before opening. ## Please complete the following: - [ ] I have added screenshots for all UI updates - [ ] I process any text displayed to the user through translateText() and I've added it to the en.json file - [ ] I have added relevant tests to the test directory - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced ## Please put your Discord username so you can be contacted if a bug or regression is found: sxndrexe --- src/core/execution/AttackExecution.ts | 47 ++++++++++++++------------- 1 file changed, 24 insertions(+), 23 deletions(-) diff --git a/src/core/execution/AttackExecution.ts b/src/core/execution/AttackExecution.ts index 0e3d6eb06..b820b132f 100644 --- a/src/core/execution/AttackExecution.ts +++ b/src/core/execution/AttackExecution.ts @@ -274,12 +274,11 @@ export class AttackExecution implements Execution { this.attack.removeBorderTile(tileToConquer); let onBorder = false; - for (const n of this.mg.neighbors(tileToConquer)) { - if (this.mg.owner(n) === this._owner) { + this.mg.forEachNeighbor(tileToConquer, (n) => { + if (!onBorder && this.mg.owner(n) === this._owner) { onBorder = true; - break; } - } + }); if (this.mg.owner(tileToConquer) !== this.target || !onBorder) { continue; } @@ -323,20 +322,20 @@ export class AttackExecution implements Execution { const tickNow = this.mg.ticks(); // cache tick - for (const neighbor of this.mg.neighbors(tile)) { + this.mg.forEachNeighbor(tile, (neighbor) => { if ( this.mg.isWater(neighbor) || this.mg.owner(neighbor) !== this.target ) { - continue; + return; } - this.attack.addBorderTile(neighbor); + this.attack!.addBorderTile(neighbor); let numOwnedByMe = 0; - for (const n of this.mg.neighbors(neighbor)) { + this.mg.forEachNeighbor(neighbor, (n) => { if (this.mg.owner(n) === this._owner) { numOwnedByMe++; } - } + }); let mag: number; switch (this.mg.terrainType(neighbor)) { @@ -359,33 +358,35 @@ export class AttackExecution implements Execution { tickNow; this.toConquer.enqueue(neighbor, priority); - } + }); } private handleDeadDefender() { if (!(this.target.isPlayer() && this.target.numTilesOwned() < 100)) return; + const target: Player = this.target; - this.mg.conquerPlayer(this._owner, this.target); + this.mg.conquerPlayer(this._owner, target); for (let i = 0; i < 10; i++) { - for (const tile of this.target.tiles()) { - const borders = this.mg - .neighbors(tile) - .some((t) => this.mg.owner(t) === this._owner); + for (const tile of target.tiles()) { + let borders = false; + this.mg.forEachNeighbor(tile, (t) => { + if (!borders && this.mg.owner(t) === this._owner) { + borders = true; + } + }); if (borders) { this._owner.conquer(tile); } else { - for (const neighbor of this.mg.neighbors(tile)) { + let captured = false; + this.mg.forEachNeighbor(tile, (neighbor) => { + if (captured) return; const no = this.mg.owner(neighbor); - if ( - no.isPlayer() && - no !== this.target && - !no.isFriendly(this.target) - ) { + if (no.isPlayer() && no !== target && !no.isFriendly(target)) { this.mg.player(no.id()).conquer(tile); - break; + captured = true; } - } + }); } } }