write buffer pooling#192
write buffer pooling#192y3llowcake wants to merge 1 commit intogorilla:masterfrom y3llowcake:gb_buffers
Conversation
ghost
left a comment
There was a problem hiding this comment.
Add a test. I suggest adding a loop with pools at
Line 80 in 0868951
- The nil pool.
- A pool that fails if call order is not Get / Put / Get / Put ... Get / Put. Calls must alternate between Get and Put. First call must be Get. Last call must be Put.
- sync.Pool
| Text string | ||
| } | ||
|
|
||
| // BufferPool represents a pool of buffers. |
There was a problem hiding this comment.
Mention that sync.Pool implements the interface. Also state that a shared pool must support concurrent Get() and Put().
|
Since this pool interface is specific to this package, shouldn't it work with |
|
Working with |
|
I am not enthusiastic about my suggestion to add the dialer and upgrader write pool fields in this PR. As I said in #193, I want to avoid adding new knobs to the API. On the other hand, I don't want an unexpected change in performance for existing users of the package. If anybody has thoughts on this issue, then please comment. |
|
How about we just to expose boolean fields in Upgrader and Dialer. e.g: This seems to expose fewer details and is arguably less brittle. It could also be reused as the signal for enabling pooling of compression and read buffers when we get around to it. The tricky bit is managing pools for different buffer sizes internally, but this seems like a tractable problem. |
|
EnableBufferPooling sounds like a good plan. I suggest calling it How about rounding the buffer size up to the next power of 2 and keeping one pool for each power of 2? |
|
Yah, this sounds reasonable. I'll try the new approach in this PR soon. In the mean time, I'd appreciate if you could take a look at #194 . I'm hoping to make progress fixing the existing flate performance situation, but hope it will converge with the work in this PR soon. |
|
Any news on this one? |
|
@y3llowcake any interest in continuing this PR? Is there anyone else who is interested in this that wants to pick up the branch and look at resolving the merge conflicts? |
|
Sorry, no. I'm no longer using this library. Good luck. |
No description provided.