Use FileArtifactValue#setContentsProxy for remote repo contents cache#28654
Use FileArtifactValue#setContentsProxy for remote repo contents cache#28654fmeum wants to merge 1 commit intobazelbuild:masterfrom
FileArtifactValue#setContentsProxy for remote repo contents cache#28654Conversation
|
@bazel-io fork 9.1.0 |
github-actions
bot
added
team-Performance
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the remote external repository caching to use FileArtifactValue.createForRemoteFileWithMaterializationData. This enables the FileContentsProxy optimization, which helps avoid unnecessary Skyframe invalidations when remote repository files are materialized locally. The changes involve plumbing the remoteCacheTtl from RemoteOptions to where the FileArtifactValue is created.
While the overall change is good, I found a critical issue in how the cache TTL is handled. A TTL of 0, which should mean 'no limit', is incorrectly treated as an immediate expiration, which could lead to performance degradation or incorrect behavior. I've provided a suggestion to fix this.
src/main/java/com/google/devtools/build/lib/remote/RemoteExternalOverlayFileSystem.java
Show resolved
Hide resolved
…he files Even though expiration times don't matter in Bazel (they aren't honored), supporting the contents proxy optimization avoids Skyframe invalidation when external repo files are materialized later. Along the way ensure that the persistent action cache always recreates remote metadata via `FileArtifactValue.createForRemoteFileWithMaterializationData`. Before this change, such metadata would roundtrip into a `RemoteFileArtifactValue` (without the content proxy optimization) if `expirationTime` is set to `null`.
fmeum
force-pushed
the
file-contents-proxy
branch
from
84a97fc to
7d87090
Compare
There was a problem hiding this comment.
Code Review
This pull request refactors the creation of FileArtifactValue for remote files to consistently use a variant that supports content proxying. This is a valuable optimization that avoids unnecessary Skyframe invalidations when external repository files are materialized. The changes in CompactPersistentActionCache and RemoteExternalOverlayFileSystem are correct and well-implemented. The plumbing of remoteCacheTtl from RemoteOptions through RemoteModule is also done correctly. Overall, this is a solid improvement to Bazel's remote caching capabilities.
tjgq
added
awaiting-PR-merge
2 participants
Even though expiration times don't matter in Bazel (they aren't honored), supporting the contents proxy optimization avoids Skyframe invalidation when external repo files are materialized later.
Along the way ensure that the persistent action cache always recreates remote metadata via
FileArtifactValue.createForRemoteFileWithMaterializationData. Before this change, such metadata would roundtrip into aRemoteFileArtifactValue(without the content proxy optimization) ifexpirationTimeis set tonull.