revert: perf(shader-cache): hybrid-CPU scheduling and LPT dispatch#2039
revert: perf(shader-cache): hybrid-CPU scheduling and LPT dispatch#2039alandtse wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR simplifies the shader compilation system by removing priority-based task scheduling, diagnostic telemetry APIs, and complex thread management. It replaces Windows-specific performance core detection with standard Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/ShaderCache.h (2)
448-450:⚠️ Potential issue | 🟠 MajorThread pool initialization disconnected from configurable thread count.
The
compilationPool{}default initialization ignorescompilationThreadCount. The calculated thread count (line 448) and the user-configurable slider have no effect on the actual pool size. This should be addressed in the constructor (see related comment insrc/ShaderCache.cpp).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ShaderCache.h` around lines 448 - 450, The thread pool is default-constructed (compilationPool{}) so the computed compilationThreadCount and slider have no effect; update the ShaderCache constructor in ShaderCache.cpp to initialize compilationPool with the intended thread count (use the compilationThreadCount variable or the user-configurable value) and similarly ensure any background pool uses backgroundCompilationThreadCount when created; locate symbols compilationThreadCount, backgroundCompilationThreadCount and compilationPool and pass the desired integer into the pool constructor so the configured sizes are applied at initialization.
675-675:⚠️ Potential issue | 🔴 CriticalEnsure null-safety when accessing
managementThread.This handle is initialized to
nullptrand only assigned withinManageCompilationSet(). The destructor cleanup code must guard against null before calling Windows API functions on this handle (see related comment insrc/ShaderCache.cpp).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ShaderCache.h` at line 675, The destructor currently assumes managementThread is valid; update the destructor cleanup to null-check managementThread before calling any Windows API on it (e.g., WaitForSingleObject, CloseHandle) and only perform signaling/wait/close when managementThread != nullptr; ensure you also reset managementThread to nullptr after closing and keep ManageCompilationSet() as the sole initializer for this handle.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ShaderCache.cpp`:
- Around line 1929-1934: The destructor is calling GetThreadId,
WaitForSingleObject, TerminateThread, and CloseHandle on managementThread
without validating it; add a guard that checks managementThread is non-null (and
not INVALID_HANDLE_VALUE) before calling compilationPool.wait_for(...) and any
thread APIs. Update the block around compilationPool.wait_for(...) to first
verify managementThread (the handle set in ManageCompilationSet) is valid, and
only then call GetThreadId(managementThread),
WaitForSingleObject(managementThread,...),
TerminateThread(managementThread,...), and CloseHandle(managementThread);
otherwise skip those calls to avoid undefined behavior.
- Line 2349: The detached task created with compilationPool.detach_task that
calls listener->processQueue() can dereference a freed listener because
StopFileWatcher() sets listener = nullptr without clearing useFileWatcher;
update StopFileWatcher() to set useFileWatcher = false before releasing/freeing
listener so the loop guarded by UseFileWatcher() inside processQueue() exits
cleanly, ensuring the detached task won't access listener after destruction
(adjust StopFileWatcher(), and verify any shutdown wait_for(1000ms) logic in the
destructor still gracefully waits for processQueue() to finish).
- Around line 2314-2316: The ShaderCache constructor logs compilationThreadCount
but default-initializes compilationPool, so the pool size ignores the configured
value; update the ShaderCache constructor to initialize compilationPool with the
configured thread count (use compilationThreadCount) rather than default
construction, ensure the detached compilation task still uses compilationPool
(keep the call to compilationPool.detach_task([...]) and
ManageCompilationSet(token)), and verify throttling in WaitTake() (which
compares get_tasks_total() to compilationThreadCount) now matches the actual
pool size; this aligns the logged value, the "Compiler Threads" slider, and the
get_tasks_total() checks.
---
Duplicate comments:
In `@src/ShaderCache.h`:
- Around line 448-450: The thread pool is default-constructed
(compilationPool{}) so the computed compilationThreadCount and slider have no
effect; update the ShaderCache constructor in ShaderCache.cpp to initialize
compilationPool with the intended thread count (use the compilationThreadCount
variable or the user-configurable value) and similarly ensure any background
pool uses backgroundCompilationThreadCount when created; locate symbols
compilationThreadCount, backgroundCompilationThreadCount and compilationPool and
pass the desired integer into the pool constructor so the configured sizes are
applied at initialization.
- Line 675: The destructor currently assumes managementThread is valid; update
the destructor cleanup to null-check managementThread before calling any Windows
API on it (e.g., WaitForSingleObject, CloseHandle) and only perform
signaling/wait/close when managementThread != nullptr; ensure you also reset
managementThread to nullptr after closing and keep ManageCompilationSet() as the
sole initializer for this handle.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a915e8d0-18e0-466f-80d0-461e291a6aa6
📒 Files selected for processing (10)
src/Features/UnifiedWater/WaterCache.cppsrc/Menu/AdvancedSettingsRenderer.cppsrc/Menu/OverlayRenderer.cppsrc/Menu/SettingsTabRenderer.cppsrc/ShaderCache.cppsrc/ShaderCache.hsrc/Utils/Format.cppsrc/Utils/Format.hsrc/Utils/WinApi.cppsrc/Utils/WinApi.h
💤 Files with no reviewable changes (5)
- src/Utils/WinApi.h
- src/Utils/Format.cpp
- src/Menu/SettingsTabRenderer.cpp
- src/Utils/WinApi.cpp
- src/Utils/Format.h
| if (!compilationPool.wait_for(std::chrono::milliseconds(1000))) { | ||
| logger::info("Tasks still running despite request to stop; killing management thread {}!", GetThreadId(managementHandle)); | ||
| WaitForSingleObject(managementHandle, 1000); | ||
| TerminateThread(managementHandle, 0); | ||
| logger::info("Tasks still running despite request to stop; killing thread {}!", GetThreadId(managementThread)); | ||
| WaitForSingleObject(managementThread, 1000); | ||
| TerminateThread(managementThread, 0); | ||
| CloseHandle(managementThread); | ||
| } |
There was a problem hiding this comment.
Potential crash when managementThread is null or invalid.
The destructor calls GetThreadId(managementThread), WaitForSingleObject, TerminateThread, and CloseHandle without first checking if managementThread is valid. Per the relevant code snippet from src/ShaderCache.h:675, managementThread is initialized to nullptr and is only assigned in ManageCompilationSet(). If compilation was never started, these calls will operate on a null handle, causing undefined behavior or crashes.
🐛 Proposed fix to guard against null handle
if (!compilationPool.wait_for(std::chrono::milliseconds(1000))) {
- logger::info("Tasks still running despite request to stop; killing thread {}!", GetThreadId(managementThread));
- WaitForSingleObject(managementThread, 1000);
- TerminateThread(managementThread, 0);
- CloseHandle(managementThread);
+ if (managementThread != nullptr) {
+ logger::info("Tasks still running despite request to stop; killing thread {}!", GetThreadId(managementThread));
+ WaitForSingleObject(managementThread, 1000);
+ TerminateThread(managementThread, 0);
+ CloseHandle(managementThread);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ShaderCache.cpp` around lines 1929 - 1934, The destructor is calling
GetThreadId, WaitForSingleObject, TerminateThread, and CloseHandle on
managementThread without validating it; add a guard that checks managementThread
is non-null (and not INVALID_HANDLE_VALUE) before calling
compilationPool.wait_for(...) and any thread APIs. Update the block around
compilationPool.wait_for(...) to first verify managementThread (the handle set
in ManageCompilationSet) is valid, and only then call
GetThreadId(managementThread), WaitForSingleObject(managementThread,...),
TerminateThread(managementThread,...), and CloseHandle(managementThread);
otherwise skip those calls to avoid undefined behavior.
| logger::debug("ShaderCache initialized with {} compiler threads", (int)compilationThreadCount); | ||
| compilationPool.detach_task([this, token = ssource.get_token()] { ManageCompilationSet(token); }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how BS::thread_pool<> is initialized and whether reset() is available
rg -n "thread_pool" --type cpp -C 3 | head -50
# Check if reset() method exists in the thread pool header
fd -e hpp "thread_pool" | xargs grep -n "reset" 2>/dev/null | head -20Repository: doodlum/skyrim-community-shaders
Length of output: 2600
🏁 Script executed:
# Find the BS_thread_pool.hpp file
find . -name "*thread_pool*" -type f
# Check what methods are available on BS::thread_pool<>
rg -n "reset\(" --type hpp | grep -i thread
# Look for how pool is reinitialized elsewhere in the codebase
rg -n "compilationPool" --type cpp -B 2 -A 2Repository: doodlum/skyrim-community-shaders
Length of output: 2751
🏁 Script executed:
# Find BS_thread_pool.hpp
find . -path "*/include/*" -name "*thread*pool*" -o -name "*BS_thread*"
# Check what method is available by looking at other usages
rg -n "get_tasks_total|wait_for|detach_task" --type cpp | head -30
# Try to find the header in include directories
fd "thread_pool" --type fRepository: doodlum/skyrim-community-shaders
Length of output: 739
🏁 Script executed:
# Check for external dependencies and include paths
rg -n "#include.*BS_thread_pool" --type cpp -A 2 -B 2
# Look for vcpkg or other dependency management
find . -name "vcpkg*" -o -name "conanfile*" -o -name "CMakeLists.txt" | head -5
# Check if there are any build scripts that might reference the library
cat CMakeLists.txt 2>/dev/null | grep -i "thread_pool" || echo "No CMakeLists.txt or no thread_pool reference"Repository: doodlum/skyrim-community-shaders
Length of output: 1104
🌐 Web query:
Bshoshany thread pool reset() method API documentation
💡 Result:
The BS::thread_pool class from the bshoshany/thread-pool library (latest version v5.1.0 as of 2026-01-03) provides several overloads of the reset member function to safely change the number of threads in the pool on-the-fly. These functions wait for currently running tasks to complete (or all tasks if pausing is not enabled), destroy the existing threads, create a new pool, and resume queued tasks. Function signatures and descriptions (from BS_thread_pool.hpp source and README): - void reset: Resets the pool to the default number of threads (std::thread::hardware_concurrency). Waits for all tasks if pausing disabled; otherwise only running tasks. - void reset(const std::size_t num_threads): Resets to a specified number of threads. - void reset(std::function init_task): Resets to default threads with a new thread initialization function. - void reset(const std::size_t num_threads, std::function init_task): Resets to specified threads with new init function. Internally calls wait then reset_pool to rebuild the pool. Paused state is preserved if pausing enabled (via BS_THREAD_POOL_ENABLE_PAUSE). Example usage: BS::thread_pool pool(4); // 4 threads pool.reset(8); // Change to 8 threads
Citations:
- 1: https://github.com/bshoshany/thread-pool/blob/master/README.md
- 2: https://github.com/bshoshany/thread-pool
- 3: https://arxiv.org/html/2105.00613v4
- 4: https://github.com/ElsevierSoftwareX/SOFTX-D-22-00214
- 5: https://raw.githubusercontent.com/bshoshany/thread-pool/master/include/BS_thread_pool.hpp
Thread pool ignores compilationThreadCount setting.
The constructor logs compilationThreadCount as the number of compiler threads, but compilationPool{} is default-initialized, which uses std::thread::hardware_concurrency() internally. This means:
- The log message is misleading—the actual pool size may differ from what's logged
- The user-configurable "Compiler Threads" slider in
src/Menu/AdvancedSettingsRenderer.cpp:126has no effect on compilation parallelism - The throttling logic in
WaitTake()(line 2788) comparesget_tasks_total()againstcompilationThreadCount, creating a mismatch
🐛 Proposed fix to initialize pool with configured thread count
+ compilationPool.reset(compilationThreadCount);
logger::debug("ShaderCache initialized with {} compiler threads", (int)compilationThreadCount);
compilationPool.detach_task([this, token = ssource.get_token()] { ManageCompilationSet(token); });📝 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.
| logger::debug("ShaderCache initialized with {} compiler threads", (int)compilationThreadCount); | |
| compilationPool.detach_task([this, token = ssource.get_token()] { ManageCompilationSet(token); }); | |
| } | |
| compilationPool.reset(compilationThreadCount); | |
| logger::debug("ShaderCache initialized with {} compiler threads", (int)compilationThreadCount); | |
| compilationPool.detach_task([this, token = ssource.get_token()] { ManageCompilationSet(token); }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ShaderCache.cpp` around lines 2314 - 2316, The ShaderCache constructor
logs compilationThreadCount but default-initializes compilationPool, so the pool
size ignores the configured value; update the ShaderCache constructor to
initialize compilationPool with the configured thread count (use
compilationThreadCount) rather than default construction, ensure the detached
compilation task still uses compilationPool (keep the call to
compilationPool.detach_task([...]) and ManageCompilationSet(token)), and verify
throttling in WaitTake() (which compares get_tasks_total() to
compilationThreadCount) now matches the actual pool size; this aligns the logged
value, the "Compiler Threads" slider, and the get_tasks_total() checks.
| capturedListener->fileWatcherThread = std::jthread([capturedListener]() { | ||
| capturedListener->processQueue(); | ||
| }); | ||
| compilationPool.detach_task([this] { listener->processQueue(); }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how processQueue() handles shutdown
ast-grep --pattern $'void UpdateListener::processQueue() {
$$$
}'
# Check if UseFileWatcher is checked properly in the loop
rg -n "UseFileWatcher" src/ShaderCache.cpp -A 2 -B 2Repository: doodlum/skyrim-community-shaders
Length of output: 1325
🏁 Script executed:
#!/bin/bash
# Find StopFileWatcher implementation
rg -n "StopFileWatcher" src/ShaderCache.cpp -A 10
# Find the destructor and its shutdown sequence
rg -n "~ShaderCache|~UpdateListener" src/ShaderCache.cpp -A 15Repository: doodlum/skyrim-community-shaders
Length of output: 1738
🏁 Script executed:
#!/bin/bash
# Find UpdateListener::processQueue implementation
rg -n "processQueue()" src/ShaderCache.cpp -A 30 | head -50
# Also check what members processQueue accesses
rg -n "queue\.empty\|actionMutex|this->" src/ShaderCache.cpp -B 2 -A 2 | grep -A 5 -B 5 "processQueue"Repository: doodlum/skyrim-community-shaders
Length of output: 1834
Fix use-after-free: listener is freed while detached task may still execute.
The detached task at line 2349 calls listener->processQueue(), capturing this (the ShaderCache). However, StopFileWatcher() sets listener = nullptr without setting useFileWatcher = false. Inside processQueue() (line 3011 onwards), the code accesses actionMutex and other members of the now-null listener object. The while (cache->UseFileWatcher()) loop won't exit because UseFileWatcher() still returns true—only the listener pointer was freed.
The timeout in the destructor (wait_for(1000ms)) provides only partial safety. If the detached task exceeds the timeout, it will attempt to dereference the null listener pointer, causing a crash.
Solution: Set useFileWatcher = false in StopFileWatcher() before freeing the listener, ensuring the loop exits promptly and safely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/ShaderCache.cpp` at line 2349, The detached task created with
compilationPool.detach_task that calls listener->processQueue() can dereference
a freed listener because StopFileWatcher() sets listener = nullptr without
clearing useFileWatcher; update StopFileWatcher() to set useFileWatcher = false
before releasing/freeing listener so the loop guarded by UseFileWatcher() inside
processQueue() exits cleanly, ensuring the detached task won't access listener
after destruction (adjust StopFileWatcher(), and verify any shutdown
wait_for(1000ms) logic in the destructor still gracefully waits for
processQueue() to finish).
|
May not be needed but queued up in case |
1 participant
Reverts #2021
This wasn't updated for some reason. Reverting for now.
Summary by CodeRabbit
Removed Features
Refactor