fix: compiler remaining tasks#2276
Conversation
📝 WalkthroughWalkthroughProcessCompilationSet's remaining-task logging now snapshots atomic counters and uses saturating arithmetic to compute remaining tasks, avoiding unsigned underflow when counters race (e.g., ChangesShader Timing Underflow Prevention
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
No actionable suggestions for changed features. |
There was a problem hiding this comment.
Pull request overview
This PR updates shader-compilation telemetry in ShaderCache so the developer-facing timing logs no longer underflow the remaining count when async compilation state is reset during a build.
Changes:
- Replaced the direct
total - completed - failedsubtraction with a saturating calculation. - Reads
totalTasks,completedTasks, andfailedTasksexplicitly from atomics for the log-onlyremainingvalue. - Documents the race with
Clear()that previously produceduint64_tunderflow in shader timing logs.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ShaderCache.cpp (1)
2969-2974: ⚡ Quick winFactor out the saturated remaining-count calculation.
This fixes the timing log, but
StopCompilation()still uses raw subtraction for the same counters, so the same race can still leak into logs there. A shared helper would keep both call sites consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ShaderCache.cpp` around lines 2969 - 2974, Create a shared helper that computes the saturated remaining task count from a compilation set instead of duplicating the subtraction logic: implement a function (e.g., computeSaturatedRemaining or saturatedRemainingFor) that reads compilationSet.totalTasks, compilationSet.completedTasks, and compilationSet.failedTasks with the same memory_order_relaxed loads and returns (total > done ? total - done : 0); replace the inline calculation in ShaderCache.cpp (the current remaining computation using total/completed/failed) and update StopCompilation() to call this helper so both call sites use the same race-safe saturated subtraction logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ShaderCache.cpp`:
- Around line 2969-2974: Create a shared helper that computes the saturated
remaining task count from a compilation set instead of duplicating the
subtraction logic: implement a function (e.g., computeSaturatedRemaining or
saturatedRemainingFor) that reads compilationSet.totalTasks,
compilationSet.completedTasks, and compilationSet.failedTasks with the same
memory_order_relaxed loads and returns (total > done ? total - done : 0);
replace the inline calculation in ShaderCache.cpp (the current remaining
computation using total/completed/failed) and update StopCompilation() to call
this helper so both call sites use the same race-safe saturated subtraction
logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 027f85b3-ef03-4943-950a-3b7b63866a57
📒 Files selected for processing (1)
src/ShaderCache.cpp
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
✅ A pre-release build is available for this PR: |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
[15:54:03.843] [28964] [I] [ShaderTiming] Very slow 12317ms | queue_wait=0ms | remaining=18446744073709551615 | defines=5 | src=111008B | prio=3350 | Lighting:Pixel:GLINT TRUE_PBR DEFERRED WETNESS_EFFECTS WATER_EFFECTS VOLUMETRIC_SHADOWS UNIFIED_WATER TERRAIN_VARIATION TERRAIN_SHADOWS SSS SCREEN_SPACE_SHADOWS LOD_BLENDING ISL LIGHT_LIMIT_FIX CS_HAIR EXTENDED_TRANSLUCENCY EXTENDED_MATERIALS EXP_HEIGHT_FOG CLOUD_SHADOWS TERRAIN_BLENDING SKYLIGHTING IBL DYNAMIC_CUBEMAPS LANDSCAPE MULTI_TEXTURE ANISO_LIGHTING VC
Previously it shows something like that ^ with nonsense for "remaining"
Now correctly shows the below, accurately counting down the remaining shaders to compile:
[16:52:52.354] [28832] [I] [ShaderTiming] Very slow 11714ms | queue_wait=115ms | remaining=5 | defines=4 | src=109690B | prio=2320 | Lighting:Pixel:GLINT TRUE_PBR DEFERRED WETNESS_EFFECTS WATER_EFFECTS VOLUMETRIC_SHADOWS UNIFIED_WATER TERRAIN_SHADOWS SSS SCREEN_SPACE_SHADOWS LOD_BLENDING ISL LIGHT_LIMIT_FIX CS_HAIR EXTENDED_TRANSLUCENCY EXTENDED_MATERIALS EXP_HEIGHT_FOG CLOUD_SHADOWS TERRAIN_BLENDING SKYLIGHTING IBL DYNAMIC_CUBEMAPS ANISO_LIGHTING VC
[16:52:52.638] [8144 ] [I] [ShaderTiming] Very slow 12022ms | queue_wait=52ms | remaining=4 | defines=5 | src=109690B | prio=2350 | Lighting:Pixel:GLINT TRUE_PBR DEFERRED WETNESS_EFFECTS WATER_EFFECTS VOLUMETRIC_SHADOWS UNIFIED_WATER TERRAIN_SHADOWS SSS SCREEN_SPACE_SHADOWS LOD_BLENDING ISL LIGHT_LIMIT_FIX CS_HAIR EXTENDED_TRANSLUCENCY EXTENDED_MATERIALS EXP_HEIGHT_FOG CLOUD_SHADOWS TERRAIN_BLENDING SKYLIGHTING IBL DYNAMIC_CUBEMAPS ANISO_LIGHTING DO_ALPHA_TEST VC
[16:53:07.486] [24744] [I] [ShaderTiming] Very slow 26880ms | queue_wait=49ms | remaining=3 | defines=6 | src=109690B | prio=3080 | Lighting:Pixel:TRUE_PBR DEFERRED WETNESS_EFFECTS WATER_EFFECTS VOLUMETRIC_SHADOWS UNIFIED_WATER TERRAIN_VARIATION TERRAIN_SHADOWS SSS SCREEN_SPACE_SHADOWS LOD_BLENDING ISL LIGHT_LIMIT_FIX CS_HAIR EXTENDED_TRANSLUCENCY EXTENDED_MATERIALS EXP_HEIGHT_FOG CLOUD_SHADOWS TERRAIN_BLENDING SKYLIGHTING IBL DYNAMIC_CUBEMAPS LOD_LAND_BLEND LANDSCAPE MULTI_TEXTURE VC
[16:53:13.901] [10880] [I] [ShaderTiming] Very slow 33314ms | queue_wait=30ms | remaining=2 | defines=5 | src=109690B | prio=3350 | Lighting:Pixel:GLINT TRUE_PBR DEFERRED WETNESS_EFFECTS WATER_EFFECTS VOLUMETRIC_SHADOWS UNIFIED_WATER TERRAIN_VARIATION TERRAIN_SHADOWS SSS SCREEN_SPACE_SHADOWS LOD_BLENDING ISL LIGHT_LIMIT_FIX CS_HAIR EXTENDED_TRANSLUCENCY EXTENDED_MATERIALS EXP_HEIGHT_FOG CLOUD_SHADOWS TERRAIN_BLENDING SKYLIGHTING IBL DYNAMIC_CUBEMAPS LANDSCAPE MULTI_TEXTURE ANISO_LIGHTING VC
[16:53:14.455] [8924 ] [I] [ShaderTiming] Very slow 33895ms | queue_wait=2ms | remaining=1 | defines=7 | src=109690B | prio=3410 | Lighting:Pixel:GLINT TRUE_PBR DEFERRED WETNESS_EFFECTS WATER_EFFECTS VOLUMETRIC_SHADOWS UNIFIED_WATER TERRAIN_VARIATION TERRAIN_SHADOWS SSS SCREEN_SPACE_SHADOWS LOD_BLENDING ISL LIGHT_LIMIT_FIX CS_HAIR EXTENDED_TRANSLUCENCY EXTENDED_MATERIALS EXP_HEIGHT_FOG CLOUD_SHADOWS TERRAIN_BLENDING SKYLIGHTING IBL DYNAMIC_CUBEMAPS LOD_LAND_BLEND LANDSCAPE MULTI_TEXTURE ANISO_LIGHTING VC
Summary by CodeRabbit