Refactor BufferPool #62
No reviewers
TrueCloudLab/storage-core-developers
Labels
No labels
P0
P1
P2
P3
good first issue
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
No milestone
No project
No assignees
5 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: TrueCloudLab/frostfs-api-go#62
Loading…
Reference in a new issue
No description provided.
Delete branch "achuprov/frostfs-api-go:buffer"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
@ -0,0 +12,4 @@
}
func NewBufferPool(poolSliseMaxSize int) BufferPool {
pool := sync.Pool{
Please add panic if poolSliceMaxSize is negative
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:)
make()
takes andcap()
returns anint
fixed
@ -0,0 +11,4 @@
buffersPool *sync.Pool
}
func NewBufferPool(poolSliseMaxSize int) BufferPool {
Slise => Slice
fixed
@ -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?
The
BufferPool
is used as a replacement formake
, so in my view the interface ofBufferPool
should be made similar to the interface ofmake
Clear, ok
a95d5c04dc
tof56634e6d1
f56634e6d1
to64ba7473fb
@ -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?
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.
@ -0,0 +23,4 @@
return BufferPool{poolSliceMaxSize: poolSliceMaxSize, buffersPool: &pool}
}
func (pool BufferPool) TryGetNewBufferFromPool(size uint32) (*Buffer, bool) {
What about
Get(size)
andPut(*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]
.fixed
64ba7473fb
tofa62f4b05e
fa62f4b05e
to014e8024f0
@ -0,0 +4,4 @@
"sync"
)
type Buffer struct {
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, justPoolSliceMaxSize()
-- it it similar to len, cap, like here https://github.com/panjf2000/ants/blob/dev/pool.go#L249fixed
@ -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 thisif
? (we do not do this e.g. inutil/signature/options.go
below)fixed
014e8024f0
toa747c4f2a4
@ -0,0 +38,4 @@
return result
}
// Put returns a Buffer to the pool if its capacity is greater than or equal to the poolSliceSize.
Could you clarify, please:
It says
but
that means exceeding capacity buffer is not returned to the pool. How come?
Fixed
a747c4f2a4
tob46e8cfbda