-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add write buffer pooling #407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
conn.go
Outdated
return p.pool.Get().([]byte) | ||
} | ||
|
||
func (p *syncBufferPool) Put(buf []byte) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the buffers in this pool will grow over time: unless they are resized by the user, the pool will inevitably contain only the largest buffers used by your program, even if most of your buffers were small.
This can lead to growing memory usage between GC cycles, and increased GC pressure.
golang/go#23199 covers this well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The byte slices in this scenario all have the same capacity.
The byte slices in this scenario all have the same capacity. The pooled value should be opaque to the application. This opens the possibility of pooling other state with the byte slice and hides the fact that the connection adds the max header size to the application's requested buffer size. |
I updated the PR with new proposed API and much of the implementation. The API adds a single bool to Upgrader and Dialer:
I got a little carried away when modifying newConnBRW for the feature. Let me know if you want me to dial (no pun intended!) some of that back to reduce the number of lines changed. I obviously think the code is better with the rework that I did.
|
The new API is easy to use, but it's not flexible as the BufferPool interface proposed earlier. The current PR does not allow the application to instrument buffer reuse or use something other than sync.Pool. Use this API:
|
Updated per request. A []byte is stored in the pool. You want the value to be opaque. Does it make sense to wrap the slice to keep applications from peeking at the value?
|
The current test is good. Add another test that uses sync.Pool. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove WIP from commit title
Add WriteBufferPool to Dialer and Upgrader. This field specifies a pool to use for write operations on a connection. Use of the pool can reduce memory use when there is a modest write volume over a large number of connections. Use larger of hijacked buffer and buffer allocated for connection (if any) as buffer for building handshake response. This decreases possible allocations when building the handshake response. Modify bufio reuse test to call Upgrade instead of the internal newConnBRW. Move the test from conn_test.go to server_test.go because it's a serer test. Update newConn and newConnBRW: - Move the bufio "hacks" from newConnBRW to separate functions and call these functions directly from Upgrade. - Rename newConn to newTestConn and move to conn_test.go. Shorten argument list to common use case. - Rename newConnBRW to newConn. - Add pool code to newConn.
This LGTM. I am curious if others have any feedback on the API. |
This PR proposes an API for write buffer pooling #9. If the API is
approved, I'll complete the implementation.
This proposed API has a typesafe API for the buffer pool and adds a
helper function to make it easy to use sync.Pool.