Add caching for source modules - #968
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughSource-module caching adds digest-based cache identity for modules loaded from source, performs cache-first load using both cached source and compiled ChangesSource-based Module Cache
Sequence Diagram(s)sequenceDiagram
participant Client
participant SlangSession
participant FileSystem
participant SlangCompiler
Client->>SlangSession: load_module_from_source(source_code)
activate SlangSession
SlangSession->>SlangSession: compute SHA1 digest
SlangSession->>FileSystem: check cache path for `.slang` + `.slang-module`
alt cache hit
FileSystem-->>SlangSession: return cached files
SlangSession->>SlangCompiler: loadModule(from cached .slang)
SlangCompiler-->>SlangSession: IModule
else cache miss
SlangSession->>SlangCompiler: loadModuleFromSourceString(source_code)
SlangCompiler-->>SlangSession: IModule
SlangSession->>FileSystem: write temp `.slang-*` and rename -> `.slang`
SlangSession->>FileSystem: write temp `.slang-module-*` and rename -> `.slang-module`
end
SlangSession-->>Client: return IModule
deactivate SlangSession
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
Tip 💬 Introducing [Slack Agent](https://www.coderabbit.ai/agent): Turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 👉 Get your free trial and get 200 agent minutes per Slack user (a $50 value). 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. Review rate limit: 0/1 reviews remaining, refill in 8 minutes and 51 seconds.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 the current code and only fix it if needed.
Inline comments:
In `@src/sgl/device/shader.cpp`:
- Around line 1000-1042: The cache-load path currently uses
SGL_CATCH_INTERNAL_SLANG_ERROR around session_data->slang_session->loadModule
which rethrows SlangCompileError and prevents falling back to source; change the
logic so exceptions from loadModule are caught and treated the same as a nullptr
(i.e., do not let the exception escape): wrap the loadModule call (the one
inside the if (std::filesystem::exists(cache_file)) block where
SGL_CATCH_INTERNAL_SLANG_ERROR is used) so that any thrown error is
logged/debugged and loaded_from_cache remains false, allowing the subsequent
loadModuleFromSourceString code (and the later
m_session->_write_source_module_to_cache call) to run as the fallback; keep
existing behavior for a successful slang_module (set loaded_from_cache = true
and log).
🪄 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
Run ID: 227dc70c-b3b9-41aa-9168-2aa78c233c84
📒 Files selected for processing (3)
slangpy/tests/device/test_module_cache.pysrc/sgl/device/shader.cppsrc/sgl/device/shader.h
| bool loaded_from_cache = false; | ||
| if (session_data->cache_enabled && desc.source_digest.has_value()) { | ||
| std::filesystem::path cache_file | ||
| = m_session->_get_source_module_cache_path(desc.module_name, *desc.source_digest); | ||
| if (std::filesystem::exists(cache_file)) { | ||
| SGL_CATCH_INTERNAL_SLANG_ERROR( | ||
| slang_module | ||
| = session_data->slang_session->loadModule(cache_file.string().c_str(), diagnostics.writeRef()); | ||
| ); | ||
| if (slang_module) { | ||
| loaded_from_cache = true; | ||
| log_debug("Loaded source module \"{}\" from cache", desc.module_name); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| SGL_CATCH_INTERNAL_SLANG_ERROR( | ||
| slang_module = session_data->slang_session->loadModuleFromSourceString( | ||
| std::string{desc.module_name}.c_str(), | ||
| desc.path ? desc.path->string().c_str() : nullptr, | ||
| source_str.c_str(), | ||
| diagnostics.writeRef() | ||
| ) | ||
| ); | ||
| if (!slang_module) { | ||
| std::string msg = append_diagnostics( | ||
| fmt::format("Failed to load slang module \"{}\" from source", desc.module_name), | ||
| diagnostics | ||
| if (!loaded_from_cache) { | ||
| // TODO: This is a workaround until we use a Slang release with this fix: | ||
| // https://github.com/shader-slang/slang/pull/10996 | ||
| // Once this is fixed on the Slang side, we can remove this. | ||
| std::string source_str = fmt::format("// {}\n{}", desc.module_name, desc.source.value()); | ||
|
|
||
| SGL_CATCH_INTERNAL_SLANG_ERROR( | ||
| slang_module = session_data->slang_session->loadModuleFromSourceString( | ||
| std::string{desc.module_name}.c_str(), | ||
| desc.path ? desc.path->string().c_str() : nullptr, | ||
| source_str.c_str(), | ||
| diagnostics.writeRef() | ||
| ) | ||
| ); | ||
| throw SlangCompileError(msg); | ||
| if (!slang_module) { | ||
| std::string msg = append_diagnostics( | ||
| fmt::format("Failed to load slang module \"{}\" from source", desc.module_name), | ||
| diagnostics | ||
| ); | ||
| throw SlangCompileError(msg); | ||
| } | ||
|
|
||
| // Write to source module cache. | ||
| if (session_data->cache_enabled && desc.source_digest.has_value()) { | ||
| m_session->_write_source_module_to_cache(slang_module, desc.module_name, *desc.source_digest); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing fallback to source when cache-file loadModule throws — hard failure on stale/corrupt cache
SGL_CATCH_INTERNAL_SLANG_ERROR re-throws any internal Slang exception as SlangCompileError. If the cached .slang-module file is corrupted (e.g., partial write from a previous crash, a Slang version upgrade that changes the binary format), loadModule will throw rather than return nullptr. That exception propagates past the if (!loaded_from_cache) block entirely, so the source fallback is never reached. The user gets an opaque "Internal slang error: ..." message and the only recovery is manually deleting the cache directory.
A null return from loadModule is handled gracefully (loaded_from_cache stays false → source is used). The throw case must be handled the same way.
🛡️ Proposed fix
if (std::filesystem::exists(cache_file)) {
- SGL_CATCH_INTERNAL_SLANG_ERROR(
- slang_module
- = session_data->slang_session->loadModule(cache_file.string().c_str(), diagnostics.writeRef());
- );
- if (slang_module) {
+ try {
+ SGL_CATCH_INTERNAL_SLANG_ERROR(
+ slang_module = session_data->slang_session->loadModule(
+ cache_file.string().c_str(), diagnostics.writeRef()
+ );
+ );
+ } catch (const SlangCompileError& e) {
+ log_warn(
+ "Failed to load source module \"{}\" from cache ({}), falling back to source.",
+ desc.module_name,
+ e.what()
+ );
+ slang_module = nullptr;
+ }
+ if (slang_module) {
loaded_from_cache = true;
log_debug("Loaded source module \"{}\" from cache", desc.module_name);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/sgl/device/shader.cpp` around lines 1000 - 1042, The cache-load path
currently uses SGL_CATCH_INTERNAL_SLANG_ERROR around
session_data->slang_session->loadModule which rethrows SlangCompileError and
prevents falling back to source; change the logic so exceptions from loadModule
are caught and treated the same as a nullptr (i.e., do not let the exception
escape): wrap the loadModule call (the one inside the if
(std::filesystem::exists(cache_file)) block where SGL_CATCH_INTERNAL_SLANG_ERROR
is used) so that any thrown error is logged/debugged and loaded_from_cache
remains false, allowing the subsequent loadModuleFromSourceString code (and the
later m_session->_write_source_module_to_cache call) to run as the fallback;
keep existing behavior for a successful slang_module (set loaded_from_cache =
true and log).
Co-authored-by: Copilot <copilot@github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/sgl/device/shader.cpp (1)
1038-1041:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch cache-load exceptions so source fallback still runs.
At Line 1038,
SGL_CATCH_INTERNAL_SLANG_ERRORmay throw on stale/corrupt cache artifacts; that exits before Line 1049, so source fallback is skipped. This should be treated as a cache miss (loaded_from_cache = false) rather than a hard failure.Suggested fix
if (std::filesystem::exists(cache_source_file) && std::filesystem::exists(cache_binary_file)) { - SGL_CATCH_INTERNAL_SLANG_ERROR( - slang_module = session_data->slang_session - ->loadModule(cache_source_file.string().c_str(), diagnostics.writeRef()); - ); + try { + SGL_CATCH_INTERNAL_SLANG_ERROR( + slang_module = session_data->slang_session + ->loadModule(cache_source_file.string().c_str(), diagnostics.writeRef()); + ); + } catch (const SlangCompileError& e) { + log_warn( + "Failed to load source module \"{}\" from cache ({}). Falling back to source.", + desc.module_name, + e.what() + ); + slang_module = nullptr; + } if (slang_module) { loaded_from_cache = true; log_debug("Loaded source module \"{}\" from cache", desc.module_name); } }You can verify this path and macro behavior with:
#!/bin/bash set -euo pipefail # 1) Inspect macro definition/usage to confirm it can throw. rg -n -C3 'SGL_CATCH_INTERNAL_SLANG_ERROR' --type=cpp --type=h # 2) Inspect the source-cache load + fallback block in shader.cpp. rg -n -C6 'cache_source_file|string\(\)\.c_str\(\)|loaded_from_cache|loadModuleFromSourceString' --type=cppAlso applies to: 1049-1079
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/sgl/device/shader.cpp` around lines 1038 - 1041, The cache-load attempt using SGL_CATCH_INTERNAL_SLANG_ERROR around the call that assigns slang_module = session_data->slang_session->loadModule(...) can throw and currently aborts before the source-fallback runs; change the logic so any exception from that macro is caught and treated as a cache miss: wrap the cached load/assignment in a try/catch that on exception logs the error, sets loaded_from_cache = false, clears/sets slang_module appropriately, and allows execution to continue to the existing source fallback (e.g., the loadModuleFromSourceString path) so stale/corrupt cache artifacts don’t prevent fallback.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/sgl/device/shader.cpp`:
- Around line 1038-1041: The cache-load attempt using
SGL_CATCH_INTERNAL_SLANG_ERROR around the call that assigns slang_module =
session_data->slang_session->loadModule(...) can throw and currently aborts
before the source-fallback runs; change the logic so any exception from that
macro is caught and treated as a cache miss: wrap the cached load/assignment in
a try/catch that on exception logs the error, sets loaded_from_cache = false,
clears/sets slang_module appropriately, and allows execution to continue to the
existing source fallback (e.g., the loadModuleFromSourceString path) so
stale/corrupt cache artifacts don’t prevent fallback.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f1a54b5-3462-40eb-b447-9c0088678703
📒 Files selected for processing (2)
src/sgl/device/shader.cppsrc/sgl/device/shader.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/sgl/device/shader.h
Co-authored-by: Copilot <copilot@github.com>
2 participants
This doesn't currently work due to limitations of loading precompiled modules in Slang.
See shader-slang/slang#11036
Summary by CodeRabbit
New Features
Tests