BabylonJS / BabylonJS/BabylonNative

Shader cache is not invalidated when compiler output changes

Open
#1,799 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
919
Forks
162
Avg merge
1d 15h
Merged PRs (30d)
19

Description

[Filed by Copilot on behalf of @bghgary]

### Summary

`ShaderCacheImpl`'s key describes the shader *source*, but the value it stores depends on the *toolchain* that compiled it. Nothing ties the two together, so a change that alters compiler output for unchanged GLSL silently keeps serving stale entries.

```cpp
ShaderCacheImpl::ShaderHash ShaderCacheImpl::Hash(std::string_view vertexSource, std::string_view fragmentSource)
{
std::string normalizeVertexSource = NormalizeLineEndings(vertexSource);
std::string normalizeFragmentSource = NormalizeLineEndings(fragmentSource);
return {XXH3_64bits(normalizeVertexSource.data(), normalizeVertexSource.size()),
XXH3_64bits(normalizeFragmentSource.data(), normalizeFragmentSource.size())};
}
```

`CACHE_VERSION` exists to cover this, but it is hand-maintained and has been bumped twice in the cache's lifetime, both times for a *format* change rather than a content change:

| Commit | Change |
| --- | --- |
| `dd56a4d0` (#1402) | introduced, `CACHE_VERSION = 1` |
| `ec95b268` (#1598) | `1 -> 2`, precompiled shader support |

### What can change the output without changing the source

- **BabylonNative's own `ShaderCompiler`.** #1796 changes the sampler name written into the bgfx blob and the `UniformStages` key. Because `Program.cpp` resolves uniforms by name (`uniformStages.find(info.name)` and `uniformNameToIndex[info.name]`), a stale entry reproduces the exact bug that PR fixes: the sampler never binds.
- **SPIRV-Cross** (`a512817ddbcd879a3929aef7d1d762871bdf8635`) — the reserved-keyword renaming behind #1796 lives here.
- **glslang** (`284e4301e5a6b44b279635276588db7cdd942624`) — GLSL to SPIR-V.
- **bgfx**, via bgfx.cmake (`6c5515826c428337b7cbc0a5a7cc6d12325ce72b`) — blob layout and uniform table.

That last one is not hypothetical: #1795 moved the bgfx.cmake pin and merged without touching `CACHE_VERSION`.

### Suggested direction

Derive the version instead of hand-maintaining it. CMake already holds every dependency SHA above, so at configure time it can digest them together with a content hash of `Plugins/ShaderCompiler/Source/*` and the selected `GRAPHICS_API`, and `configure_file` the result into a generated header:

```cpp
constexpr uint32_t SHADER_CACHE_VERSION = /* digest(dependency pins + compiler sources + graphics api) */;
```

`CACHE_VERSION` then becomes that constant. Both PRs above would have invalidated automatically, with no author having to notice.

Keeping this as the whole-file version check rather than folding the digest into `Hash()` is deliberate: `Load` already discards the file cleanly on mismatch, whereas a key-based scheme leaves superseded entries in the file forever as unreachable garbage.

### Tradeoffs

- Over-invalidates: a comment-only edit under `Plugins/ShaderCompiler/Source/` costs one full recompile. That is strictly safer than today's under-invalidation and is a one-time startup cost.
- Hash file contents rather than a git SHA, so dirty working trees are covered and configure does not need git.
- For the precompiled shader flow added in #1598, this enforces by construction what is currently convention: a shipped cache is only consumed by a build that would have produced it.

### Note

#1796 should still bump `CACHE_VERSION` by hand; this issue is the follow-up so the next change does not depend on someone noticing.

Contributor guide

Open the contributing guide

Research direction

Start by tracing ShaderCacheImpl's CACHE_VERSION use through Load, then inspect the CMake dependency pins, GRAPHICS_API selection, and Plugins/ShaderCompiler/Source/* inputs. Follow how configure_file generates headers and replace the hand-maintained version with a digest of those inputs. Done means compiler, dependency, source, or graphics API changes produce a mismatch that causes Load to discard the cache.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp
Domain
build-system, compilers, computer-graphics
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.