Repository navigation
perf: Use per-output-base lifecycle locking for the Bazel Remote Output Service - #47
Conversation
|
Though I agree we need to solve this, I am not entirely convinced that we need to do something as complex as proposed in the PR. Can't we just add a field like this? diff --git pkg/filesystem/virtual/bazel_output_service_directory.go pkg/filesystem/virtual/bazel_output_service_directory.go
index aaaa0f7..a2af38d 100644
--- pkg/filesystem/virtual/bazel_output_service_directory.go
+++ pkg/filesystem/virtual/bazel_output_service_directory.go
@@ -39,6 +39,10 @@ type outputPathState struct {
next *outputPathState
cookie uint64
outputBaseID path.Component
+
+ // If set, one or more StartBuild, FinalizeBuild, or Clean
+ // operations are running.
+ busy <-chan struct{}
}
// BazelOutputServiceDirectory is FUSE directory that acts as theAs in, normally it's set to |
|
Or maybe we don't even need to provide any blocking? Just add a |
|
@EdSchouten yea, that would definitely be simpler and easier to read. However, that would not guard against a FinalizeBuild starting while an existing concurrent operation was still in progress, it would just block new concurrent operations. Additionally, we couldn't hold a StartBuild until the prior FinalizeBuild has completed. The spec isn't super clear me.. do you think its required to guard against these edge cases? We'd be relying on Bazel call semantics otherwise I suppose. |
You mean while some StageArtifact is running? Sure. But we don't protect against that right now either, so that's likely not an issue.
I think we would be able to do that, right? Cheap mockup: diff --git pkg/filesystem/virtual/bazel_output_service_directory.go pkg/filesystem/virtual/bazel_output_service_directory.go
index aaaa0f7..5ad65dd 100644
--- pkg/filesystem/virtual/bazel_output_service_directory.go
+++ pkg/filesystem/virtual/bazel_output_service_directory.go
@@ -39,6 +39,10 @@ type outputPathState struct {
next *outputPathState
cookie uint64
outputBaseID path.Component
+
+ // If set, one or more StartBuild, FinalizeBuild, or Clean
+ // operations are running.
+ busy <-chan struct{}
}
// BazelOutputServiceDirectory is FUSE directory that acts as the
@@ -291,11 +295,22 @@ func (d *BazelOutputServiceDirectory) StartBuild(ctx context.Context, request *b
return nil, err
}
+TryStart:
d.lock.Lock()
state, ok := d.buildIDs[request.BuildId]
if !ok {
state, ok = d.outputBaseIDs[outputBaseID]
if ok {
+ if ch := state.busy; ch != nil {
+ d.lock.Unlock()
+ select {
+ case <-ch:
+ goto TryStart
+ case <-ctx.Done():
+ return nil, util.StatusFromContext(ctx)
+ }
+ }
+
if buildState := state.buildState; buildState != nil {
// A previous build is running that wasn't
// finalized properly. Forcefully finalize it.
Yeah. Those things haven't really been taken into consideration, because we generally assume that all RPCs happen sequentially, except for BatchStat/StageArtifact which may happen in parallel with each other, but not other RPCs. |
Use a per-output-base busy channel to serialize StartBuild, FinalizeBuild, and Clean while allowing unrelated output bases to progress. Keep slow restoration and finalization outside the global registry mutex. Reject new StageArtifacts and BatchStat requests while a base is busy. Preserve duplicate-finalizer waiting and stale-build identity checks, and honor cancellation before claiming lifecycle ownership. Add lifecycle contention tests covering waiting, cancellation, unrelated base progress, and stage/stat admission.
1ed5246 to
792ec0c
Compare
|
Thanks for the guidance, @EdSchouten I've made the requested changes! I would love your thoughts :) |
| if err := ctx.Err(); err != nil { | ||
| return nil, status.FromContextError(err).Err() | ||
| } | ||
|
|
||
| d.lock.Lock() | ||
| if err := ctx.Err(); err != nil { | ||
| d.lock.Unlock() | ||
| return nil, status.FromContextError(err).Err() | ||
| } |
There was a problem hiding this comment.
Why would this be necessary? Can't we just:
return nil, util.StatusFromContext(ctx)in case <-ctx.Done() triggers?
| outputBaseIDs map[path.Component]*outputPathState | ||
| buildIDs map[string]*outputPathState | ||
| outputPaths outputPathState | ||
| busyBases map[path.Component]chan struct{} |
There was a problem hiding this comment.
busyOutputBaseIDs, to make clear that its keys are the same as those of outputBaseIDs?
I suspect that it could also be a map[path.Component]<-chan struct{}, right?
There was a problem hiding this comment.
Great suggestion, updated!
| return nil, status.FromContextError(err).Err() | ||
| } | ||
|
|
||
| d.lock.Lock() |
There was a problem hiding this comment.
Given that this is implemented using a separate map (which you likely need because outputPathState is only created after reloading the contents of an output path), do we really need to use the same lock here? Maybe a bit cleaner to add another mutex to BazelOutputServiceDirectory?
busyOutputBaseIDsLock sync.Mutex
busyOutputBaseIDs map[path.Component]<-chan struct{}There was a problem hiding this comment.
Yea, I thought it would be simpler to have 1 lock, but you are right its more readable and clearer to have a dedicated on for this map. Updated!
| return nil, nil, status.Error(codes.FailedPrecondition, "Build ID is not associated with any running build") | ||
| } | ||
| if _, ok := d.busyBases[outputPathState.outputBaseID]; ok { | ||
| return nil, nil, status.Error(codes.FailedPrecondition, "Output base is busy") |
There was a problem hiding this comment.
"Output base is currently finalizing a build or getting cleaned"
| delete(d.buildIDs, buildState.id) | ||
| outputPathState.buildState = nil | ||
| d.lock.Unlock() | ||
| } |
There was a problem hiding this comment.
What are your thoughts on writing this as follows instead?
// Look up the output base ID associated with the build ID,
// so that we may mark that output base ID busy.
d.lock.Lock()
outputPathState, ok := d.buildIDs[request.BuildId]
d.lock.Unlock()
if !ok {
return &bazeloutputservice.FinalizeBuildResponse{}, nil
}
release, err := d.acquireOutputBase(ctx, outputPathState.outputBaseID)
if err != nil {
return nil, err
}
defer release()
// Silently ignore requests for unknown build IDs. This ensures
// that FinalizeBuild() remains idempotent.
d.lock.Lock()
if outputPathState, ok := d.buildIDs[request.BuildId]; ok {
buildState := outputPathState.buildState
delete(d.buildIDs, buildState.id)
outputPathState.buildState = nil
d.lock.Unlock()
outputPathState.rootDirectory.FinalizeBuild(ctx, buildState.digestFunction)
} else {
d.lock.Unlock()
}That does add a second map lookup, but it does keep the code more readable.
There was a problem hiding this comment.
Agreed, the tradeoff seems worth it. thanks! Updated.
Give busy output base IDs their own mutex, return the context status directly when waiting is cancelled, and unregister a build before running its finalization hook.
EdSchouten
left a comment
EdSchouten
left a comment
There was a problem hiding this comment.
Thanks for your perseverance, Joel!
2 participants
Problem
The Bazel Output Service currently holds a daemon-wide mutex while restoring an output path and running
FinalizeBuild, which can include uploading local files and persisting output-tree state. Slow work for one output base therefore blocks unrelated output bases served by the same daemon.Simply releasing that mutex around expensive work is insufficient: lifecycle transitions must not race with staging/stat requests or allow queued requests to operate on a replacement build.
Changes
Introduce one context-cancelable weighted semaphore per output base:
StartBuild,FinalizeBuild, andClean.StageArtifactsandBatchStat, allowing these requests to overlap within a build.The patch also rejects build IDs associated with another output base and invokes the predecessor’s finalization hook when
StartBuildreplaces an unfinished build, as required by the protocol.Scope
This provides RPC lifecycle exclusion, not a filesystem snapshot. Existing directory/file locks continue to protect filesystem operations; NFS/FUSE requests do not acquire the lifecycle semaphore.
No protocol or public interface changes.
FinalizeArtifactsremains unchanged. Root-directory enumeration still holds the global mutex while retrieving/reporting child attributes; addressing that remaining contention is outside this change.