Move instead of copy when committing a SafeFileOutputStream - #2961
Merged
vogella merged 1 commit intoSep 22, 2026
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The bundle version must be incremented for this significant performance change.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Optimizes workspace persistence by moving temporary files instead of copying them during commit.
Changes:
- Tries an atomic move first.
- Falls back to replacing move, then copy.
| File | Description |
|---|---|
resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/localstore/SafeFileOutputStream.java |
Adds move-based commit paths to reduce writes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
|
Interestingly, the double work was introduced in 73c56a3 . Before this commit, the code used sourceFile.renameTo(destinationFile)) . |
Contributor
vogella
force-pushed
the
safe-file-output-stream-move
branch
from
September 21, 2026 22:11
36fbb55 to
722404a
Compare
The commit step copied the temporary file over the target and then deleted it, so every byte was written twice. The workspace save uses this stream for the workspace tree and the .markers and .syncinfo files of every project, so the extra write is paid on every save and every snapshot. A crash during the copy could also leave a partially written target, which the recovery in the constructor never repairs because it only acts when the target is missing. Rename the temporary file atomically instead, and fall back to the previous copy only when the file system cannot move it atomically. Contributes to eclipse-platform#2887 Assisted-by: multiple AI agents and layers of automated tooling 🤖
vogella
force-pushed
the
safe-file-output-stream-move
branch
from
September 21, 2026 23:05
722404a to
5ea80d2
Compare
Contributor
Author
|
Removed the silent fallbacks: only the case where an atomic move isn't supported falls back to the copy, and any other failure is reported to the caller. |
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
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.
SafeFileOutputStream committed by copying the temporary file over the target and deleting it afterwards, so every byte was written twice. The workspace save writes the workspace tree and the per-project .markers and .syncinfo files through this stream, so the duplicate write is paid on every save and every snapshot. The commit now renames the temporary file, falling back to a replacing move and then to the previous copy, which halves the writes and also avoids leaving a partially copied target behind after a crash. The core.resources bundle builds, and SafeFileInputOutputStreamTest, SafeChunkyInputOutputStreamTest and LocalStoreRegressionTests pass locally
Contributes to #2887