feat: update shader compile elapsed time every second

[soda3000]

Summary by CodeRabbit

• Bug Fixes

  • More accurate detection and recording of compilation completion, improving ETA and duration reporting.
  • Reduced logging contention by moving final logging out of synchronized sections.
  • Reset of completion timing when clearing state to avoid stale timings.

• Refactor

  • Reworked task-state handling for safer, more consistent progress tracking and statistics calculation.

[coderabbitai]

Walkthrough

CompilationSet adds a thread-safe completionTime and moves per-task completion state updates (processedTasks, tasksInProgress, counters, timing) into a mutex-guarded block; completion time is computed once and logged outside the lock. Stats/ETA now derive end time from completionTime when set.

Changes

Cohort / File(s)	Summary
Completion time field \src/ShaderCache.h``	Added std::atomic<int64_t> completionTime to SIE::CompilationSet and initialize it to 0 in the constructor.
Task completion & synchronization \src/ShaderCache.cpp``	Moved processedTasks insert, tasksInProgress erase, counters and totalTime/lastCalculation updates under a mutex; implemented double-checked pattern to set completionTime once; compute completionTimeMs while locked but log final completion outside the lock; Clear() resets completionTime.
Stats & ETA timing \src/ShaderCache.cpp``	GetStatsString and GetEta now use completionTime as end time when set (otherwise use current time); total/elapsed ms computed from endTime - lastReset and formatting adjusted to reflect the new semantics.

Sequence Diagram(s)

sequenceDiagram
    participant Worker as Worker Thread
    participant Mutex as Mutex
    participant State as CompilationSet
    participant Logger as Logger

    rect rgb(245, 250, 255)
    Note over Worker,State: New completion flow (double-check)
    Worker->>State: if completionTime == 0? (fast-path)
    alt completionTime == 0
        Worker->>Mutex: lock
        Worker->>State: check (processed+failed >= total && completionTime==0)
        alt condition true
            State->>State: set completionTime = now
            State->>State: update processedTasks, erase tasksInProgress
            State->>State: update lastCalculation, totalTime
            State-->>Worker: compute completionTimeMs
        end
        Mutex->>Worker: unlock
        Worker->>Logger: log final completion (outside lock)
    else
        Worker->>Worker: skip setting completionTime
    end
    end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

• Review atomic ordering and double-checked pattern for completionTime.
• Verify all state mutations (processedTasks, tasksInProgress, counters, timing) are consistently protected by the mutex.
• Confirm Clear(), GetStatsString, and GetEta correctly handle finished vs running paths and reset semantics.

Possibly related PRs

• perf(shadercache): use faster clock #1275 — earlier timing/state changes in CompilationSet and ShaderCache that touch completion/timing semantics.
• feat(UI): add shader cache duration #1408 — related adjustments to stats formatting and elapsed/ETA semantics in GetStatsString/GetEta.

Suggested reviewers

• alandtse

Poem

🐰 I hopped through mutex, careful and bright,

I set the finish in a single bite.
Tasks counted neat, logs chimed outside,
Timers settled, no more tide.
Cache compiled — hop, carrot-light!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "feat: update shader compile elapsed time every second" is directly aligned with the primary objective of the changeset. The modifications to ShaderCache.cpp and ShaderCache.h implement exactly this feature by introducing completionTime tracking, synchronizing timing updates under mutex locks, and updating the GetStatsString and GetEta methods to reflect new completion-time semantics. The title is specific and clear—a reviewer scanning the project history would immediately understand that this PR addresses shader compilation elapsed time display updates. The title accurately captures the main change without vagueness or unnecessary details.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Using provided base ref: b47ad5d
Using base ref: b47ad5d
Base commit date: 2025-10-29T16:35:58Z (Wednesday, October 29, 2025 04:35 PM)
No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8f51609 and d073d87.

📒 Files selected for processing (2)

