perf: remove LOD CPU and submission hot spots

[MichaelFisher1997]

Summary

• share frame-keyed LOD visibility and coverage between terrain and water, with cheap rejects before coverage scans
• default-enable pooled LOD MDI for terrain and water while preserving mixed/direct fallback paths
• move cache reads, serialization, and writes to a bounded dedicated worker with token/revision validation
• mesh outside the global manager lock and reconcile paused queued jobs safely
• add per-level visibility/rejection telemetry to benchmark artifacts

Validation

• nix develop --command zig build test
• nix develop --command zig build -Doptimize=ReleaseFast
• nix develop --command zig build -Dskip-present
• bounded ReleaseFast traversal benchmark (artifact produced; local GPU-memory SLO exceeded due environment allocation reporting)
• pre-push formatting and full test hooks

Closes #920

[github-actions]

📋 Summary

This PR closes #920 ("LOD GPU roadmap Phase 1: remove CPU and submission hot spots"). It implements the phase's core acceptance criteria: cache I/O is moved off the update thread to a dedicated bounded worker, meshing no longer holds the global manager lock, visibility and chunk-coverage are projected once per frame and shared between terrain and water, pooled MDI is default-enabled for both terrain and water, and pause/unpause reconciliation is added. The implementation matches the issue requirements, and the existing test suite plus new tests pass.

📌 Review Metadata

• Reviewed Commit SHA: 4b10c1113b148a789935438f2c66b30a771d01bd
• Reviewed PR: perf: remove LOD CPU and submission hot spots #925
• Linked Issue: Closes LOD GPU roadmap Phase 1: remove CPU and submission hot spots #920
• Issue Verification: The changes satisfy the issue's acceptance criteria: no synchronous disk I/O on the update path, meshing is lock-free, terrain/water share a visibility projection, MDI is production-hardened with a fallback, and per-level telemetry is added.

The PR is a focused performance refactor with good test coverage, but two medium-severity bugs should be addressed before merging.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

None identified.

💡 Medium Priority Issues (Nice to Fix)

[MEDIUM] modules/world-lod/src/lod_cache_io.zig:86 and :99 - Bounded-queue rejection leaks copied path (and write snapshot data path)
Confidence: High
Description: In enqueueRead and enqueueWrite, the path is duplicated before the queue-full check. If self.tasks.items.len >= MAX_PENDING_TASKS, the functions return false without freeing path_copy. For enqueueWrite, the caller is expected to deinit the rejected snapshot, but the internal path_copy is still leaked.
Impact: Every rejected enqueue leaks a short-lived allocation. Under sustained I/O pressure this can accumulate memory (e.g., one path per frame at 60 Hz).
Suggested Fix: Move the capacity check before allocating, or free path_copy in the return false branch:

if (self.stopping or self.tasks.items.len >= MAX_PENDING_TASKS) {
    self.allocator.free(path_copy);
    return false;
}

[MEDIUM] modules/world-lod/src/lod_renderer.zig:486 - renderProjectedLayer silently drops pooled meshes when indirect-command allocation fails
Confidence: High
Description: In renderProjectedLayer, when a pooled mesh is added to instance_data and draw_list, the subsequent draw_commands[lod_idx].append uses catch {}. If that allocation fails, the instance/draw-list entries remain but no indirect command is emitted. Because indirect_drawn will be true and the direct loop skips pooled meshes (if (indirect_drawn and mesh.isPooled()) continue;), the mesh is not rendered at all. Stats are also incremented for the missing draw.
Impact: In low-memory conditions, pooled LOD terrain/water can pop out while still being counted in telemetry.
Suggested Fix: Roll back the paired instance and draw-list entries on command-allocation failure, and only increment stats after the command is successfully appended:

if (mesh.isPooled()) {
    self.draw_commands[lod_idx].append(self.allocator, .{
        .vertexCount = range.count,
        .instanceCount = 1,
        .firstVertex = mesh.firstVertex(range),
        .firstInstance = @intCast(self.instance_data.items.len - 1),
    }) catch {
        _ = self.instance_data.pop();
        _ = self.draw_list.pop();
        continue;
    };
}
if (stats) |s| { ... }

ℹ️ Low Priority Suggestions (Optional)

