Refactor BufferPool #62

Merged
fyrchik merged 1 commits from achuprov/frostfs-api-go:buffer into master 2024-01-12 15:40:41 +00:00
Collaborator

Moved BufferPool into a separate package to enable easier reuse.

Ref: TrueCloudLab/frostfs-sdk-go#197

Signed-off-by: Alexander Chuprov a.chuprov@yadro.com

Moved `BufferPool` into a separate package to enable easier reuse. Ref: https://git.frostfs.info/TrueCloudLab/frostfs-sdk-go/pulls/197 Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
dstepanov-yadro reviewed 2023-12-22 11:27:54 +00:00
@ -0,0 +12,4 @@
}
func NewBufferPool(poolSliseMaxSize int) BufferPool {
pool := sync.Pool{

Please add panic if poolSliceMaxSize is negative

Please add panic if poolSliceMaxSize is negative
Collaborator

A year ago, we had a decision to avoid panics in rpc,sdk. If it is necessary, we can return error and panic when uses this method. Or use some default value.

A year ago, we had a decision to avoid panics in rpc,sdk. If it is necessary, we can return error and `panic` when uses this method. Or use some default value.

Then we can change type to uint32:)

Then we can change type to uint32:)
Poster
Collaborator

make() takes and cap() returns an int

`make()` takes and `cap()` returns an `int`
Poster
Collaborator

fixed