• src/ShaderCache.cpp (4 hunks)
• src/ShaderCache.h (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

• src/ShaderCache.h
• src/ShaderCache.cpp

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

• src/ShaderCache.h
• src/ShaderCache.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build plugin and addons

🔇 Additional comments (3)

src/ShaderCache.h (2)

247-247: LGTM: Clear field declaration for completion tracking.

The completionTime field is appropriately typed and documented for tracking when shader compilation completes.

---

256-256: LGTM: Proper initialization.

Zero-initialization correctly indicates compilation has not yet completed.

src/ShaderCache.cpp (1)

2783-2785: LGTM: Helpful clarification.

The comment appropriately distinguishes between active work time (totalTime) and wall-clock elapsed time, explaining why ETA continues to use the former.

[github-actions]

✅ A pre-release build is available for this PR:
Download

[doodlum]

Check rabbit complaints first

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/ShaderCache.h (1)

247-247: LGTM! Thread-safe completion time tracking added.

The atomic int64_t type is appropriate for concurrent access and matches the QuadPart type used by the existing LARGE_INTEGER timing members. The public visibility is consistent with other timing-related members in this class.

Optional enhancement: Consider expanding the comment to clarify that 0 represents "not completed" and non-zero values represent the QueryPerformanceCounter timestamp when compilation finished.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d073d87 and 109c6aa.

📒 Files selected for processing (2)

• src/ShaderCache.cpp (4 hunks)
• src/ShaderCache.h (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/ShaderCache.cpp

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

• src/ShaderCache.h

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

• src/ShaderCache.h

🧠 Learnings (1)

📓 Common learnings

Learnt from: ThePagi
PR: doodlum/skyrim-community-shaders#1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.

Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build plugin and addons

🔇 Additional comments (1)

src/ShaderCache.h (1)

256-256: LGTM! Proper initialization with appropriate memory ordering.

The initialization to 0 correctly establishes the "not completed" sentinel value, and memory_order_relaxed is appropriate for constructor initialization where no concurrent access exists.

Note: Ensure that concurrent read/write operations in ShaderCache.cpp use appropriate memory ordering (likely memory_order_acquire/memory_order_release or stronger) for the double-checked completion pattern mentioned in the summary.

[zndxcvbn]

Great fix! The current time now runs as it should. Previously, there were "jitters" in the time, and I had doubts about whether time was running correctly, whether it had sped up, slowed down, or gotten out of sync. Now I know for sure that during this build, the shader compilation time for my laptop:

RAM: 16 GB
CPU: AMD Ryzen 7 6800H with Radeon Graphics (3.20 GHz)

was 00:09:17.
At 00:08:40, the percentage reached 99%, and after about 20 seconds, it reached 100% and hung until completion.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

src/ShaderCache.cpp (1)

2771-2771: Fix inconsistent atomic operation.

The code uses .load() and .store() for completionTime on lines 2740-2741 and 2808, but line 2771 uses brace-initialization = { 0 }. If completionTime is std::atomic<int64_t>, this is incorrect.

Apply this diff:

-		completionTime = { 0 };  // Reset completion time
+		completionTime.store(0, std::memory_order_relaxed);  // Reset completion time

🧹 Nitpick comments (1)

src/ShaderCache.cpp (1)

2808-2808: Optimize redundant atomic loads.

The expression loads completionTime twice. While unlikely to cause issues, it's inefficient and could theoretically observe different values.

Apply this diff:

-		// Use completion time if compilation is finished, otherwise current time
-		int64_t endTime = (completionTime.load(std::memory_order_relaxed) != 0) ? completionTime.load(std::memory_order_relaxed) : currentTime.QuadPart;
+		// Use completion time if compilation is finished, otherwise current time
+		int64_t completionTimeValue = completionTime.load(std::memory_order_relaxed);
+		int64_t endTime = (completionTimeValue != 0) ? completionTimeValue : currentTime.QuadPart;

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 109c6aa and 2f01e09.

📒 Files selected for processing (1)

• src/ShaderCache.cpp (4 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

• src/ShaderCache.cpp

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

• src/ShaderCache.cpp

🧠 Learnings (3)

📓 Common learnings

Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

📚 Learning: 2025-07-05T05:20:45.823Z

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.

Applied to files:

• src/ShaderCache.cpp

📚 Learning: 2025-08-17T18:37:35.839Z

Learnt from: CR
Repo: doodlum/skyrim-community-shaders PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-08-17T18:37:35.839Z
Learning: Applies to features/*/Shaders/**/*.{hlsl,hlsli,fx,fxh} : Avoid GPU register/buffer conflicts in HLSL; verify register usage (e.g., with hlslkit buffer scanning)

Applied to files:

• src/ShaderCache.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build plugin and addons

🔇 Additional comments (2)

src/ShaderCache.cpp (2)

2716-2754: LGTM - Clean simplification of the locking strategy.

The refactored completion logic properly consolidates all state updates under a single mutex acquisition, eliminating the double-checked locking complexity from earlier iterations. Logging completion outside the lock is a good performance practice.

---

2789-2791: LGTM - Helpful clarification.

The comment clearly explains why totalTime (work time) rather than wall-clock time is used for ETA calculation, which aids maintainability.

[alandtse]

@coderabbitai review this pr's changes critically to determine 1) what problem it actually solved and 2) if it's a correct and a performant solution. Because the code was written by AI, both the problem identification and solution may be incorrect.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the diff for this PR
gh pr diff 1587

echo "================================"
echo "Getting current file contents"
echo "================================"

# Get the current contents of the modified files
cat src/ShaderCache.h
echo "================================"
cat src/ShaderCache.cpp | head -500

Length of output: 46197

---

@alandtse, I've reviewed this PR critically. Here's my analysis:

1. Problem Identification ✓ (Correct)

The PR correctly identifies that the elapsed time display had "jitters" because:

• Original behavior: GetStatsString() used totalTime to calculate elapsed time, which was only updated when individual shader tasks completed
• Result: The timer appeared to jump forward in chunks rather than updating smoothly every second
• User confirmation: zndxcvbn verified the fix eliminates the jitters

2. Solution Correctness ⚠️ (Mostly Correct, Some Issues)

What the solution does:

1. Changes GetStatsString() to use wall-clock time (currentTime - lastReset) instead of totalTime
2. Adds completionTime atomic to freeze the end timestamp when all tasks finish
3. Moves timing updates under the compilationMutex

Correctness issues found:

✅ Fixes a race condition

The original code had a critical bug:

// OLD CODE - RACE CONDITION
LARGE_INTEGER now;
QueryPerformanceCounter(&now);
totalTime.QuadPart += now.QuadPart - lastCalculation.QuadPart;  // ← Not protected
lastCalculation = now;  // ← Not protected
std::scoped_lock lock(compilationMutex);  // ← Lock acquired AFTER

Multiple threads calling Complete() could race when updating totalTime and lastCalculation, corrupting both values. The new code correctly places these updates inside the mutex.

⚠️ Performance concern: Logging inside mutex

std::scoped_lock lock(compilationMutex);
if (shaderBlob) {
    logger::debug("Compiling Task succeeded: {}", key);  // ← Inside lock
    completedTasks++;
}

The logger::debug() calls happen while holding the lock, which could increase contention if logging involves I/O. The final completion log is correctly moved outside the lock, but per-task logs remain inside.

⚠️ Atomic operation ordering

The completion check reads atomics without proper synchronization:

if (completionTime.load(std::memory_order_relaxed) == 0 && 
    completedTasks + failedTasks >= totalTasks) {  // ← Non-atomic read of two atomics

While completedTasks and failedTasks are atomics, reading both and comparing their sum isn't atomic. Multiple threads could simultaneously pass this check. The completionTime.load() == 0 check mitigates this with a compare-and-set pattern, but it's not bulletproof.

✅ Semantic change is appropriate

Changing from "work time" (totalTime) to "wall-clock time" for display purposes makes sense—users want to see real elapsed time, not accumulated task time.

3. Performance ⚠️ (Acceptable but could be optimized)

Trade-offs:

• Before: Better performance but incorrect (race condition)
• After: Worse performance but correct (everything under one lock)

Specific concerns:

1. Lock contention: Every task completion now acquires the mutex for longer (includes QueryPerformanceCounter + logging)
2. QueryPerformanceCounter cost: This Windows API call is relatively fast (~100ns) but non-zero
3. Mutex serialization: All completion operations are now serialized, reducing parallelism

Mitigation: For a shader compilation workload where individual tasks take milliseconds to seconds, the added mutex overhead is likely negligible compared to compilation time.

Recommendations

---

🏁 Script executed:

#!/bin/bash
# Check if there are patterns of similar timing/threading code that might have the same race condition
rg -nP --type=cpp -C3 'QueryPerformanceCounter.*\n.*(?!std::.*lock).*QuadPart' src/

Unrecoverable error: Stream setup permanently failed: 14 UNAVAILABLE: read ECONNRESET

---

Recommendations

1. Move debug logging outside the mutex (Performance improvement)

// Store info for logging outside the lock
bool succeeded = static_cast<bool>(shaderBlob);
{
    std::scoped_lock lock(compilationMutex);
    
    // Update counters without logging
    if (succeeded) {
        completedTasks++;
    } else {
        failedTasks++;
    }
    
    // ... rest of timing logic ...
}

// Log outside the lock
if (succeeded) {
    logger::debug("Compiling Task succeeded: {}", key);
} else {
    logger::debug("Compiling Task failed: {}", key);
}

2. Consider stronger memory ordering for completion check (Correctness)

While unlikely to cause issues in practice, using memory_order_acquire when loading completionTime would provide stronger guarantees:

if (completionTime.load(std::memory_order_acquire) == 0 && 
    completedTasks + failedTasks >= totalTasks) {
    completionTime.store(now.QuadPart, std::memory_order_release);
    // ...
}

3. Alternative: Use a completion flag instead of timestamp (Simplification)

Since the completion check is already inside the mutex, a simple boolean might be clearer:

bool compilationComplete = false;  // Inside the mutex-protected section

Verdict

Problem identification: ✅ Correct - The PR accurately identified the jitter issue caused by chunked time updates.

Solution correctness: ⚠️ Mostly correct - Fixes a critical race condition but has minor concerns around logging performance and atomic ordering.

Performance: ⚠️ Acceptable - The increased lock contention is a reasonable trade-off for correctness in this workload, though it could be optimized further.

Overall: The PR is safe to merge as it fixes both the reported jitter issue and an underlying race condition. The performance concerns are minor and could be addressed in a follow-up if profiling shows contention issues.

---

🧠 Learnings used

Learnt from: ThePagi
Repo: doodlum/skyrim-community-shaders PR: 1369
File: src/Features/SnowCover.cpp:260-260
Timestamp: 2025-08-05T18:13:03.123Z
Learning: In the skyrim-community-shaders SnowCover feature, the time calculation uses division by 61.0 instead of 60.0 for seconds conversion in the perFrame.Month calculation. The original author ThePagi indicated this was intentional and makes no discernible difference to the snow cover functionality, suggesting it may be related to game-specific timing mechanics or balance considerations.

---

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.