mirror of
https://github.com/openfrontio/OpenFrontIO.git
synced 2026-07-17 00:01:14 +00:00
Return the live TileSet from Player.tiles() instead of cloning (#4600)
Fixes #4598
## Problem
`PlayerImpl.tiles()` copied the entire owned-tile set into a fresh
native `Set` on every call — an O(owned tiles) allocation that grows
with player success (100k+ entries for a large late-game player). The
issue reporter measured 71.8% of sim CPU inside this clone in a bot
harness that samples tiles on large players.
## Why the copy existed, and why it's now safe to drop
The snapshot dates to ed4201ab8 (Aug 2024), which added a dead-defender
loop that conquers tiles out of the set while iterating it — the copy
guarded against mutation during iteration, back when `_tiles` was a
native collection.
`TileSet` has since made that guard obsolete: it guarantees
Set-identical iteration semantics under mutation (entries deleted during
iteration are skipped, compaction is deferred while any iterator is
live, so positions never shift under an iterator). All three call sites
that mutate while iterating (`AttackExecution.handleDeadDefender`,
`SpawnExecution`'s relinquish loop, the existing `PlayerImpl.test.ts`
conquer loop) only ever delete the entry currently being visited, which
is safe under Set semantics — so the live set is behavior-identical to
the snapshot.
## Change
- `Player.tiles()` now returns `ReadonlyTileSet` (matching
`borderTiles()`) and `PlayerImpl` returns `this._tiles` directly —
zero-copy, mirroring the approach in #4230.
- `ReadonlyTileSet` mirrors the `ReadonlySet` read surface (`size`,
`has`, `forEach`, `values`, iteration); every existing caller typechecks
unchanged.
- Added regression tests pinning the new live-view semantics and the
relinquish-during-iteration pattern `SpawnExecution` relies on.
## Verification
- `npx tsc --noEmit` clean
- Full suite: 21 files, 166 tests pass
- `npm run lint` + prettier clean
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -574,7 +574,7 @@ export interface Player {
|
||||
spawnTile(): TileRef | undefined;
|
||||
|
||||
// Territory
|
||||
tiles(): ReadonlySet<TileRef>;
|
||||
tiles(): ReadonlyTileSet;
|
||||
borderTiles(): ReadonlyTileSet;
|
||||
numTilesOwned(): number;
|
||||
conquer(tile: TileRef): void;
|
||||
|
||||
@@ -476,8 +476,8 @@ export class PlayerImpl implements Player {
|
||||
return this._tiles.size;
|
||||
}
|
||||
|
||||
tiles(): ReadonlySet<TileRef> {
|
||||
return new Set(this._tiles.values()) as Set<TileRef>;
|
||||
tiles(): ReadonlyTileSet {
|
||||
return this._tiles;
|
||||
}
|
||||
|
||||
borderTiles(): ReadonlyTileSet {
|
||||
|
||||
@@ -143,4 +143,30 @@ describe("PlayerImpl", () => {
|
||||
}
|
||||
expect(other.canSendAllianceRequest(player)).toBe(false);
|
||||
});
|
||||
|
||||
describe("tiles()", () => {
|
||||
test("returns a live view that reflects later ownership changes", () => {
|
||||
const tiles = player.tiles();
|
||||
const sizeBefore = tiles.size;
|
||||
const tile = game.ref(5, 5);
|
||||
player.conquer(tile);
|
||||
expect(tiles.has(tile)).toBe(true);
|
||||
expect(tiles.size).toBe(sizeBefore + 1);
|
||||
});
|
||||
|
||||
test("every tile is visited when relinquishing during iteration", () => {
|
||||
player.conquer(game.ref(1, 0));
|
||||
player.conquer(game.ref(2, 0));
|
||||
const owned = player.numTilesOwned();
|
||||
expect(owned).toBeGreaterThan(1);
|
||||
// SpawnExecution relinquishes all tiles while iterating tiles().
|
||||
let visited = 0;
|
||||
player.tiles().forEach((t) => {
|
||||
visited++;
|
||||
player.relinquish(t);
|
||||
});
|
||||
expect(visited).toBe(owned);
|
||||
expect(player.numTilesOwned()).toBe(0);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user