fixed
dstepanov-yadro reviewed 2023-12-22 11:28:05 +00:00
@ -0,0 +11,4 @@
buffersPool *sync.Pool
}
func NewBufferPool(poolSliseMaxSize int) BufferPool {

Slise => Slice

Slise => Slice
Poster
Collaborator

fixed

fixed
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-12-22 11:30:40 +00:00
@ -0,0 +27,4 @@
return pool.NewBufferFromPool(size), true
}
func (pool BufferPool) NewBufferFromPool(size int, capacity ...int) *Buffer {

To tricky. I say that capacity is not required here. How often do you check slice capacity?

To tricky. I say that capacity is not required here. How often do you check slice capacity?
Poster
Collaborator

The BufferPool is used as a replacement for make, so in my view the interface of BufferPool should be made similar to the interface of make

The `BufferPool` is used as a replacement for `make`, so in my view the interface of `BufferPool` should be made similar to the interface of `make`

Clear, ok

Clear, ok
achuprov force-pushed buffer from a95d5c04dc to f56634e6d1 2023-12-22 12:08:19 +00:00 Compare
achuprov force-pushed buffer from f56634e6d1 to 64ba7473fb 2023-12-22 12:56:56 +00:00 Compare
achuprov requested review from storage-core-committers 2023-12-22 13:27:32 +00:00
achuprov requested review from storage-core-developers 2023-12-22 13:27:35 +00:00
fyrchik reviewed 2023-12-22 13:46:25 +00:00
@ -0,0 +17,4 @@
func NewBufferPool(poolSliceMaxSize uint32) BufferPool {
pool := sync.Pool{
New: func() any {
return new(Buffer)

Maybe it would be beneficial to create max-sized slice here immediately?

Maybe it would be beneficial to _create_ max-sized slice here immediately?
Poster
Collaborator

In my view, this approach doesn't enhance performance, it risks allocating resources that may remain unused.

In my view, this approach doesn't enhance performance, it risks allocating resources that may remain unused.

Yes, you are right, user requested slice can be less than max size.

Yes, you are right, user requested slice can be less than max size.
fyrchik marked this conversation as resolved
fyrchik reviewed 2023-12-22 13:49:03 +00:00
@ -0,0 +23,4 @@
return BufferPool{poolSliceMaxSize: poolSliceMaxSize, buffersPool: &pool}
}
func (pool BufferPool) TryGetNewBufferFromPool(size uint32) (*Buffer, bool) {

What about Get(size) and Put(*Buffer)?
If we need slice, then the only reasonable behaviour in caseTry fails is to create it on client, this complicates code for no reason, we could create it here instead. Or do you have some cases for this?

As for Put -- this is somewhat common ,ReturnBufferToPool is long.

What about `Get(size)` and `Put(*Buffer)`? If we need slice, then the only reasonable behaviour in case`Try` fails is to create it on client, this complicates code for no reason, we could create it here instead. Or do you have some cases for this? As for `Put` -- this is somewhat common ,`ReturnBufferToPool` is long.
@ -0,0 +30,4 @@
return pool.NewBufferFromPool(size), true
}
func (pool BufferPool) NewBufferFromPool(size uint32, capacity ...uint32) *Buffer {

Why do we have 2 parameters here? We really only need capacity -- the we can do a[:size].

Why do we have 2 parameters here? We really only need capacity -- the we can do `a[:size]`.
Poster
Collaborator

fixed

fixed
fyrchik marked this conversation as resolved
achuprov force-pushed buffer from 64ba7473fb to fa62f4b05e 2024-01-09 07:50:34 +00:00 Compare
achuprov force-pushed buffer from fa62f4b05e to 014e8024f0 2024-01-09 08:00:32 +00:00 Compare
fyrchik reviewed 2024-01-09 16:05:55 +00:00
@ -0,0 +4,4 @@
"sync"
)
type Buffer struct {

Could you add comments for public structs and functions?

Could you add comments for public structs and functions?
@ -0,0 +42,4 @@
pool.buffersPool.Put(buf)
}
func (pool BufferPool) GetPoolSliceMaxSize() uint32 {

Let's not use Get for getters, just PoolSliceMaxSize() -- it it similar to len, cap, like here https://github.com/panjf2000/ants/blob/dev/pool.go#L249

Let's not use `Get` for getters, just `PoolSliceMaxSize()` -- it it similar to len, cap, like here https://github.com/panjf2000/ants/blob/dev/pool.go#L249
Poster
Collaborator

fixed

fixed
@ -39,2 +43,2 @@
if ok {
defer returnBufferToPool(buffer)
buffer := buffersPool.Get(uint32(src.SignedDataSize()))
if src.SignedDataSize() < int(buffersPool.GetPoolSliceMaxSize()) {

We already perform the comparision in Put, do we need this if? (we do not do this e.g. in util/signature/options.go below)

We already perform the comparision in `Put`, do we need this `if`? (we do not do this e.g. in `util/signature/options.go` below)
Poster
Collaborator

fixed

fixed
achuprov force-pushed buffer from 014e8024f0 to a747c4f2a4 2024-01-11 10:08:46 +00:00 Compare
fyrchik approved these changes 2024-01-11 11:17:16 +00:00
aarifullin reviewed 2024-01-12 09:24:55 +00:00
@ -0,0 +38,4 @@
return result
}
// Put returns a Buffer to the pool if its capacity is greater than or equal to the poolSliceSize.
Collaborator

Could you clarify, please:

It says

if its capacity is greater than or equal to the poolSliceSize

but

if cap(buf.Data) > int(pool.poolSliceSize) {
   return
}

that means exceeding capacity buffer is not returned to the pool. How come?

Could you clarify, please: It says > if its capacity is greater than or equal to the poolSliceSize but ```go if cap(buf.Data) > int(pool.poolSliceSize) { return } ``` that means exceeding capacity buffer is not returned to the pool. How come?
Poster
Collaborator

Fixed

Fixed
achuprov force-pushed buffer from a747c4f2a4 to b46e8cfbda 2024-01-12 15:09:28 +00:00 Compare
achuprov added 1 commit 2024-01-12 15:09:40 +00:00
Tests and linters / Tests (1.19) (pull_request) Successful in 7m46s Details
DCO action / DCO (pull_request) Successful in 8m16s Details
Tests and linters / Tests (1.20) (pull_request) Successful in 9m24s Details
Tests and linters / Tests with -race (pull_request) Successful in 9m25s Details
Tests and linters / Lint (pull_request) Successful in 9m49s Details
72885aae83
[#62] signature: Refactor BufferPool
Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
fyrchik approved these changes 2024-01-12 15:40:29 +00:00
fyrchik merged commit 72885aae83 into master 2024-01-12 15:40:41 +00:00
Sign in to join this conversation.
There is no content yet.