improvement: Add an option to cache turbine results - #8772
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable Java Turbine caching. The cache stores lowered compilation results in a deterministic JAR, restores them on first compilation, and invalidates stale entries. MBT wiring and integration tests cover restart persistence and dirty Java source precedence. ChangesJava Turine compilation cache
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LSP
participant MbtWorkspaceSymbolProvider
participant TurbineCompiler
participant TurbineCache
participant GitVCS
LSP->>MbtWorkspaceSymbolProvider: initialize workspace
MbtWorkspaceSymbolProvider->>TurbineCompiler: configure cache and dirty Java sources
TurbineCompiler->>TurbineCache: loadFromCache(classpath)
TurbineCache->>GitVCS: getHeadHash(workspace)
GitVCS-->>TurbineCache: Git HEAD key
alt valid cache
TurbineCache-->>TurbineCompiler: cached compilation result
TurbineCompiler->>TurbineCompiler: add dirty Java files to sourcepath
else no valid cache
TurbineCompiler->>TurbineCompiler: compile sources
TurbineCompiler->>TurbineCache: writeCache(result)
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
tgodzik
force-pushed
the
cache-turbine-results
branch
from
August 5, 2026 13:32
d328154 to
c159536
Compare
This priduces a jar in .metals directory that is later read by metals into turbine.
tgodzik
force-pushed
the
cache-turbine-results
branch
from
August 6, 2026 12:10
c159536 to
5e0e70d
Compare
tgodzik
marked this pull request as ready for review
August 6, 2026 12:11
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCache.scala`:
- Around line 101-138: Update
metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCache.scala lines
101-138 and the read/write cache flow around readCache to persist a versioned
cache key covering the source revision and effective JAR classpath, validate it
before returning TurbineCompileResult, and delete mismatched entries. Update
metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala lines
196-235 to compute and provide the expected key when loading the cache, falling
back to compilation when validation fails.
In `@metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scala`:
- Line 110: Update UserConfiguration.toString’s fields list to include
javaTurbineCache.enabled alongside javaTurbineRecompileDelay, so serialized
configuration output exposes the cache setting.
In `@tests/unit/src/test/scala/tests/mbt/TurbineCacheSuite.scala`:
- Around line 189-195: Extend the restart scenario around assertHovers and
doCompileNow with a test-only observable that records Turbine cache loads, then
assert the observable confirms the restarted TurbineCompiler loaded the
unchanged cache rather than recompiling. Keep the existing hover and cache-file
assertions, and ensure the observable is reset or scoped so it specifically
verifies cache reuse after newServer().
🪄 Autofix
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 Plus
Run ID: 59133c64-92f6-47eb-a441-3cb1483987e1
📒 Files selected for processing (8)
metals/src/main/scala/scala/meta/internal/metals/Configs.scalametals/src/main/scala/scala/meta/internal/metals/Directories.scalametals/src/main/scala/scala/meta/internal/metals/MetalsLspService.scalametals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/mbt/MbtWorkspaceSymbolProvider.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCache.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalatests/unit/src/test/scala/tests/mbt/TurbineCacheSuite.scala
odisseus
reviewed
Aug 7, 2026
Contributor
Author
We now add all dirty files to sourcepath, so any changes from the last hash should be picked up. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala (1)
230-235: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDo not restore dirty compilation output by Git HEAD alone.
A compilation with dirty Java sources writes cached classfiles under the unchanged HEAD hash. If the user then reverts those sources and restarts Metals, the worktree is clean, the HEAD hash still matches, and Line 230 restores the stale dirty classfiles without dirty sources to shadow them.
Skip cache writes when relevant compilation inputs are dirty, while retaining the last clean cache entry. Alternatively, include a deterministic worktree fingerprint in the cache key. Add a regression test that compiles dirty Java, reverts it, restarts, and verifies that removed dirty symbols do not resolve.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala` around lines 230 - 235, The Turbine cache key currently permits dirty Java compilation output to be restored after the worktree returns to HEAD. Update the cache read/write flow around isFirstCompile, loadFromCache, and TurbineCompiler.validClasspaths so dirty compilation inputs do not overwrite or restore the last clean cache entry; use a deterministic worktree fingerprint if that is the established approach. Add a regression test covering dirty Java compilation, reverting the changes, restarting, and confirming removed dirty symbols no longer resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@metals/src/main/scala/scala/meta/internal/metals/mbt/MbtWorkspaceSymbolProvider.scala`:
- Around line 160-170: Update the dirty compilation-unit flow around the
`documents.get(status.file)` lookup in the workspace symbol provider to derive
an `IndexedDocument` from the current source input, so untracked Java files and
package moves use current metadata instead of stale or missing indexed
documents. Preserve the existing Git status filtering and compilation-unit
generation, and add restart coverage for both an untracked Java file and a Java
package relocation.
---
Outside diff comments:
In `@metals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scala`:
- Around line 230-235: The Turbine cache key currently permits dirty Java
compilation output to be restored after the worktree returns to HEAD. Update the
cache read/write flow around isFirstCompile, loadFromCache, and
TurbineCompiler.validClasspaths so dirty compilation inputs do not overwrite or
restore the last clean cache entry; use a deterministic worktree fingerprint if
that is the established approach. Add a regression test covering dirty Java
compilation, reverting the changes, restarting, and confirming removed dirty
symbols no longer resolve.
🪄 Autofix
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 Plus
Run ID: f5307473-32b1-48e6-aab7-16bf2c79fdfd
📒 Files selected for processing (7)
metals/src/main/scala/scala/meta/internal/metals/UserConfiguration.scalametals/src/main/scala/scala/meta/internal/metals/mbt/GitVCS.scalametals/src/main/scala/scala/meta/internal/metals/mbt/MbtWorkspaceSymbolProvider.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCache.scalametals/src/main/scala/scala/meta/internal/metals/mbt/TurbineCompiler.scalatests/unit/src/test/scala/tests/UserConfigurationSuite.scalatests/unit/src/test/scala/tests/mbt/TurbineCacheSuite.scala
tgodzik
force-pushed
the
cache-turbine-results
branch
from
August 7, 2026 16:31
ca3586e to
b7e1914
Compare
Contributor
Author
|
Will go ahead and merge it for people to test, since it's under the flag. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
java-turbine-cacheconfiguration setting.