[buffered encoder pool] Add maximum capacity check to buffers returned to pool #76
Conversation
…n a certain capacity to the pool
| @@ -23,12 +23,16 @@ package msgpack | |||
| import "github.com/m3db/m3x/pool" | |||
|
|
|||
| type bufferedEncoderPool struct { | |||
| pool pool.ObjectPool | |||
| maxBufferCapacity int | |||
There was a problem hiding this comment.
Perhaps just maxCapacity?
| @@ -42,5 +46,8 @@ func (p *bufferedEncoderPool) Get() BufferedEncoder { | |||
| } | |||
|
|
|||
| func (p *bufferedEncoderPool) Put(encoder BufferedEncoder) { | |||
| if p.maxBufferCapacity != 0 && cap(encoder.Buffer().Bytes()) > p.maxBufferCapacity { | |||
There was a problem hiding this comment.
Should probably do encoder.Buffer().Cap() here? .Bytes() returns the a slice whose length is the same as the number of unread bytes so that's probably not what we want.
Also nit: no need to special check p.maxBufferCapacity != 0.
There was a problem hiding this comment.
Ahh, ok, was not aware of that distinction, will update.
Initially, I had thought of using 0 to indicate no max capacity, will just change the default to be max int so we don't have to make this check.
| // Retrieve an encoder and assert it's a different encoder since | ||
| // the previous one exceeded the maximum capacity of the pool. | ||
| encoder = p.Get() | ||
| fmt.Println(encoder.Buffer().Len()) |
| // the previous one exceeded the maximum capacity of the pool. | ||
| encoder = p.Get() | ||
| fmt.Println(encoder.Buffer().Len()) | ||
| require.Equal(t, 0, encoder.Buffer().Len()) |
There was a problem hiding this comment.
Probably better to assert encoder.Buffer().Cap() here?
protocol/msgpack/options.go
Outdated
| @@ -42,6 +47,39 @@ const ( | |||
| defaultAggregatedReaderBufferSize = 1440 | |||
| ) | |||
|
|
|||
| type bufferedEncoderPoolOptions struct { | |||
| maxBufferCapacity int | |||
protocol/msgpack/types.go
Outdated
| // BufferedEncoderPoolOptions provides options for buffered encoder pools. | ||
| type BufferedEncoderPoolOptions interface { | ||
| // SetMaxBufferCapacity sets the maximum buffer capacity. | ||
| SetMaxBufferCapacity(value int) BufferedEncoderPoolOptions |
There was a problem hiding this comment.
nit: just SetMaxCapacity?
protocol/msgpack/types.go
Outdated
| SetMaxBufferCapacity(value int) BufferedEncoderPoolOptions | ||
|
|
||
| // MaxBufferCapacity returns the maximum buffer capacity. | ||
| MaxBufferCapacity() int |
| @@ -23,15 +23,15 @@ package msgpack | |||
| import "github.com/m3db/m3x/pool" | |||
|
|
|||
| type bufferedEncoderPool struct { | |||
| maxBufferCapacity int | |||
| pool pool.ObjectPool | |||
| maxCapacity int64 | |||
There was a problem hiding this comment.
nit: probably int is sufficient, given that that's the return type of cap.
protocol/msgpack/options.go
Outdated
| @@ -21,12 +21,13 @@ | |||
| package msgpack | |||
|
|
|||
| import xpool "github.com/m3db/m3x/pool" | |||
| import "math" | |||
There was a problem hiding this comment.
Group them into an import block?
There was a problem hiding this comment.
Not sure why VS Code did it like that lol
protocol/msgpack/options.go
Outdated
| @@ -48,26 +49,26 @@ const ( | |||
| ) | |||
|
|
|||
| type bufferedEncoderPoolOptions struct { | |||
| maxBufferCapacity int | |||
| poolOpts xpool.ObjectPoolOptions | |||
| maxCapacity int64 | |||
protocol/msgpack/options.go
Outdated
|
|
||
| const ( | ||
| // The maximum capacity of buffers that can be returned to the buffered | ||
| // encoder pool. A value of 0 indicates no maximum so all buffered |
There was a problem hiding this comment.
Remove the A value of 0... part?
jeromefroe
deleted the
jeromefroe/buffered-encoder/provide-option-to-not-return-large-buffers-to-pool
branch
This PR adds a maximum capacity check to the buffered encoder pool. When set, buffers with a capacity greater than the maximum capacity will not be returned to the pool. This PR is to address memory growth in the proxycollectors when they receive large timer batches, write them into a single buffer, and return that buffer to the pool.
cc @xichen2020