Refactor BufferPool #62
54
util/pool/buffer.go
Normal file
|
@ -0,0 +1,54 @@
|
|||
package pool
|
||||
|
||||
import (
|
||||
"sync"
|
||||
)
|
||||
|
||||
// Buffer contains a byte slice.
|
||||
|
||||
type Buffer struct {
|
||||
Data []byte
|
||||
}
|
||||
|
||||
// BufferPool manages a pool of Buffers.
|
||||
type BufferPool struct {
|
||||
poolSliceSize uint32 // Size for the buffer slices in the pool.
|
||||
dstepanov-yadro marked this conversation as resolved
Outdated
dstepanov-yadro
commented
Slise => Slice Slise => Slice
achuprov
commented
fixed fixed
|
||||
buffersPool *sync.Pool
|
||||
dstepanov-yadro
commented
Please add panic if poolSliceMaxSize is negative Please add panic if poolSliceMaxSize is negative
acid-ant
commented
A year ago, we had a decision to avoid panics in rpc,sdk. If it is necessary, we can return error and 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.
dstepanov-yadro
commented
Then we can change type to uint32:) Then we can change type to uint32:)
achuprov
commented
`make()` takes and `cap()` returns an `int`
achuprov
commented
fixed fixed
|
||||
}
|
||||
|
||||
// NewBufferPool creates a BufferPool with a specified size.
|
||||
func NewBufferPool(poolSliceSize uint32) BufferPool {
|
||||
pool := sync.Pool{
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
Maybe it would be beneficial to create max-sized slice here immediately? Maybe it would be beneficial to _create_ max-sized slice here immediately?
achuprov
commented
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.
fyrchik
commented
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.
|
||||
New: func() any {
|
||||
return new(Buffer)
|
||||
},
|
||||
}
|
||||
return BufferPool{poolSliceSize: poolSliceSize, buffersPool: &pool}
|
||||
}
|
||||
fyrchik
commented
What about As for 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.
|
||||
|
||||
// Get retrieves a Buffer from the pool or creates a new one if necessary.
|
||||
// It ensures the buffer's capacity is at least the specified size.
|
||||
func (pool BufferPool) Get(size uint32) *Buffer {
|
||||
dstepanov-yadro
commented
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?
achuprov
commented
The 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`
dstepanov-yadro
commented
Clear, ok Clear, ok
|
||||
result := pool.buffersPool.Get().(*Buffer)
|
||||
|
||||
if cap(result.Data) < int(size) {
|
||||
fyrchik marked this conversation as resolved
Outdated
fyrchik
commented
Why do we have 2 parameters here? We really only need capacity -- the we can do Why do we have 2 parameters here? We really only need capacity -- the we can do `a[:size]`.
achuprov
commented
fixed fixed
|
||||
result.Data = make([]byte, size)
|
||||
} else {
|
||||
result.Data = result.Data[:size]
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
// Put returns a Buffer to the pool if its capacity does not exceed poolSliceSize.
|
||||
aarifullin
commented
Could you clarify, please: It says
but
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?
achuprov
commented
Fixed Fixed
|
||||
func (pool BufferPool) Put(buf *Buffer) {
|
||||
if cap(buf.Data) > int(pool.poolSliceSize) {
|
||||
return
|
||||
}
|
||||
fyrchik
commented
Let's not use 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
achuprov
commented
fixed fixed
|
||||
|
||||
buf.Data = buf.Data[:0]
|
||||
pool.buffersPool.Put(buf)
|
||||
}
|
||||
|
||||
// PoolSliceSize returns the size for buffer slices in the pool.
|
||||
func (pool BufferPool) PoolSliceSize() uint32 {
|
||||
return uint32(pool.poolSliceSize)
|
||||
}
|
|
@ -1,40 +0,0 @@
|
|||
package signature
|
||||
|
||||
import "sync"
|
||||
|
||||
const poolSliceMaxSize = 128 * 1024
|
||||
|
||||
type buffer struct {
|
||||
data []byte
|
||||
}
|
||||
|
||||
var buffersPool = sync.Pool{
|
||||
New: func() any {
|
||||
return new(buffer)
|
||||
},
|
||||
}
|
||||
|
||||
func tryGetNewBufferFromPool(size int) (*buffer, bool) {
|
||||
if size > poolSliceMaxSize {
|
||||
return &buffer{}, false
|
||||
}
|
||||
return newBufferFromPool(size), true
|
||||
}
|
||||
|
||||
func newBufferFromPool(size int) *buffer {
|
||||
result := buffersPool.Get().(*buffer)
|
||||
if cap(result.data) < size {
|
||||
result.data = make([]byte, size)
|
||||
} else {
|
||||
result.data = result.data[:size]
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
func returnBufferToPool(buf *buffer) {
|
||||
if cap(buf.data) > poolSliceMaxSize {
|
||||
return
|
||||
}
|
||||
buf.data = buf.data[:0]
|
||||
buffersPool.Put(buf)
|
||||
}
|
|
@ -4,9 +4,14 @@ import (
|
|||
"crypto/ecdsa"
|
||||
|
||||
"git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/refs"
|
||||
"git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/util/pool"
|
||||
crypto "git.frostfs.info/TrueCloudLab/frostfs-crypto"
|
||||
)
|
||||
|
||||
const poolSliceMaxSize = 128 * 1024
|
||||
|
||||
var buffersPool = pool.NewBufferPool(poolSliceMaxSize)
|
||||
|
||||
type DataSource interface {
|
||||
ReadSignedData([]byte) ([]byte, error)
|
||||
SignedDataSize() int
|
||||
|
@ -35,12 +40,10 @@ func SignDataWithHandler(key *ecdsa.PrivateKey, src DataSource, handler KeySigna
|
|||
opts[i](cfg)
|
||||
}
|
||||
|
||||
buffer, ok := tryGetNewBufferFromPool(src.SignedDataSize())
|
||||
if ok {
|
||||
defer returnBufferToPool(buffer)
|
||||
}
|
||||
buffer := buffersPool.Get(uint32(src.SignedDataSize()))
|
||||
defer buffersPool.Put(buffer)
|
||||
fyrchik
commented
We already perform the comparision in 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)
achuprov
commented
fixed fixed
|
||||
|
||||
data, err := src.ReadSignedData(buffer.data)
|
||||
data, err := src.ReadSignedData(buffer.Data)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
@ -66,12 +69,10 @@ func VerifyDataWithSource(dataSrc DataSource, sigSrc KeySignatureSource, opts ..
|
|||
opts[i](cfg)
|
||||
}
|
||||
|
||||
buffer, ok := tryGetNewBufferFromPool(dataSrc.SignedDataSize())
|
||||
if ok {
|
||||
defer returnBufferToPool(buffer)
|
||||
}
|
||||
buffer := buffersPool.Get(uint32(dataSrc.SignedDataSize()))
|
||||
defer buffersPool.Put(buffer)
|
||||
|
||||
data, err := dataSrc.ReadSignedData(buffer.data)
|
||||
data, err := dataSrc.ReadSignedData(buffer.Data)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
|
|
|
@ -35,10 +35,10 @@ func verify(cfg *cfg, data []byte, sig *refs.Signature) error {
|
|||
case refs.ECDSA_RFC6979_SHA256:
|
||||
return crypto.VerifyRFC6979(pub, data, sig.GetSign())
|
||||
case refs.ECDSA_RFC6979_SHA256_WALLET_CONNECT:
|
||||
buffer := newBufferFromPool(base64.StdEncoding.EncodedLen(len(data)))
|
||||
defer returnBufferToPool(buffer)
|
||||
base64.StdEncoding.Encode(buffer.data, data)
|
||||
if !walletconnect.Verify(pub, buffer.data, sig.GetSign()) {
|
||||
buffer := buffersPool.Get(uint32(base64.StdEncoding.EncodedLen(len(data))))
|
||||
defer buffersPool.Put(buffer)
|
||||
base64.StdEncoding.Encode(buffer.Data, data)
|
||||
if !walletconnect.Verify(pub, buffer.Data, sig.GetSign()) {
|
||||
return crypto.ErrInvalidSignature
|
||||
}
|
||||
return nil
|
||||
|
@ -54,10 +54,10 @@ func sign(cfg *cfg, key *ecdsa.PrivateKey, data []byte) ([]byte, error) {
|
|||
case refs.ECDSA_RFC6979_SHA256:
|
||||
return crypto.SignRFC6979(key, data)
|
||||
case refs.ECDSA_RFC6979_SHA256_WALLET_CONNECT:
|
||||
buffer := newBufferFromPool(base64.StdEncoding.EncodedLen(len(data)))
|
||||
defer returnBufferToPool(buffer)
|
||||
base64.StdEncoding.Encode(buffer.data, data)
|
||||
return walletconnect.Sign(key, buffer.data)
|
||||
buffer := buffersPool.Get(uint32(base64.StdEncoding.EncodedLen(len(data))))
|
||||
defer buffersPool.Put(buffer)
|
||||
base64.StdEncoding.Encode(buffer.Data, data)
|
||||
return walletconnect.Sign(key, buffer.Data)
|
||||
default:
|
||||
panic(fmt.Sprintf("unsupported scheme %s", cfg.scheme))
|
||||
}
|
||||
|
|
Could you add comments for public structs and functions?