receive.handler pooling improvements#299
Open
gavins-db wants to merge 21 commits intodatabricks:db_mainfrom
Open
receive.handler pooling improvements#299gavins-db wants to merge 21 commits intodatabricks:db_mainfrom
gavins-db wants to merge 21 commits intodatabricks:db_mainfrom
Conversation
gavins-db
commented
Feb 19, 2026
pkg/receive/handler.go
Outdated
Comment on lines
97
to
99
| defaultCompressedBufCap = 32 * 1024 | ||
| maxPooledCompressedCap = 1 << 20 // 1MB | ||
| maxPooledDecompressedCap = 4 << 20 // 4MB |
Author
There was a problem hiding this comment.
I'm curious about these numbers- where did they come from originally? i.e. do we have empirical data / histograms on incoming request sizes? Do we know what contributes to such large request sizes?
gavins-db
commented
Feb 20, 2026
|
|
||
| // Default / max capacities for pooled buffers. These caps prevent "pool ballooning" | ||
| // where a single large request permanently inflates process RSS. | ||
| defaultCompressedBufCap = 32 * 1024 |
Author
There was a problem hiding this comment.
note: I removed this defaultCompressedBufCap variable for 3 reasons:
- We don't want to over allocate from the beginning. As buffers in the pool are reused, they will grow to reach the steady state of a usual request.
- I wasn't able to find any empirical evidence that this 32KB actually is an expected average size.
- When pooling is deactivated, I want it to operate as close to the previous implementation as possible (before there was any pooling). The previous implementation stared with a default size of a)
content-length(we still do this a labuffer.Grow(content-length)) b) 512b if no content-length header is set (which actually is what the buffer will do internally).
gavins-db
commented
Feb 23, 2026
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.
Changes
This PR has three primary changes related to existing pooling in the receive handler that should improve memory usage and performance. Those changes are detailed blow. In addition to the changes above, I added a
syncutil.Poolutility that is a generic wrapper for pooling any type. This is a pattern I've found extremely useful in past lives to reduce boilerplate, improve readability, and make sure we're actually using pools correctly. And I also added flags to configure the pooling behavior (enable, max capacity, etc.).The first change removes the copyBufPool pool which was used as an intermediate buffer for compressed request bodies. This was an unnecessary intermediate buffer; the destination buffer (a
bytes.Buffer) has aReadFrommethod that can more efficiently copy over the contents of the source reader using its internal byte slice.Note: This means that we no longer validate rate limiting at the intermediate buffer level when no content-length header was provided, but rather at the final buffer level. This is OKAY. We have reduced the overall heap size by removing this unnecessary intermediate buffer. And this pattern ensures we fully drain the request body before returning a response which can help avoid tcp connection reset that occur if there is > 256KB of data remaining in the request body (see net/http/server.go#1089). There actually may be other places we should be discarding remaining
r.Bodydata before returning a response as well, but that was not my focus in this PR.The second change fixes an issue that was likely rendering the uncompressed slice pool ineffective due to how the
s2.Decodefunction works internally. Specifically,s2.Decodewill allocate a new slice if the provideddstbyte slice does not have enough capacity to hold the decoded data. This means that any uncompressed body larger than 128KB (the initial capacity of the byte slice) would be allocated and then thrown to GC. Now, we ensure the buffer is at least the capacity of the decoded data before passing it to thes2.Decodefunction so that we guarantee the buffer is large enough to be used in all cases.The third change was to remove the writeRequestPool which was using the
.Reset()of a proto object. This is not a good idea as it will replace the reference with a new struct, so we would effectively be pooling a pointer. I hope we'll be able to add pooling back to this in a future pr.Testing
go test -race -v -count=10 -timeout 15m ./pkg/receive/.... I'm relying on existing tests for the receive handler. But I verified that the tests break if the pools were improperly configured by temporarily skipping reset steps.