Propagate the round-robin field on a worker thread while the units step - #212
Propagate the round-robin field on a worker thread while the units step#212Giszmo wants to merge 1 commit into
Conversation
The resource, guard and clear area fields are refreshed one per tick, round robin, and that propagation is about two thirds of the simulation time on a 512x512 game. It now overlaps the units step: at the start of Game::syncStep, Map::stepGradients publishes the field due this tick, seeds the next one from the live map into a spare buffer and hands it to a worker thread. The worker reads only terrain and its own buffer, with its own propagation scratch. The field is published by a pointer swap GRADIENT_PIPELINE_TICKS (3) ticks after seeding; until then units read the previous buffer. Seeding and publication happen at fixed ticks, so the result does not depend on thread timing (per-tick checksum sidecars are identical to a synchronous build on 128x128 and 512x512 games). Whole-game wall time against master: 512x512, 8 Nicowars, 9216 ticks 42.7 s -> 31.4 s; 128x128, 2 Nicowars, 15360 ticks 3.37 s -> 2.67 s. The update*Gradient functions are split into a seed step and the propagation; the round robin moves from Map::syncStep into Map::pickRoundRobinField unchanged. Map::clear drains and stops the worker before freeing any field. Fable 5.1 helped authoring this commit.
|
Bradley approves the design direction and accepts the deliberate fixed delay in periodic gradient refreshes, conditional on resolving the technical concerns below. This is not approval to merge the current head (508395e). The bounded buffer pool and fixed-tick publication are a sensible approach. We independently built this head and its parent f50ab27 on macOS and ran three alternating 15,000-tick headless runs per build on each of two saved games. Peak RSS averaged 27.51 -> 28.55 MiB on 128x128 G2 (four teams) and 83.68 -> 83.28 MiB on 256x256 Oazis (eleven teams); the latter difference is within run variation, not evidence of savings. The pipeline retains three extra map-sized fields, not a second copy of every gradient. Memory does not look like a blocker on these workloads. CPU-time observations were noisy under concurrent machine load and are not a controlled performance result. Before merge:
With those concerns resolved and the final head reviewed/tested, I support landing this. No need to remove the threading or the accepted fixed-delay design merely to preserve the previous simulation's outcomes. |
|
The threading design here looks right and the speedup is real — overlapping the one round-robin field with the units step is a genuinely better answer than One thing is missing, and it is a blocker: this changes simulation results and does not bump the version. The branch touches no Measured directly rather than inferred. Same seed, same map, same AI line-up, this branch against its own merge-base
Ticks 0–198 are byte-identical in the sidecars; tick 199 is where they part. The game then ends 11360 ticks earlier with a quarter fewer orders. That is not drift in the noise — it is a different game from the same seed. What follows from that:
Note master is now at 96 and #232 took 97 (merged), so this wants 98 — but #256 and #257 are also converging on 97/98, so whichever lands last needs to move again. #232 landed a named-constant pattern in Nothing else in the review is blocking. Two smaller notes:
|
|
Approved in principle — handing the rebase back to you, because the architecture call in it is yours to make. To be clear about what that approval covers: the threading design, the speedup, and the determinism argument. Seeding on the main thread at a fixed tick and publishing by pointer swap a fixed number of ticks later is the right way to keep a worker off the critical path without making results depend on thread timing, and the evidence for it is thorough — pinned to one core, under six busy-loop processes, a synchronous build, 1 vs 8 workers, all byte-identical. That is the hard part and it is done. We are not asking you to re-litigate any of it. What we are not going to do is resolve the rebase ourselves. Four PRs have landed in exactly this machinery since this branch was cut, and the conflicts are semantic rather than textual:
Six hunks, about 128 lines, across #232 in particular is worth reading before you start: it means a structural map change now invalidates every cached field of every team, and the rebuild throttle is 100 ticks rather than 25. Whether that changes what your pipeline should schedule, or makes some of it unnecessary, is a judgement we would rather have from you than make for you. The two mechanical things, so they do not cost you a round trip
Ticks 0–198 are byte-identical; 199 is where they part. Divergence is expected and fine — we have been bumping the replay version freely through this run of engine work and are content to keep doing so. Tick 199 is offered only because One caveat on our own number: that is a single seed, and AI game length varies a lot between seeds. It proves the games differ; it says nothing about magnitude. Your pooled n=36 comparison is the better evidence there and we are not disputing it. Use the named-constant pattern. #232 landed Ping us when it is rebased and we will take it from there. |
2 participants
Follow-up to #184, prompted by @stephanemagnenat's question there about building gradients with
std::execution::par.What is parallel today. Nothing: the resource, guard and clear area fields are refreshed one per tick, round robin, at the end of
Map::syncStep, and building fields are built lazily inside the unit step that first asks for them.propagateGradientused one static scratch. There is never a batch of independent fields to hand to a parallel algorithm, sostd::execution::par(which on libstdc++ also means linking TBB, and which Apple's libc++ does not implement) has nothing to work on.What this does instead. The one round-robin field per tick is overlapped with the units step:
Game::syncStep,Map::stepGradientspublishes the field due this tick, then seeds the next one from the live map into a spare buffer and hands it to a worker thread.GradientScratch, one per thread, owned by the caller; the main thread's lives inMap).GRADIENT_PIPELINE_TICKS(3) ticks after seeding; until then units read the previous buffer. The old buffer becomes the spare.Seeding and publication happen at fixed ticks, so the simulation does not depend on thread timing. Verified with per-tick checksum sidecars (
GLOB2_CHECKSUM_SIDECAR=1) on 128² Mazury, 2 Nicowars, 15360 ticks: two plain runs, a run pinned to a single core (taskset -c 3, so main thread and worker interleave completely differently) and a run under six busy-loop processes all produce byte-identical sidecars; during development, a build that propagated the same jobs synchronously on the main thread matched the threaded one byte for byte on both maps, and 1 vs 8 worker threads likewise.Map::cleardrains and stops the worker before freeing any field.The lazily built building fields stay synchronous: on the 512² game they are 0.3 builds per tick (first build or stuck rebuild); "dirty" rebuilds do not occur outside the unit step in practice, because a field dirtied after its 25-tick delay is rebuilt in the same tick the map changes.
Measurements (release build, g++ 15,
-test-games-nox,GLOB2_TEST_SEED=1, base = masterf50ab27a, whole-game wall time, base and new alternated on an otherwise idle 20-core VM):The "worker disabled" column is from the development build that had a switch for it; the PR itself has no such switch.
Propagation is about two thirds of the simulation time on the 512² game, so this is most of what overlapping one field can give; the main thread still waits on the worker in short early-game ticks (about 10 % of its time there), which depth 3 already halves compared to depth 1. Refreshing two fields per tick on two workers was tried and dropped: 8 % slower on 512² (memory bandwidth) and no measurable economy gain.
Economy (end-of-game units, mean over seeds, base vs this PR; the field is 3 ticks staler than before on a refresh cycle of 20 to 60 ticks): Mazury n=36: 123.0 vs 120.0 (−2.4 %, t=−0.8); balanced_for_2 n=24: 164.2 vs 167.5 (+2.1 %, t=+0.8); 512² n=3: 472 vs 461. Within noise.
Code notes.
updateResourcesGradient/updateGuardAreasGradient/updateClearAreasGradientare split into a seed step and the propagation so a job can be seeded on the main thread and propagated elsewhere; the synchronous callers are unchanged. The round robin moved fromMap::syncStepintoMap::pickRoundRobinField, same order, same reset. Requires the thread support glob2 already links (pthread); MinGW's posix thread model covers Windows.Tests: the 170 unit tests and every
test/harness pass;immobile-unit-gradient-testandglobal-gradient-testpass.