server: add --max_inflight_bytes to bound concurrent request memory#908
Open
mjonss wants to merge 2 commits into
Open
server: add --max_inflight_bytes to bound concurrent request memory#908mjonss wants to merge 2 commits into
mjonss wants to merge 2 commits into
Conversation
Adds BenchmarkDiskCacheGetDecompress(+Parallel) exercising the Get + decompress serving path. Unlike the write path, the read path reuses a pooled zstd decoder, so per-request churn is tiny (~45 KB/op serial vs ~685 KB/op parallel). This documents that the download-burst memory growth is driven by live per-stream state times concurrency, not allocation churn -- motivating a concurrency limit rather than buffer pooling on the read side.
Under a burst of concurrent uploads or downloads, bazel-remote could be OOM-killed by unbounded request concurrency: each in-flight gRPC bytestream request pins live memory (a per-stream read buffer, a zstd encoder/decoder window, and gRPC send/receive buffers), and there was no limit on how many ran at once, so peak memory scaled with client fan-out. Heap profiling of a download burst showed ~5.8 GB resident across ~1000 concurrent Read streams (48% gRPC send buffers, 29% zstd decoder windows, 20% read buffers), all under grpcServer.Read. Add a --max_inflight_bytes flag (env BAZEL_REMOTE_MAX_INFLIGHT_BYTES) that bounds the total logical size of blobs being served or received concurrently, via a weighted semaphore held for the whole request. Requests block (backpressure) instead of exhausting memory once the budget is reached. The weight is clamped to [1, budget] so a blob larger than the budget is still served, serialized against others. 0 (the default) disables the limit, preserving existing behavior. Client cancellation releases the wait. TestGrpcInflightLimitBoundsConcurrentReads verifies the cap: with a budget of K*blobSize, at most K reads reach the cache concurrently and the rest block until budget frees.
Author
|
This is to make it easier to run bazel-remote with gRPC in low memory setups, which was an issue for me. The argument and env variable name was chosen to be more related to memory usage, instead of directly to concurrency (which then needs to be calculated from how much memory would be used per stream). |
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.
Problem
Under a burst of concurrent uploads or downloads, bazel-remote's peak memory scales with client fan-out and can OOM (see #529): each in-flight gRPC bytestream request pins live memory — a per-stream read buffer, a zstd encoder/decoder window, and gRPC send/receive buffers — and there is no limit on how many run concurrently.
A heap profile captured at the peak of a real download burst showed ~5.8 GB resident across ~1000 concurrent
Readstreams — 48% gRPC send buffers, 29% zstd decoder windows, 20% read buffers — essentially all undergrpcServer.Read. The working set fully recovers afterward, so it is live memory proportional to concurrency, not a leak.Fix
Add a
--max_inflight_bytesflag (envBAZEL_REMOTE_MAX_INFLIGHT_BYTES) that bounds the total logical (uncompressed) size of blobs being served or received concurrently, via a weighted semaphore held for the whole request. Once the budget is reached, further requests block (backpressure) instead of exhausting memory. The weight is clamped to[1, budget]so a blob larger than the whole budget is still served (serialized against others) rather than deadlocking.0(the default) disables the limit, preserving existing behavior. Client cancellation releases the wait.This applies the same bounded-concurrency idea that #480 / #556 introduced for disk eviction to request memory, and makes the high memory usage reported in #529 configurable.
Tests
TestGrpcInflightLimitBoundsConcurrentReadsverifies the cap deterministically: with a budget of K×blobSize, at most K reads reach the cache concurrently and the rest block until budget frees (verified to fail when the limit is disabled).This PR also adds a read/decompress benchmark documenting that the read path barely churns (the zstd decoder is pooled) — i.e. the download OOM is driven by live per-stream state × concurrency, not allocation churn, which is why the fix is a concurrency bound rather than buffer pooling.