Refactor BufferPool #62

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

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
Member

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:)
Author
Member

make() takes and cap() returns an int

`make()` takes and `cap()` returns an `int`
Author
Member

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
Author
Member

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?
Author
Member

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)
Owner

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

Maybe it would be beneficial to _create_ max-sized slice here immediately?
Author
Member

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.
Owner

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) {
Owner

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 {
Owner

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]`.
Author
Member

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 {
Owner

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 {
Owner

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
Author
Member

fixed

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

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)
Author
Member

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.
Member

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?
Author
Member

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
[#62] signature: Refactor BufferPool
All checks were successful
Tests and linters / Tests (1.19) (pull_request) Successful in 7m46s
DCO action / DCO (pull_request) Successful in 8m16s
Tests and linters / Tests (1.20) (pull_request) Successful in 9m24s
Tests and linters / Tests with -race (pull_request) Successful in 9m25s
Tests and linters / Lint (pull_request) Successful in 9m49s
72885aae83
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
fyrchik referenced this pull request from a commit 2024-01-12 15:40:42 +00:00
fyrchik referenced this pull request from a commit 2024-07-17 11:19:14 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-core-developers
No milestone
No project
No assignees
5 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-api-go#62
No description provided.