None identified.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Cache I/O, visibility projection, and lifecycle accounting are cleanly separated into new/specialized modules.
Open/Closed	8	renderFrame is added as an optional field on LODRenderInterface; existing consumers fall back to render.
Liskov Substitution	8	The renderer interface remains substitutable: null render_frame_fn transparently routes to render.
Interface Segregation	8	LODRenderInterface only adds one optional entry point; no unnecessary surface area.
Dependency Inversion	8	Rendering depends on the RHI interface; cache I/O depends on abstraction (LODProfilingCollector pointer).
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 82%

Confidence Breakdown:

• Code Quality: 85% (well-structured, but two real bugs need fixing)
• Completeness: 90% (all issue acceptance criteria are addressed)
• Risk Level: 65% (concurrency, default MDI, and new async I/O pipeline are moderate-risk)
• Test Coverage: 85% (existing tests updated, new pause and MDI telemetry tests added)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The PR is architecturally sound and meets the linked issue's requirements, but the two medium issues (queue-full leak and silent pooled-mesh drop) should be fixed before merging.

{
  "reviewed_sha": "4b10c1113b148a789935438f2c66b30a771d01bd",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 2,
  "overall_confidence_score": 82,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

This PR closes #920 ("LOD GPU roadmap Phase 1: remove CPU and submission hot spots"). It satisfies the phase's acceptance criteria: cache I/O is moved off the update thread to a dedicated bounded worker, meshing no longer holds the global manager lock, visibility and chunk-coverage are projected once per frame and shared between terrain and water, pooled MDI is default-enabled with a direct fallback, pause/unpause reconciliation is added, and per-level visibility telemetry is wired to benchmark artifacts. The existing test suite plus new tests pass.

📌 Review Metadata

• Reviewed Commit SHA: e2e67f5d49399c025d862e1de70e736246ba5772
• Reviewed PR: perf: remove LOD CPU and submission hot spots #925
• Linked Issue: Closes LOD GPU roadmap Phase 1: remove CPU and submission hot spots #920
• Issue Verification: The changes satisfy the issue's acceptance criteria: no synchronous disk I/O on the update path, meshing is lock-free, terrain/water share a visibility projection, MDI is production-hardened with a fallback, and per-level telemetry is added.

This is a focused performance refactor with good architecture and test coverage. The previous review's two medium issues are fixed, but one new high-priority state-transition bug remains.

🔴 Critical Issues (Must Fix - Blocks Merge)

None identified.

⚠️ High Priority Issues (Should Fix)

[HIGH] modules/world-lod/src/lod_manager_generation_ops.zig:275 and :284 - Scratch-buffer allocation failure can strand chunks in .meshing or .uploading
Confidence: High
Description: In processStateTransitions, chunk.setState(.meshing) and chunk.setState(.uploading) are executed immediately after each successful append to the scratch buffers. The append is fallible; if it fails on the Nth candidate, the function returns the error immediately. Candidates 0..N-1 have already been flipped to .meshing/.uploading, but their jobs have not been pushed yet (that happens after the loop). On the next update tick, processStateTransitions only looks for .generated and .mesh_ready chunks, so these stranded chunks never get queued again. pending_region_count also remains inflated, eventually blocking new admissions.
Impact: Under memory pressure, LOD regions can become permanently stuck in a non-renderable state and the admission budget can be exhausted, causing visible holes and stalling new region generation.
Suggested Fix: Separate collection from state transitions. Collect all candidates into the scratch buffers without changing state, then only after the collection loop succeeds, transition all collected candidates to .meshing/.uploading before unlocking. If a later queue-push operation fails, revert the transitioned states in the error path.

💡 Medium Priority Issues (Nice to Fix)

✅ [FIXED] Previous issue: Bounded-queue rejection leaked the copied path in lod_cache_io.zig. The current code frees path_copy in the return false branch at lines 92 and 108.

✅ [FIXED] Previous issue: renderProjectedLayer silently dropped pooled meshes when indirect-command allocation failed. The current code rolls back instance_data and draw_list in the catch block at lines 492-496.

ℹ️ Low Priority Suggestions (Optional)

None identified.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Cache I/O, visibility projection, and lifecycle accounting are cleanly separated into new/specialized modules.
Open/Closed	8	renderFrame is added as an optional field on LODRenderInterface; existing consumers fall back to render.
Liskov Substitution	8	The renderer interface remains substitutable: null render_frame_fn transparently routes to render.
Interface Segregation	8	LODRenderInterface only adds one optional entry point; no unnecessary surface area.
Dependency Inversion	8	Rendering depends on the RHI interface; cache I/O depends on abstraction (LODProfilingCollector pointer).
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 75%

Confidence Breakdown:

• Code Quality: 80% (well-structured, but the state-transition bug is a real correctness issue)
• Completeness: 90% (all issue acceptance criteria are addressed)
• Risk Level: 65% (concurrency, default MDI, and new async I/O pipeline are moderate-risk)
• Test Coverage: 85% (existing tests updated, new pause and MDI telemetry tests added)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing (if applicable)

Verdict:

MERGE WITH FIXES — The PR is architecturally sound and meets the linked issue's requirements, but the processStateTransitions state-transition bug must be fixed before merging.

Machine Readable Verdict

{
  "reviewed_sha": "e2e67f5d49399c025d862e1de70e736246ba5772",
  "critical_issues": 0,
  "high_priority_issues": 1,
  "medium_priority_issues": 0,
  "overall_confidence_score": 75,
  "recommendation": "MERGE WITH FIXES"
}

opencode session  |  github run

[github-actions]

📋 Summary

This PR closes #920 ("LOD GPU roadmap Phase 1: remove CPU and submission hot spots"). It fully implements the phase's acceptance criteria: cache I/O is moved to a dedicated bounded worker (lod_cache_io.zig), meshing no longer holds the global manager lock, terrain and water share a single frame-keyed visibility projection through renderFrame, pooled MDI is default-enabled while preserving direct fallback paths, and per-level visibility telemetry is added to benchmark artifacts.

The changes are focused, well-structured, and all previously reported issues are resolved. I ran nix develop --command zig build test, nix develop --command zig fmt --check src/ modules/, and nix develop --command zig build -Dskip-present; all passed.

📌 Review Metadata

• Reviewed Commit SHA: aa2d85fe480d0fd0c91df11d7e7c892f9bbe9e2d
• Reviewed PR: perf: remove LOD CPU and submission hot spots #925
• Linked Issue: Closes LOD GPU roadmap Phase 1: remove CPU and submission hot spots #920
• Issue Verification: The implementation satisfies the issue requirements: no synchronous disk I/O on the update path, lock-free meshing, shared terrain/water visibility projection, production-hardened MDI with fallback, pause/unpause reconciliation, and per-level telemetry wired to benchmark artifacts.

This is a clean performance refactor with good architecture and test coverage.

🔴 Critical Issues (Must Fix - Blocks Merge)

✅ All previously reported critical issues have been resolved.

None identified.

⚠️ High Priority Issues (Should Fix)

✅ All previously reported high priority issues have been resolved.

None identified.

💡 Medium Priority Issues (Nice to Fix)

✅ All previously reported medium priority issues have been resolved.

None identified.

ℹ️ Low Priority Suggestions (Optional)

None identified.

📊 SOLID Principles Score

Principle	Score	Notes
Single Responsibility	8	Cache I/O, visibility projection, and lifecycle accounting are cleanly separated into specialized modules.
Open/Closed	8	renderFrame is added as an optional field on LODRenderInterface; existing consumers fall back to render.
Liskov Substitution	8	The renderer interface remains substitutable: null render_frame_fn transparently routes to render.
Interface Segregation	8	LODRenderInterface only adds one optional entry point; no unnecessary surface area.
Dependency Inversion	8	Rendering depends on the RHI interface; cache I/O depends on abstraction (LODProfilingCollector pointer).
Average	8.0

🎯 Final Assessment

Overall Confidence Score: 90%

Confidence Breakdown:

• Code Quality: 90% (well-structured, idiomatic Zig, good error handling)
• Completeness: 95% (all issue acceptance criteria are addressed)
• Risk Level: 70% (concurrency, default MDI, and new async I/O pipeline are moderate-risk, but tests and defensive state handling mitigate this)
• Test Coverage: 90% (existing tests updated, new pause/MDI/telemetry tests added)

Merge Readiness:

• All critical issues resolved
• SOLID average score >= 6.0
• Overall confidence >= 60%
• No security concerns
• Tests present and passing

Verdict:

MERGE — The PR is architecturally sound, meets the linked issue's requirements, and all previously reported issues are resolved.

{
  "reviewed_sha": "aa2d85fe480d0fd0c91df11d7e7c892f9bbe9e2d",
  "critical_issues": 0,
  "high_priority_issues": 0,
  "medium_priority_issues": 0,
  "overall_confidence_score": 90,
  "recommendation": "MERGE"
}

opencode session  |  github run