Cache computed target build graphs across repeated builds#1124
Cache computed target build graphs across repeated builds#1124jverkoey wants to merge 7 commits into
Conversation
owenv
left a comment
There was a problem hiding this comment.
We may eventually want to fold more of this into the build description itself, but I think an in-memory cache of the TargetBuildGraph makes sense. However, I think some more preparatory refactoring might be needed to help ensure the caching is correct, I left more detailed comments below. I think we'll also need to add some tests for this functionality
| /// index preparation vs normal build). A multi-entry cache ensures | ||
| /// these don't evict each other. | ||
| /// | ||
| /// The cache is static (process-level) because `WorkspaceContext` is |
There was a problem hiding this comment.
All caches should be at least Session-scoped because in the context of an IDE/BSP/etc. the build service may regularly open and close unrelated sessions. Instead of working around the WorkspaceContext recreation, we should look at being less aggressive about tearing it down unnecessarily if we're going to do more caching at this layer.
There was a problem hiding this comment.
Agreed. The cached Target objects use reference identity, so serving them across sessions with different WorkspaceContexts would be incorrect. In practice cross-session signature collisions are unlikely (PIF signatures include all target GUIDs and project paths), but it's not safe in general.
Deferred for now since it requires threading a cache instance through the call chain (PlanningOperation, DependencyGraphMessages, CleanOperation, etc). Session seems like the right owner since it already manages BuildDescriptionManager. Open to guidance on where you'd want this to live.
There was a problem hiding this comment.
I'm more concerned about ensuring cached dependency graphs don't persist in memory when a session is closed but the process is continuing to run builds for other sessions.
| public enum TargetBuildGraphCache { | ||
| /// The data we cache — everything needed by the | ||
| /// `TargetBuildGraph` memberwise init except the live context | ||
| /// objects (workspaceContext, buildRequest, buildRequestContext). |
There was a problem hiding this comment.
This comment is incorrectly referring to the build request as context, we should consider it a primary input to the computation
There was a problem hiding this comment.
Agreed, the resolver reads inputs through context objects that aren't reflected in the signature. Deferred since the fix requires refactoring the resolver to declare explicit inputs. Low practical risk within a single Xcode session but not correct in general.
| case .dependencyGraph: | ||
| hasher.combine("depgraph") | ||
| } | ||
|
|
There was a problem hiding this comment.
This cache key computation is missing at least a few important components, like the signature of xcconfig files loaded while computing settings in the dependency resolver, user preferences, etc.
I think I'd recomment first refactoring the dependency resolver to eliminate direct access to the buildRequestContext and workspaceContext in favor of finer-grained inputs. This should make it easier to audit the inputs they're indirectly introducing today and ensure the cache key is complete.
There was a problem hiding this comment.
Agreed, the resolver reads inputs through context objects that aren't reflected in the signature. Deferred since the fix requires refactoring the resolver to declare explicit inputs. Low practical risk within a single Xcode session but not correct in general.
|
Thank you for the feedback! While I'm working on the feedback I shared #1111 (comment) which includes a few Instruments runs that might be of use for understanding the dragon I'm trying to slay here.. |
Xcode issues multiple TargetBuildGraph requests per build action with different parameters (dependency graph vs build, index prep vs normal). For large workspaces (2000+ targets), computeGraph() takes 7-37s per call. Without caching, this cost is paid on every build even when nothing has changed. Add a process-level multi-entry cache keyed by a signature of the normalized PIF workspace signature, build parameters, and request flags. Each distinct request type gets its own cache slot (up to 8 entries). The cache is static because WorkspaceContext is recreated on every PIF transfer even when nothing has changed. On cache hit, unapproved target diagnostics are re-emitted to preserve correctness.
- Rename CachedTopology → CachedDependencyGraph - Remove leading underscores from private properties - Replace full-reset eviction with LRU (track lastAccess per entry) - Skip caching for prepareForIndexing (low-QoS, large, rarely reused) - Cache all diagnostics during resolution via DiagnosticCollectingDelegate, re-emit on cache hit. Don't cache graphs where errors were emitted. - Make DependencyScope and Purpose Hashable, replacing inline switches - Don't sort build targets in signature (preserve input order) - Don't normalize workspace signature (use as-is for reference safety) - Convert caching init to static factory: TargetBuildGraph.cached(...)
6245fe9 to
59f080e
Compare
|
All feedback addressed in the latest force-push. Summary: Implemented:
Remaining work (deferred):
Happy to tackle either of these in a follow-up or in this PR if you'd prefer. |
|
Moving this to a draft state because I discovered a critical bug in this implementation where changing a file doesn't result in a new build being kicked off. |
The cached dependency graph stores live ConfiguredTarget objects whose Target references use reference identity (ObjectIdentifier) for hash/equality. When the PIF is re-transferred, IncrementalPIFLoader may create new Target objects even when the content is unchanged. The workspace signature (content-based) stays the same, causing a cache hit that serves stale Target references — downstream dictionary lookups using these references silently fail, skipping recompilation of changed source files. Fix: include ObjectIdentifier(workspaceContext.workspace) in the cache signature. Same Workspace instance (reused within a session) produces a cache hit. New Workspace instance (PIF re-transferred) produces a cache miss, which is correct because Target references are tied to the Workspace instance lifetime.
|
Pushed a fix for a cache invalidation bug I discovered during testing: Bug: The cached dependency graph stores live Fix: Include This also partially addresses the session-scoping feedback — within a single session the |
When Xcode re-transfers the PIF after source-only changes, the workspace object identity changes but the graph structure is identical. Instead of recomputing the full dependency graph (~9.5s for large projects), detect content-signature matches and remap all Target references to point at the new workspace's objects. The remap bails out if any cached Target GUID is missing from the new workspace, falling through to a full recompute for structural changes.
New commit: Remap cached target graph on PIF re-transferI've pushed a follow-up commit that addresses the performance cost of the workspace identity fix (91afdfa). ProblemThe SolutionWhen the full signature (with workspace identity) misses, check if any cached entry has the same content signature (everything except workspace identity). If found, remap all Target references in the cached graph to point at the new workspace's Target objects via New methods on
The remap path in Safety
Every Target reference in the remapped graph comes from Confirmed with Xcode iterative buildsTested with 3 consecutive builds in Xcode where only a single source file was changed between each build. Instruments Time Profiler traces confirm:
TestsAdded
|
owenv
left a comment
There was a problem hiding this comment.
I left a few additional comments, but at a high level I think we probably need to take a different approach to introducing caching of the dependency graph than what is currently in this PR. The current soundness holes will lead to incorrect incremental builds in real-world projects, for example, when modifying OTHER_LDFLAGS in an xcconfig to introduce a new target dependency.
I think my suggested approach would be something like:
- Refactor PIF loading to reduce unnecessary invalidation of model objects. This eliminates the need for fragile remapping of cached graphs in addition to potentially speeding up null builds a bit.
- Extract the workspaceContext from target dependency resolution. This is largely only used for access to the project model objects, user preferences, and the SDKRegistry. The first two can be modeled as explicit inputs to the computation, and the third can largely be treated as immutable for the lifetime of the service process.
- Extract the buildRequestContext from target dependency resolution. This is largely used for settings computation, which might introduce additional inputs to target dependency resolution, especially of implicit dependencies. Currently, changes to dependencies triggered by settings changes won't always correctly invalidate the cached graph.
- Eliminate feature flags which impact the dependency graph or move to modeling them as explicit inputs.
- Introduce a simpler in-memory cache tied to the lifetime of the session and cleared upon cleaning which explicitly tracks all of the inputs and does not attempt to remap stale project model objects.
There was a problem hiding this comment.
This seems to have been accidentally included in the history
| /// index preparation vs normal build). A multi-entry cache ensures | ||
| /// these don't evict each other. | ||
| /// | ||
| /// The cache is static (process-level) because `WorkspaceContext` is |
There was a problem hiding this comment.
I'm more concerned about ensuring cached dependency graphs don't persist in memory when a session is closed but the process is continuing to run builds for other sessions.
| /// produces large dependency graphs that are rarely reused — | ||
| /// the memory overhead of caching them is not worth it. | ||
| static func shouldSkipCache(buildCommand: BuildCommand) -> Bool { | ||
| buildCommand.isPrepareForIndexing |
There was a problem hiding this comment.
My earlier comment was suggesting looking at the QoS property itself rather than hardcoding in a special case for indexing.
| } | ||
|
|
||
| /// Store a computed dependency graph for the given signature. | ||
| static func store(signature: Int, graph: CachedDependencyGraph) { |
There was a problem hiding this comment.
I'd prefer we use existing cache types instead of reimplementing this from scratch, a lot of the surrounding code is redundant
| /// The data we cache — everything needed by the | ||
| /// `TargetBuildGraph` memberwise init except the live context | ||
| /// objects (workspaceContext, buildRequest, buildRequestContext). | ||
| package struct CachedDependencyGraph: @unchecked Sendable { |
| /// Returns nil if any cached Target GUID is not found in the new | ||
| /// workspace, which means the PIF structure changed and a full | ||
| /// rebuild is needed. | ||
| package static func remapGraph( |
There was a problem hiding this comment.
This remapping feels very fragile and error-prone. I think the underlying issue is best addressed by the PIF transfer infrastructure rather than attempting to work around it here.
|
|
||
| // Build command affects the early-return for | ||
| // assembly/preprocessor. BuildCommand has associated values | ||
| // that prevent auto-Hashable, so we hash only the |
| @@ -0,0 +1,51 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
This file is needs to be added to CMakeLists.txt
| // FIXME: Report cycles via the delegate. | ||
| // | ||
| /// Construct a new graph for the given build request. | ||
| /// Construct a new dependency graph with caching. |
There was a problem hiding this comment.
A lot of these comments are just repeating information from comments elsewhere
| @@ -0,0 +1,722 @@ | |||
| //===----------------------------------------------------------------------===// | |||
There was a problem hiding this comment.
These tests currently fail to compile, and only cover the signature computation itself. I think this change needs more comprehensive testing of incremental builds which tigger hits/misses in the cache and verify rebuilds (or not)

Problem
TargetBuildGraph.initcallscomputeGraph()on every build, even when nothing inthe workspace has changed. For large workspaces this is expensive — in our monorepo
(2100+ targets) Instruments traces showed
computeGraph()taking 7–37 seconds percall, with 83% of the time spent in the recursive
addDependenciestraversal.Xcode also issues multiple graph requests per build action with different
parameters (dependency graph vs actual build, index preparation vs normal build),
compounding the cost. In our workspace each no-change build was spending ~42 seconds
in
computeGraph()across all requests.Solution
Add a process-level multi-entry cache (
TargetBuildGraphCache) that stores computedgraph topologies keyed by a signature of:
useImplicitDependencies,useParallelTargets,skipDependencies,dependencyScope,buildCommand).buildvs.dependencyGraph)The cache is static (process-level) because
WorkspaceContextis recreated on everyPIF transfer, even when nothing has changed. Multiple entries (up to 8) are stored so
that Xcode's different request types don't evict each other.
On cache hit, unapproved target diagnostics are re-emitted to preserve correctness.
Measurements
Tested on a 2100+ target monorepo (5159 configured targets in the full graph).
Each no-change build triggers two graph requests:
The first build after launch populates the cache (cold miss); every subsequent
no-change build hits. The cache is automatically invalidated when the PIF workspace
signature changes (i.e. when the project structure actually changes).