Round-2 stability: profiler honesty, GI backend logging, crash-triage kit, test repairs#75
Merged
Merged
Conversation
…nd-2 audit) Profiler correctness (audit F9 — these skewed every round-1 measurement): - Rolling stats expire: a pass that stops running (feature toggled off) drops out of snapshot/summary/overlay after one window instead of showing a frozen average forever; entries evict after 4 windows. - Re-enabling the profiler starts a fresh session (clears rolling stats + frame histogram) instead of blending pre-disable numbers in. - One-shot warning when the 32-pair GPU timestamp budget is exhausted (was a silent truncation of later passes). - Restore the orphaned `post_fx` CPU bracket — begin() was lost at some point, leaving end() a no-op while a comment claimed the phase covered the composite tail's cost. - frame_end doc no longer claims the readback is non-blocking: it map_asyncs + poll(Wait)s every frame, serialising CPU<->GPU, so wall fps is pessimistic while profiling (measured ~3 fps at combat load). - Unit tests for the expiry + fresh-session semantics. Boot observability (audit F4 — the silent HW->SW GI fallback cost a debugger day): - windows: log adapter name/backend + ray_query/timestamp feature flags at device creation. - One-shot "bloom: ssgi trace backend = hw-ray-query|sdf-clipmap| hiz-screen" when the trace tier is chosen (re-logs on promotion). Crash-triage kit (audit F1, EN-020): - native/windows release profile carries line-tables-only debug info so `perry compile --debug-symbols` yields a usable PDB. - docs/crash-triage-windows.md: the standing WER LocalDumps + symbols + llvm-symbolizer workflow, and everything known about the layout- sensitive heap-read AV (3x reproduced pre-relink, gone after relink). - docs/tickets.md: EN-020 filed with repro history and ruled-out causes. Test repairs (pre-existing breakage from the round-1 merge): - material_system_tests: MaterialDrawCommand fixture gained the `model` field Tier 2 added — cargo test has not compiled since. - Regenerate the two golden references (lit_primitives_3d, many_point_lights_clustered_scene) that still encoded pre-round-1 shading; the other goldens pass unchanged and stay untouched.
📝 WalkthroughWalkthroughThis PR adds rolling-stats eviction and session-reset logic plus a one-shot GPU budget warning to the profiler, introduces SSGI backend-change logging tied to a new Renderer field, fixes an unbalanced post_fx profiler bracket, enables Windows release line-table debug symbols with adapter diagnostic logging, and adds crash-triage documentation and a ticket entry. ChangesProfiler rolling-stats eviction and reset
SSGI backend logging and renderer state
Windows crash triage: symbols, adapter logging, and docs
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Frame as Frame Loop
participant Profiler
participant RollingStats
Frame->>Profiler: frame_end_cpu(frame_count)
Profiler->>RollingStats: record entry.last_frame per label
Profiler->>RollingStats: retain labels within 4*ROLLING_FRAMES
Frame->>Profiler: snapshot()/summary()/avg_frame_cpu_us()
Profiler->>RollingStats: filter by fc - last_frame <= ROLLING_FRAMES
RollingStats-->>Profiler: recent stats only
Profiler-->>Frame: filtered readouts
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@native/shared/src/profiler.rs`:
- Around line 161-174: The fresh-session reset in set_enabled should also clear
the current in-flight profiler state, not just the rolling aggregates. Update
Profiler::set_enabled so that when transitioning from disabled to enabled it
resets frame, open_cpu, next_query, pending_gpu, and budget_warned alongside the
existing rolling/history fields, ensuring any old samples or warning suppression
do not carry into the first frame of a new session.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e33ac7c9-a6ec-48c6-9f4a-bf01b14c7419
⛔ Files ignored due to path filters (2)
native/shared/tests/golden/lit_primitives_3d.pngis excluded by!**/*.pngnative/shared/tests/golden/many_point_lights_clustered_scene.pngis excluded by!**/*.png
📒 Files selected for processing (8)
docs/crash-triage-windows.mddocs/tickets.mdnative/shared/src/profiler.rsnative/shared/src/renderer/material_system_tests.rsnative/shared/src/renderer/mod.rsnative/shared/src/renderer/ssgi_pass.rsnative/windows/Cargo.tomlnative/windows/src/lib.rs
Comment on lines
+161
to
+174
| pub fn set_enabled(&mut self, on: bool) { | ||
| if on && !self.enabled { | ||
| // Fresh measuring session: without this, stats captured before | ||
| // a disable (potentially under a different feature set) show | ||
| // until the rolling window refills and skew the first seconds | ||
| // of every new session. | ||
| self.rolling.clear(); | ||
| self.frame_total_cpu_us = [0.0; ROLLING_FRAMES]; | ||
| self.frame_total_gpu_us = [0.0; ROLLING_FRAMES]; | ||
| self.histogram_idx = 0; | ||
| self.histogram_filled = 0; | ||
| } | ||
| self.enabled = on; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear current-frame profiler state when starting a fresh session.
set_enabled(true) resets rolling history, but not frame, open_cpu, next_query, pending_gpu, or budget_warned. If an external caller toggles off→on before frame_end runs, old in-flight samples can be folded into the first “fresh” frame, and the GPU budget warning stays suppressed from the previous session.
Proposed fix
pub fn set_enabled(&mut self, on: bool) {
if on && !self.enabled {
// Fresh measuring session: without this, stats captured before
// a disable (potentially under a different feature set) show
// until the rolling window refills and skew the first seconds
// of every new session.
+ self.open_cpu.clear();
+ self.frame.clear();
self.rolling.clear();
+ self.next_query = 0;
+ self.pending_gpu.clear();
+ self.budget_warned = false;
self.frame_total_cpu_us = [0.0; ROLLING_FRAMES];
self.frame_total_gpu_us = [0.0; ROLLING_FRAMES];
self.histogram_idx = 0;
self.histogram_filled = 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn set_enabled(&mut self, on: bool) { | |
| if on && !self.enabled { | |
| // Fresh measuring session: without this, stats captured before | |
| // a disable (potentially under a different feature set) show | |
| // until the rolling window refills and skew the first seconds | |
| // of every new session. | |
| self.rolling.clear(); | |
| self.frame_total_cpu_us = [0.0; ROLLING_FRAMES]; | |
| self.frame_total_gpu_us = [0.0; ROLLING_FRAMES]; | |
| self.histogram_idx = 0; | |
| self.histogram_filled = 0; | |
| } | |
| self.enabled = on; | |
| } | |
| pub fn set_enabled(&mut self, on: bool) { | |
| if on && !self.enabled { | |
| // Fresh measuring session: without this, stats captured before | |
| // a disable (potentially under a different feature set) show | |
| // until the rolling window refills and skew the first seconds | |
| // of every new session. | |
| self.open_cpu.clear(); | |
| self.frame.clear(); | |
| self.rolling.clear(); | |
| self.next_query = 0; | |
| self.pending_gpu.clear(); | |
| self.budget_warned = false; | |
| self.frame_total_cpu_us = [0.0; ROLLING_FRAMES]; | |
| self.frame_total_gpu_us = [0.0; ROLLING_FRAMES]; | |
| self.histogram_idx = 0; | |
| self.histogram_filled = 0; | |
| } | |
| self.enabled = on; | |
| } |
🤖 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 `@native/shared/src/profiler.rs` around lines 161 - 174, The fresh-session
reset in set_enabled should also clear the current in-flight profiler state, not
just the rolling aggregates. Update Profiler::set_enabled so that when
transitioning from disabled to enabled it resets frame, open_cpu, next_query,
pending_gpu, and budget_warned alongside the existing rolling/history fields,
ensuring any old samples or warning suppression do not carry into the first
frame of a new session.
This was referenced Jul 4, 2026
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
1 participant
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.
Phase-0 of the round-2 audit plan (full report: shooter
docs/audit-round2.md). No rendering behavior changes — observability, measurement correctness, and CI health.Profiler correctness (audit F9)
Round-1's numbers were all taken through two silent distortions this PR fixes/documents:
post_fxCPU bracket'sbegin()had been lost —end()was a no-op while comments claimed the phase covered the composite tail.frame_enddocs now state the truth: the GPU readback blocks (map_async + poll(Wait)) every frame, so wall fps is pessimistic while profiling (~3 fps at the shooter's combat load).Boot observability (audit F4)
The GI stack silently falls back HW→SDF→Hi-Z; nothing reported which tier ran, and it cost the audit a debugger-day to learn the dev box never runs HW ray query. Now:
Crash-triage kit (audit F1 / EN-020)
Three scripted audit runs hit the same c0000005 (
main.exe+0xe8e5, read 8 bytes below a page boundary); after a relink it stopped reproducing — layout-sensitive heap-read overrun. Rather than guess: line-tables now ship in the staticlib (debug = "line-tables-only", negligible cost),perry compile --debug-symbolsyields a PDB, WER LocalDumps are armed on the dev box, anddocs/crash-triage-windows.mdis the runbook. EN-020 tracks it; the next occurrence is a five-minute symbolized diagnosis.Test repairs (pre-existing round-1 breakage)
cargo testhas not compiled since Tier 2 addedMaterialDrawCommand.model— the translucent-sort fixture was never updated.lit_primitives_3d,many_point_lights_clustered_scene) and failed beyond tolerance; regenerated viaBLOOM_UPDATE_GOLDEN=1. The other goldens pass unchanged and are not touched.Suite is green: 99 lib tests + 7 golden (5 profiler tests including the 2 new ones).
Summary by CodeRabbit
Bug Fixes
Tests
Documentation