perf(map-generator): major CPU and memory optimisations#3860
Merged
evanpelle merged 9 commits intoopenfrontio:mainfrom May 6, 2026
Merged
perf(map-generator): major CPU and memory optimisations#3860evanpelle merged 9 commits intoopenfrontio:mainfrom
evanpelle merged 9 commits intoopenfrontio:mainfrom
Conversation
|
|
Contributor
WalkthroughAdds a CLI ChangesMap generation + concurrency limit
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Loader as loadTerrainMaps
participant Semaphore as sem(chan rgba(0,128,255,0.5))
participant Worker as mapWorker
participant Generator as MapGenerator
CLI->>Loader: start (reads --workers)
Loader->>Semaphore: make(chan struct{}, workersFlag)
Loader->>Worker: spawn goroutine per map
Worker->>Semaphore: acquire (sem <- struct{}{})
Worker->>Generator: process map (processWater, processShore, pack)
Generator-->>Worker: packed map data
Worker->>Semaphore: release (<-sem)
Worker-->>Loader: done
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
…ssing Previously all maps were processed concurrently with no limit, so all goroutines held their terrain data simultaneously causing peak RSS to spike on large batches. The new --workers flag (default: 4) uses a semaphore to cap true concurrency, keeping peak memory predictable. Note: GOMAXPROCS=1 only limits CPU threads; it does not prevent multiple goroutines from holding terrain arrays in memory at the same time. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…]bool
BFS area-finding in getArea, processWater and removeSmallIslands used a
map[string]bool keyed by fmt.Sprintf("%d,%d", x, y). On large maps this
map held millions of string keys, with every lookup and insert paying the
cost of string allocation and hash comparison.
Replaced with a flat []bool of size width*height, indexed by x*height+y.
Single allocation, O(1) access, zero GC pressure.
Benchmark (3 runs, -benchmem):
bosphorusstraits (~612K tiles): 578ms → 105ms (5.5×), 594MB → 458MB, 11.5M → 3.1M allocs
world (~2M tiles): 2333ms → 320ms (7.3×), 2128MB → 1658MB, 42.4M → 10.0M allocs
giantworldmap (~8M tiles): 10701ms → 1333ms (8.0×), 9300MB → 7196MB, 182.7M → 40.0M allocs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…te slice allocs getNeighborCoords allocated a new []Coord on every call (up to 4 elements), and getNeighbors allocated a further []Terrain slice on top. Both were called millions of times per map — in processShore (once per tile) and getArea (once per queued tile in every BFS traversal). Inlined both functions at their two call sites: - processShore: replaced neighbor slice iteration with 4 direct bounds-checked comparisons using short-circuit evaluation. - getArea: replaced getNeighborCoords + queue append with 4 conditional appends. Removed getNeighbors and getNeighborCoords entirely. Benchmark vs previous commit (3 runs, -benchmem): bosphorusstraits (~612K tiles): 105ms → 52ms (2.0×), 458MB → 254MB, 3.1M → 664K allocs world (~2M tiles): 320ms → 170ms (1.9×), 1658MB → 989MB, 10.0M → 2.2M allocs giantworldmap (~8M tiles): 1333ms → 734ms (1.8×), 7196MB → 4516MB, 40.0M → 8.6M allocs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…der and uint8 type Two changes with no semantic impact: - TerrainType: int (8 bytes) → uint8 (1 byte). Only two values (Land=0, Water=1) are ever used, well within uint8 range. - Struct field order: Magnitude float64 moved first so it sits at offset 0 (natural 8-byte alignment). The three 1-byte fields follow with 5 bytes of tail padding, giving 16 bytes vs 24 with the original layout. No casts or logic changes required. Benchmark vs previous commit (3 runs, -benchmem): bosphorusstraits (~612K tiles): 52ms → 44ms (1.2×), 254MB → 240MB world (~2M tiles): 170ms → 148ms (1.2×), 989MB → 946MB giantworldmap (~8M tiles): 734ms → 635ms (1.2×), 4516MB → 4339MB Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…no longer needed The PNG image buffer and decoded image.Image are only needed for the pixel classification loop. The three terrain grids (1x, 4x, 16x) are each only needed until their packTerrain call completes. Nil-ing them immediately allows the GC to reclaim the memory sooner. This has no measurable effect in single-map benchmarks (GC cleans up at the same time either way), but meaningfully reduces peak RSS when multiple maps are processed concurrently via --workers, since goroutines no longer hold finished terrain arrays while waiting for others to complete. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously getArea pushed all neighbours onto the BFS queue without checking visited first. This allowed the same tile to be enqueued up to 4 times (once per neighbour that discovers it), bloating the queue to ~4× the actual region size and wasting matching pop iterations. Fix: mark each tile visited immediately before pushing, so each tile enters the queue at most once. Benchmark vs previous commit (3 runs, -benchmem): bosphorusstraits (~612K tiles): 49.7ms → 44.9ms (1.1×), 240MB → 134MB (1.8×), 664K → 661K allocs world (~2M tiles): 165ms → 150ms (1.1×), 946MB → 553MB (1.7×), 2.2M → 2.2M allocs giantworldmap (~8M tiles): 706ms → 635ms (1.1×), 4339MB → 2508MB (1.7×), 8.6M → 8.6M allocs Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
alexjurkiewicz
force-pushed
the
feat/map-generator-optimisations
branch
from
9c5be57 to
644c432
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
map-generator/main.go (1)
263-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-positive
--workersvalues before using the semaphore.
--workers=0deadlocks onsem <- struct{}{}, and negative values panic inmake(chan struct{}, workersFlag). Please validate this flag as>= 1beforeloadTerrainMapsstarts.Suggested fix
func loadTerrainMaps() error { + if workersFlag < 1 { + return fmt.Errorf("--workers must be >= 1, got %d", workersFlag) + } + selectedMaps, err := parseMapsFlag() if err != nil { return err } var wg sync.WaitGroupAlso applies to: 304-304
🤖 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 `@map-generator/main.go` around lines 263 - 275, Reject invalid worker counts by validating the workersFlag (or equivalent flag variable) is >= 1 before creating the semaphore or calling loadTerrainMaps: check the flag early (e.g., in main or before sem := make(chan struct{}, workersFlag)) and if workersFlag < 1 return/exit with a clear error message so you never call make(chan struct{}, workersFlag) or attempt sem <- struct{}{} with 0 or negative capacity; update any other uses (e.g., the loadTerrainMaps invocation) to assume a validated positive worker count.
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
553-587: ⚡ Quick winUpdate the pack-format comment to match the new water encoding.
Lines 558-559 still describe water magnitude as a
0-31value, but Line 586 now clamps encoded water depth to10. Please update the comment here so the binary format is not misleading.🤖 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 `@map-generator/map_generator.go` around lines 553 - 587, The packTerrain function's header comment is out of sync: update the binary format comment to reflect that water magnitude is encoded 0-10 (not 0-31) because the code clamps water depth to 10; specifically edit the comment lines describing "Bits 0-4: Magnitude (0-31). For Water, this is (Distance / 2)." to indicate bits 0-4 hold magnitude for land (0-31) but for water the stored magnitude is clamped to 0-10 (computed as Distance/2), so the comment matches the encoding in packTerrain.
🤖 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.
Outside diff comments:
In `@map-generator/main.go`:
- Around line 263-275: Reject invalid worker counts by validating the
workersFlag (or equivalent flag variable) is >= 1 before creating the semaphore
or calling loadTerrainMaps: check the flag early (e.g., in main or before sem :=
make(chan struct{}, workersFlag)) and if workersFlag < 1 return/exit with a
clear error message so you never call make(chan struct{}, workersFlag) or
attempt sem <- struct{}{} with 0 or negative capacity; update any other uses
(e.g., the loadTerrainMaps invocation) to assume a validated positive worker
count.
---
Nitpick comments:
In `@map-generator/map_generator.go`:
- Around line 553-587: The packTerrain function's header comment is out of sync:
update the binary format comment to reflect that water magnitude is encoded 0-10
(not 0-31) because the code clamps water depth to 10; specifically edit the
comment lines describing "Bits 0-4: Magnitude (0-31). For Water, this is
(Distance / 2)." to indicate bits 0-4 hold magnitude for land (0-31) but for
water the stored magnitude is clamped to 0-10 (computed as Distance/2), so the
comment matches the encoding in packTerrain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87a9401b-b196-4d13-9236-67880196c6ad
📒 Files selected for processing (3)
map-generator/main.gomap-generator/map-generatormap-generator/map_generator.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
map-generator/map_generator.go (1)
397-440:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear inherited
Oceanflags before reclassifying water.
createMiniMapcopiesTerrainstructs from the previous scale, so 4x/16x tiles can arrive here withOcean=truealready set. This function only marks the new largest body as ocean, so staleOceanbits can survive on smaller lakes and get serialized intopackTerrain.Suggested fix
func processWater(ctx context.Context, terrain [][]Terrain, removeSmall bool) { logger := LoggerFromContext(ctx) logger.Info("Processing water bodies") width := len(terrain) height := len(terrain[0]) visited := make([]bool, width*height) + + for x := 0; x < width; x++ { + for y := 0; y < height; y++ { + terrain[x][y].Ocean = false + } + }🤖 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 `@map-generator/map_generator.go` around lines 397 - 440, The code fails to clear existing Terrain.Ocean flags before reclassifying the largest water body, causing stale Ocean=true values to persist; modify the createMiniMap flow (where terrain, visited, getArea and waterBodies are used) to first iterate the entire terrain grid and set terrain[x][y].Ocean = false for every cell (or at least for every Water cell) before computing waterBodies and marking largestWaterBody.coords as Ocean; ensure you reference the same terrain slice and Coord struct so the cleared flags are the ones later set on largestWaterBody.map-generator/main.go (1)
256-275:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject non-positive
--workersvalues.
make(chan struct{}, workersFlag)panics for negatives, and--workers=0deadlocks here because every goroutine blocks onsem <- struct{}{}before it can reach the deferred receive. Please validateworkersFlag >= 1before building the semaphore.Suggested fix
func loadTerrainMaps() error { + if workersFlag < 1 { + return fmt.Errorf("--workers must be >= 1") + } + selectedMaps, err := parseMapsFlag() if err != nil { return err }🤖 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 `@map-generator/main.go` around lines 256 - 275, The code creates a buffered semaphore with make(chan struct{}, workersFlag) in loadTerrainMaps but doesn't validate workersFlag, so negative values panic and zero deadlocks; add a validation at the start of loadTerrainMaps (or immediately after parseMapsFlag) to require workersFlag >= 1 and return a descriptive error (e.g., "invalid --workers value: must be >= 1") when it is not, before constructing sem; keep the semaphore logic (sem := make(chan struct{}, workersFlag)) and goroutine usage unchanged once validation is in place.
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
177-182: ⚡ Quick winPack and release each scale at its first safe point.
terrainstays live until after 4x/16x generation and WebP encoding, even though it is no longer needed onceterrain4xexists apart from its own packing step. Reordering the pack/nil steps would drop the largest grid earlier and reduce peak RSS more on the big maps this PR targets.🤖 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 `@map-generator/map_generator.go` around lines 177 - 182, The large terrain grids are kept live too long; after calling packTerrain for each scale (packTerrain(ctx, terrain), packTerrain(ctx, terrain4x), packTerrain(ctx, terrain16x)) immediately set the corresponding source grid variable to nil (terrain = nil right after packing terrain, terrain4x = nil right after packing mapData4x) so each large slice can be GC'd before the next-scale generation and WebP encoding; locate the packTerrain calls and reorder/insert the nil assignments so each scale is cleared as soon as its packed mapData/mapNumLandTiles are produced, and ensure later code reads only mapData/mapNumLandTiles variables.
🤖 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.
Outside diff comments:
In `@map-generator/main.go`:
- Around line 256-275: The code creates a buffered semaphore with make(chan
struct{}, workersFlag) in loadTerrainMaps but doesn't validate workersFlag, so
negative values panic and zero deadlocks; add a validation at the start of
loadTerrainMaps (or immediately after parseMapsFlag) to require workersFlag >= 1
and return a descriptive error (e.g., "invalid --workers value: must be >= 1")
when it is not, before constructing sem; keep the semaphore logic (sem :=
make(chan struct{}, workersFlag)) and goroutine usage unchanged once validation
is in place.
In `@map-generator/map_generator.go`:
- Around line 397-440: The code fails to clear existing Terrain.Ocean flags
before reclassifying the largest water body, causing stale Ocean=true values to
persist; modify the createMiniMap flow (where terrain, visited, getArea and
waterBodies are used) to first iterate the entire terrain grid and set
terrain[x][y].Ocean = false for every cell (or at least for every Water cell)
before computing waterBodies and marking largestWaterBody.coords as Ocean;
ensure you reference the same terrain slice and Coord struct so the cleared
flags are the ones later set on largestWaterBody.
---
Nitpick comments:
In `@map-generator/map_generator.go`:
- Around line 177-182: The large terrain grids are kept live too long; after
calling packTerrain for each scale (packTerrain(ctx, terrain), packTerrain(ctx,
terrain4x), packTerrain(ctx, terrain16x)) immediately set the corresponding
source grid variable to nil (terrain = nil right after packing terrain,
terrain4x = nil right after packing mapData4x) so each large slice can be GC'd
before the next-scale generation and WebP encoding; locate the packTerrain calls
and reorder/insert the nil assignments so each scale is cleared as soon as its
packed mapData/mapNumLandTiles are produced, and ensure later code reads only
mapData/mapNumLandTiles variables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bdb9bca6-edb2-40c5-bd37-f0cb552eb5c6
📒 Files selected for processing (3)
map-generator/main.gomap-generator/map-generatormap-generator/map_generator.go
--workers=0 would deadlock every goroutine on sem <- struct{}{} before
it could proceed. Negative values would panic in make(chan struct{}, n).
Add an early check in loadTerrainMaps and return a clear error.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
createMiniMap copies Terrain structs directly, so downscaled grids can inherit Ocean=true from the previous scale. processWater only marks the new largest body as ocean and never cleared existing flags, meaning small lakes adjacent to ocean tiles at a finer scale could retain Ocean=true in the packed output, incorrectly making them ocean rather than lake in the game. Fix by resetting all Ocean flags to false at the start of processWater, before any body classification occurs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
map-generator/map_generator.go (4)
479-506: 💤 Low valueBFS visit-on-push is correct here; one tiny note on queue growth.
Marking
visitedwhen pushing (line 498) is right: a tile can never be enqueued twice because the only path to enqueue it is via an unvisited check. Off-type neighbours getting marked is also harmless because the outer loops inprocessWaterandremoveSmallIslandsonly enter BFS on tiles of the matching type, and each call uses its ownvisitedslice.Optional:
queue = queue[1:]keeps the original backing array alive until the slice is fully dropped, so peak queue memory is the sum of every tile ever pushed in a body, not the live frontier. For very large oceans ongiantworldmapthis can matter. A head-index pop avoids it:♻️ Optional refactor
- queue := []Coord{{X: x, Y: y}} - - var buf [4]Coord - for len(queue) > 0 { - coord := queue[0] - queue = queue[1:] + queue := []Coord{{X: x, Y: y}} + head := 0 + + var buf [4]Coord + for head < len(queue) { + coord := queue[head] + head++🤖 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 `@map-generator/map_generator.go` around lines 479 - 506, getArea currently pops the queue with queue = queue[1:], which retains the underlying backing array and can cause high peak memory for large BFS runs; change the queue handling in getArea to use a head index (e.g., head := 0; push with queue = append(queue, c); pop by reading queue[head] and incrementing head) and, to avoid unbounded slice growth, periodically reset the slice when head grows large (e.g., when head > len(queue)/2 or a fixed threshold) by doing queue = queue[head:] and head = 0; update references in getArea (queue, head) and keep visited/neighbor logic the same.
343-343: 💤 Low valueMove
directionsto a package-levelvar(or drop it entirely here).This 4-element slice is rebuilt on every
processDistToLandcall. Tiny win, but you already useneighborCoordselsewhere — using it here too would also unify the neighbour-iteration style across the file.♻️ Suggested change
- directions := []Coord{{0, 1}, {1, 0}, {0, -1}, {-1, 0}} - - for len(queue) > 0 { - current := queue[0] - queue = queue[1:] - - for _, dir := range directions { - nx := current.x + dir.X - ny := current.y + dir.Y - - if nx >= 0 && ny >= 0 && nx < width && ny < height && - !visited[nx][ny] && terrain[nx][ny].Type == Water { - - visited[nx][ny] = true - terrain[nx][ny].Magnitude = float64(current.dist + 1) - queue = append(queue, queueItem{x: nx, y: ny, dist: current.dist + 1}) - } - } - } + var buf [4]Coord + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + + n := neighborCoords(current.x, current.y, width, height, &buf) + for _, c := range buf[:n] { + if !visited[c.X][c.Y] && terrain[c.X][c.Y].Type == Water { + visited[c.X][c.Y] = true + terrain[c.X][c.Y].Magnitude = float64(current.dist + 1) + queue = append(queue, queueItem{x: c.X, y: c.Y, dist: current.dist + 1}) + } + } + }🤖 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 `@map-generator/map_generator.go` at line 343, The local slice directions := []Coord{{0, 1}, {1, 0}, {0, -1}, {-1, 0}} inside processDistToLand is being reallocated on every call; move it to a package-level var (e.g. var directions = []Coord{...}) or replace its usage with the existing neighborCoords symbol to unify neighbor iteration. Update processDistToLand to reference the package-level directions (or neighborCoords) instead of creating the slice locally, and remove the local declaration to avoid repeated allocations.
419-419: 💤 Low valueHeads up: indexing convention differs between flat slices in this file.
getArea/processWater/removeSmallIslandsusex*height+y, whilepackTerrainusesy*width+x(line 596). Both are internally consistent so there is no bug, but it is easy to copy-paste the wrong formula in future edits. Worth a one-liner comment near each, or pick one convention everywhere.🤖 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 `@map-generator/map_generator.go` at line 419, getArea/processWater/removeSmallIslands currently index flat slices with x*height+y while packTerrain uses y*width+x; to avoid future copy-paste bugs, either standardize on one convention across the file or add a one-line clarifying comment next to each function/usage (e.g., near getArea, processWater, removeSmallIslands, and packTerrain) that states the exact indexing formula used for the visited/slice (mentioning whether the slice is row-major y*width+x or column-major x*height+y). Update any mismatched usages if you choose to standardize so all references use the same formula (and adjust variable names if needed) or leave code as-is but add the explicit comment beside visited, packTerrain, and the listed functions.
433-439: ⚡ Quick winReplace the hand-rolled bubble sort with
sort.Slice.O(n²) is fine when there are only a few water bodies, but for maps with many tiny lakes this is wasted work, and
sort.Slicereads much easier. Same idea applies if you later sortlandBodiestoo.♻️ Suggested change
+ sort.Slice(waterBodies, func(i, j int) bool { + return waterBodies[i].size > waterBodies[j].size + }) - for i := 0; i < len(waterBodies)-1; i++ { - for j := i + 1; j < len(waterBodies); j++ { - if waterBodies[j].size > waterBodies[i].size { - waterBodies[i], waterBodies[j] = waterBodies[j], waterBodies[i] - } - } - }Add
"sort"to the import block.🤖 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 `@map-generator/map_generator.go` around lines 433 - 439, Replace the hand-rolled O(n²) bubble sort loop that swaps elements of waterBodies with a call to sort.Slice: add "sort" to the import block and call sort.Slice(waterBodies, func(i, j int) bool { return waterBodies[i].size > waterBodies[j].size }) so the slice is sorted by size descending; apply the same pattern later if you sort landBodies as well.map-generator/main.go (1)
273-287: 💤 Low valueSmall tweak: acquire the semaphore before
wg.Add/goto cap goroutine count too.Right now each filtered map spawns a goroutine that immediately blocks on
sem <- struct{}{}. With ~80 maps that is fine, but if the registry grows a lot you will still hold ~N goroutine stacks waiting. Acquiring the slot in the loop body (beforego func()) keeps in-flight goroutines bounded byworkersFlag. Totally optional given the current scale.♻️ Optional refactor
for _, mapItem := range maps { if selectedMaps != nil && !selectedMaps[mapItem.Name] { continue } + sem <- struct{}{} wg.Add(1) mapItem := mapItem go func() { defer wg.Done() - sem <- struct{}{} defer func() { <-sem }() mapLogTag := slog.String("map", mapItem.Name)🤖 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 `@map-generator/main.go` around lines 273 - 287, The loop spawns goroutines that immediately block on sem <- struct{}{}, which still creates N waiting goroutine stacks; move the semaphore acquire to before creating the goroutine to bound in-flight goroutines. Specifically, perform sem <- struct{}{} in the loop before calling wg.Add(1) and starting the goroutine, then inside the goroutine keep defer wg.Done() and defer func() { <-sem }() to release the slot; keep passing mapItem (or use a loop-local copy) and keep calling processMap and sending errors to errChan as before.
🤖 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 `@map-generator/main.go`:
- Around line 273-287: The loop spawns goroutines that immediately block on sem
<- struct{}{}, which still creates N waiting goroutine stacks; move the
semaphore acquire to before creating the goroutine to bound in-flight
goroutines. Specifically, perform sem <- struct{}{} in the loop before calling
wg.Add(1) and starting the goroutine, then inside the goroutine keep defer
wg.Done() and defer func() { <-sem }() to release the slot; keep passing mapItem
(or use a loop-local copy) and keep calling processMap and sending errors to
errChan as before.
In `@map-generator/map_generator.go`:
- Around line 479-506: getArea currently pops the queue with queue = queue[1:],
which retains the underlying backing array and can cause high peak memory for
large BFS runs; change the queue handling in getArea to use a head index (e.g.,
head := 0; push with queue = append(queue, c); pop by reading queue[head] and
incrementing head) and, to avoid unbounded slice growth, periodically reset the
slice when head grows large (e.g., when head > len(queue)/2 or a fixed
threshold) by doing queue = queue[head:] and head = 0; update references in
getArea (queue, head) and keep visited/neighbor logic the same.
- Line 343: The local slice directions := []Coord{{0, 1}, {1, 0}, {0, -1}, {-1,
0}} inside processDistToLand is being reallocated on every call; move it to a
package-level var (e.g. var directions = []Coord{...}) or replace its usage with
the existing neighborCoords symbol to unify neighbor iteration. Update
processDistToLand to reference the package-level directions (or neighborCoords)
instead of creating the slice locally, and remove the local declaration to avoid
repeated allocations.
- Line 419: getArea/processWater/removeSmallIslands currently index flat slices
with x*height+y while packTerrain uses y*width+x; to avoid future copy-paste
bugs, either standardize on one convention across the file or add a one-line
clarifying comment next to each function/usage (e.g., near getArea,
processWater, removeSmallIslands, and packTerrain) that states the exact
indexing formula used for the visited/slice (mentioning whether the slice is
row-major y*width+x or column-major x*height+y). Update any mismatched usages if
you choose to standardize so all references use the same formula (and adjust
variable names if needed) or leave code as-is but add the explicit comment
beside visited, packTerrain, and the listed functions.
- Around line 433-439: Replace the hand-rolled O(n²) bubble sort loop that swaps
elements of waterBodies with a call to sort.Slice: add "sort" to the import
block and call sort.Slice(waterBodies, func(i, j int) bool { return
waterBodies[i].size > waterBodies[j].size }) so the slice is sorted by size
descending; apply the same pattern later if you sort landBodies as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3da5d93c-c88b-4868-943d-91e9b11af03f
📒 Files selected for processing (2)
map-generator/main.gomap-generator/map_generator.go
visited slices use x*height+y (column-major, mirrors terrain[x][y]). packTerrain output uses y*width+x (row-major, matches raster scan order). Both are intentional; add a note to each to prevent confusion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
Workers deadlock/panic + stale Ocean flags: Both fixed in commits Indexing inconsistency ( BFS head-index to reduce peak memory: Acknowledged, but the map generator is a CLI batch tool and peak memory is already significantly reduced by the visit-on-push change. Declining for now.
Acquire semaphore before spawning goroutine: At ~80 maps goroutine stacks are negligible. The acquire-inside pattern is idiomatic Go. Leaving as-is. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
map-generator/map_generator.go (1)
496-501: ⚡ Quick winFilter neighbors by
targetTypebefore enqueueing ingetArea.Right now, Line 499 marks non-target neighbors as visited and queues them, then they are discarded when popped. This adds avoidable queue churn on big maps.
♻️ Suggested change
n := neighborCoords(coord.X, coord.Y, width, height, &buf) for _, c := range buf[:n] { - if !visited[c.X*height+c.Y] { - visited[c.X*height+c.Y] = true - queue = append(queue, c) - } + idx := c.X*height + c.Y + if !visited[idx] && terrain[c.X][c.Y].Type == targetType { + visited[idx] = true + queue = append(queue, c) + } }🤖 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 `@map-generator/map_generator.go` around lines 496 - 501, In getArea, avoid marking and enqueueing neighbors that aren't the desired tile type: when iterating neighbors from neighborCoords (buf[:n]), first check that the neighbor's tile matches targetType before setting visited[c.X*height+c.Y] and appending to queue; leave non-matching neighbors unvisited so they won't generate unnecessary queue churn. Ensure you still prevent re-enqueueing by using the visited map only for neighbors that pass the targetType check.
🤖 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 `@map-generator/map_generator.go`:
- Around line 496-501: In getArea, avoid marking and enqueueing neighbors that
aren't the desired tile type: when iterating neighbors from neighborCoords
(buf[:n]), first check that the neighbor's tile matches targetType before
setting visited[c.X*height+c.Y] and appending to queue; leave non-matching
neighbors unvisited so they won't generate unnecessary queue churn. Ensure you
still prevent re-enqueueing by using the visited map only for neighbors that
pass the targetType check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0a9b1bfd-10fa-4bd3-b597-a51e7015dc3e
📒 Files selected for processing (1)
map-generator/map_generator.go
github-project-automation
Bot
moved this from Triage
to Final Review
in OpenFront Release Management
github-project-automation
Bot
moved this from Final Review
to Complete
in OpenFront Release Management
evanpelle
pushed a commit
that referenced
this pull request
May 6, 2026
## Description: Five performance improvements to the map generator, measured on three maps of increasing size. End-to-end time on `world` improved ~15×, heap allocations ~19×. | Map | Before | After | Speedup | |-----|--------|-------|---------| | bosphorusstraits (~612K tiles) | 578ms / 594MB | 45ms / 134MB | 13× / 4.4× | | world (~2M tiles) | 2333ms / 2128MB | 150ms / 553MB | 15× / 3.8× | | giantworldmap (~8M tiles) | 10701ms / 9300MB | 635ms / 2509MB | 17× / 3.7× | Changes (one commit each): - **`--workers` flag**: bounds concurrent map processing to limit peak memory - **Flat `[]bool` visited sets**: replaced `map[string]bool` keyed by `fmt.Sprintf` with flat `[]bool` indexed `x*height+y` — the dominant cost - **`neighborCoords` with stack buffer**: eliminates per-call slice allocation for neighbour lookups - **`Terrain` struct 24→16 bytes**: field reorder + `uint8` type for `TerrainType` - **Nil buffers early**: releases image/terrain arrays as soon as they're no longer needed - **BFS mark-visited on push**: each tile enters the queue once instead of up to 4×, halving queue memory also fixes a bug (according to Claude): Here's the bug: createMiniMap downscales by averaging/sampling 2x2 blocks, copying field values across — including Ocean=true from the parent scale. When a single connected ocean at 1x splits into multiple disconnected bodies at 4x (because narrow water channels disappear when you halve resolution), those smaller fragments still carry Ocean=true from the carryover. The 4x processWater call picks the new largest fragment and sets it to Ocean=true, but never clears the others — so multiple disconnected bodies end up flagged as Ocean. This PR's fix: before the new BFS pass, zero out every Ocean flag, so only the truly-largest body at the current scale ends up marked. It's incidental to the perf work but it's a real semantic change — the on-disk .bin files will differ from main on any map where ocean splits across downscaling. The PR doesn't mention it, which is why I flagged it. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
4 tasks
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
3 participants
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.
Description:
Five performance improvements to the map generator, measured on three maps of increasing size. End-to-end time on
worldimproved ~15×, heap allocations ~19×.Changes (one commit each):
--workersflag: bounds concurrent map processing to limit peak memory[]boolvisited sets: replacedmap[string]boolkeyed byfmt.Sprintfwith flat[]boolindexedx*height+y— the dominant costneighborCoordswith stack buffer: eliminates per-call slice allocation for neighbour lookupsTerrainstruct 24→16 bytes: field reorder +uint8type forTerrainTypeAlso fixes a bug:
createMiniMapdownscales by averaging/sampling 2x2 blocks, copying field values across — includingOcean=truefrom the parent scale. When a single connected ocean at 1x splits into multiple disconnected bodies at 4x (because narrow water channels disappear when you halve resolution), those smaller fragments still carryOcean=truefrom the carryover. The 4xprocessWatercall picks the new largest fragment and sets it toOcean=true, but never clears the others — so multiple disconnected bodies end up flagged as Ocean.Fix: before the new BFS pass, zero out every Ocean flag, so only the truly-largest body at the current scale ends up marked. This is a real semantic change — the on-disk
.binfiles will differ from main on any map where ocean splits across downscaling.Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
alexjurkiewicz