Add buffer support for payloadSizeLimiter #197

Merged
fyrchik merged 1 commit from achuprov/frostfs-sdk-go:buff_pool into master 2024-03-27 08:53:31 +00:00
Member

Close: #155

Example:
./frostfs-cli object put -r s01.frostfs.devenv:8080 --config conf.yml --file 1gb --cid cid --prepare-locally

Before:
image

Benchmark

goos: linux
goarch: amd64
pkg: git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/transformer
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz

Benchmark Iterations Time (ns/op) Memory (B/op) Allocations (allocs/op)
BenchmarkTransformer/small/no_size_hint-8 17,372 69,187 17,835 122
BenchmarkTransformer/small/no_size_hint, with buffer-8 17,535 69,673 17,834 122
BenchmarkTransformer/small/with size hint, with buffer-8 17,265 69,388 17,835 122
BenchmarkTransformer/big/no_size_hint-8 3 404,778,519 302,051,938 750
BenchmarkTransformer/big/no_size_hint, with buffer-8 2 726,653,062 1,492,467,568 810
BenchmarkTransformer/big/with size hint, with buffer-8 3 379,738,645 302,052,208 751

After:
image

Benchmark

goos: linux
goarch: amd64
pkg: git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/transformer
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz

Benchmark Iterations Time (ns/op) Memory (B/op) Allocations (allocs/op)
BenchmarkTransformer/small/no_size_hint-8 13,707 80,207 19,064 126
BenchmarkTransformer/small/no_size_hint, with buffer-8 13,214 75,819 19,063 126
BenchmarkTransformer/small/with size hint, with buffer-8 15,249 84,833 19,065 126
BenchmarkTransformer/big/no_size_hint-8 2 518,361,846 67,171,892 751
BenchmarkTransformer/big/no_size_hint, with buffer-8 3 506,265,786 67,171,546 748
BenchmarkTransformer/big/with size hint, with buffer-8 3 510,603,633 67,171,578 749

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

Close: #155 Example: ``` ./frostfs-cli object put -r s01.frostfs.devenv:8080 --config conf.yml --file 1gb --cid cid --prepare-locally ``` Before: ![image](/attachments/aa93dc9b-5798-49f6-ad6d-e92bf7c5eab9) <details><summary>Benchmark</summary> goos: linux goarch: amd64 pkg: git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/transformer cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz | Benchmark | Iterations | Time (ns/op) | Memory (B/op) | Allocations (allocs/op) | |----------------------------------------------------------|-----------:|-------------:|---------------:|------------------------:| | BenchmarkTransformer/small/no_size_hint-8 | 17,372 | 69,187 | 17,835 | 122 | | BenchmarkTransformer/small/no_size_hint, with buffer-8 | 17,535 | 69,673 | 17,834 | 122 | | BenchmarkTransformer/small/with size hint, with buffer-8 | 17,265 | 69,388 | 17,835 | 122 | | BenchmarkTransformer/big/no_size_hint-8 | 3 | 404,778,519 | 302,051,938 | 750 | | BenchmarkTransformer/big/no_size_hint, with buffer-8 | 2 | 726,653,062 | 1,492,467,568 | 810 | | BenchmarkTransformer/big/with size hint, with buffer-8 | 3 | 379,738,645 | 302,052,208 | 751 | </details> After: ![image](/attachments/321a8d0d-03f7-4fb7-87ae-9c889c1c2395) <details><summary>Benchmark</summary> goos: linux goarch: amd64 pkg: git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/transformer cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz | Benchmark | Iterations | Time (ns/op) | Memory (B/op) | Allocations (allocs/op) | |---------------------------------------------------------|-----------:|-------------:|--------------:|------------------------:| | BenchmarkTransformer/small/no_size_hint-8 | 13,707 | 80,207 | 19,064 | 126 | | BenchmarkTransformer/small/no_size_hint, with buffer-8 | 13,214 | 75,819 | 19,063 | 126 | | BenchmarkTransformer/small/with size hint, with buffer-8| 15,249 | 84,833 | 19,065 | 126 | | BenchmarkTransformer/big/no_size_hint-8 | 2 | 518,361,846 | 67,171,892 | 751 | | BenchmarkTransformer/big/no_size_hint, with buffer-8 | 3 | 506,265,786 | 67,171,546 | 748 | | BenchmarkTransformer/big/with size hint, with buffer-8 | 3 | 510,603,633 | 67,171,578 | 749 | </details> Signed-off-by: Alexander Chuprov <a.chuprov@yadro.com>
achuprov changed title from [#155] sdk-go: Add buffer support for payloadSizeLimiter to Add buffer support for payloadSizeLimiter 2023-12-20 15:26:36 +00:00
achuprov requested review from storage-core-developers 2023-12-20 15:35:30 +00:00
achuprov requested review from storage-core-committers 2023-12-20 15:35:30 +00:00
achuprov requested review from storage-services-developers 2023-12-20 15:35:52 +00:00
achuprov force-pushed buff_pool from efe8d55ce4 to d9d482c6b6 2023-12-21 06:39:34 +00:00 Compare
dstepanov-yadro reviewed 2023-12-21 07:13:54 +00:00
@ -12,9 +12,12 @@ import (
func (c *Client) objectPutInitTransformer(prm PrmObjectPutInit) (*objectWriterTransformer, error) {
var w objectWriterTransformer
pool := transformer.NewBytePool(prm.MaxSize)

I prefer to add pool optional. If it is possible, add so please.

I prefer to add pool optional. If it is possible, add so please.
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-12-21 07:15:35 +00:00
@ -0,0 +26,4 @@
}
func (bsp *BytePool) Put(s []byte) {
if cap(s) < int(bsp.size) {

I think It must be if cap(s) > int(bsp.size)

I think It must be ```if cap(s) > int(bsp.size)```
dstepanov-yadro marked this conversation as resolved
dstepanov-yadro reviewed 2023-12-21 07:18:01 +00:00
@ -0,0 +9,4 @@
size uint64
}
func NewByteSlicePool(initialCapacity uint64) *BytePool {
We have such implementation already: https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/util/signature/buffer.go
dstepanov-yadro marked this conversation as resolved
achuprov force-pushed buff_pool from d9d482c6b6 to 14a056fa02 2024-01-24 19:27:11 +00:00 Compare
achuprov reviewed 2024-01-24 19:33:21 +00:00
@ -232,4 +228,0 @@
},
key: v2acl.FilterObjectType,
value: "STORAGE_GROUP",
},
Author
Member

STORAGE_GROUP has been removed in TrueCloudLab/frostfs-api#32

STORAGE_GROUP has been removed in https://git.frostfs.info/TrueCloudLab/frostfs-api/pulls/32
dkirillov reviewed 2024-01-25 06:54:34 +00:00
@ -199,0 +212,4 @@
if s.Pool != nil {
var buffer buffPool.Buffer
buffer.Data = s.payload
defer s.Pool.Put(&buffer)
Member

Why do we use defer here?

Why do we use `defer` here?
dkirillov marked this conversation as resolved
achuprov force-pushed buff_pool from 14a056fa02 to 7606ce7f87 2024-01-25 11:14:51 +00:00 Compare
dkirillov approved these changes 2024-01-25 11:26:48 +00:00
dkirillov left a comment
Member

LGTM

LGTM
aarifullin approved these changes 2024-01-25 16:03:10 +00:00
aarifullin left a comment
Member

Very nice

Very nice
dstepanov-yadro approved these changes 2024-01-26 06:52:24 +00:00
fyrchik reviewed 2024-01-26 06:53:48 +00:00
@ -141,0 +144,4 @@
s.payload = make([]byte, 0, payloadSize)
} else {
if payloadSize == 0 {
payloadSize = s.MaxSize
Owner

So we will allocate 64 MiB even for 1KiB object?
There is a benchmark in the transformer package btw, could be useful.

So we will allocate 64 MiB even for 1KiB object? There is a benchmark in the transformer package btw, could be useful.
Owner

@achuprov the question is still valid

@achuprov the question is still valid
Author
Member

fixed

fixed
@ -199,0 +212,4 @@
if s.Pool != nil {
var buffer buffPool.Buffer
buffer.Data = s.payload
s.Pool.Put(&buffer)
Owner

If it is too big, it will be dropped, because each buffer pool has some max size for slices, right?

If it is too big, it will be dropped, because each buffer pool has some max size for slices, right?
Author
Member

Yes

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

Yes https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/commit/d60ce83e4271747cb4ee89e55cdaa698f17011e9/util/pool/buffer.go#L43
fyrchik marked this conversation as resolved
achuprov force-pushed buff_pool from 7606ce7f87 to fb97eb1106 2024-02-01 08:56:59 +00:00 Compare
Author
Member

Previously, the buffer would return the transformer to the pool after being used. However, this buffer could still be referenced by an object, potentially leading to data corruption. Now, the buffer is returned to the pool by the object itself.
Fixed handling of small objects
Example of usage: TrueCloudLab/frostfs-node#939

Previously, the buffer would return the `transformer` to the pool after being used. However, this buffer could still be referenced by an `object`, potentially leading to data corruption. Now, the buffer is returned to the pool by the `object `itself. Fixed handling of small objects Example of usage: https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/939
dkirillov reviewed 2024-02-02 06:12:56 +00:00
@ -37,2 +38,4 @@
Key *ecdsa.PrivateKey
//Optional. It makes sense to pass this if the file is large.
Member

Maybe it's better to change if the file is large -> if the object is large?

Maybe it's better to change `if the file is large` -> `if the object is large`?
Owner

We write that it is optional, but do not say what it is and why we may need this.
Can we extend the description and use appropriate godoc format? (// Pool is ...)
Also, other optional fields do not have comment, not sure we need it.

We write that it is optional, but do not say what it is and why we may need this. Can we extend the description and use appropriate godoc format? (`// Pool is ...`) Also, other optional fields do not have comment, not sure we need it.
Author
Member

Fixed

Fixed
dkirillov reviewed 2024-02-02 06:17:27 +00:00
@ -141,0 +149,4 @@
buffer := s.Pool.Get(uint32(payloadSize))
s.payload = buffer.Data
s.payload = s.payload[:0]
Member

Can we just write

s.payload = buffer.Data[:0]

?

Can we just write ```golang s.payload = buffer.Data[:0] ``` ?
Author
Member

Fixed

Fixed
dkirillov marked this conversation as resolved
fyrchik requested changes 2024-02-05 08:51:48 +00:00
@ -110,6 +112,8 @@ func (it *internalTarget) tryPutSingle(ctx context.Context, o *object.Object) (b
}
if err == nil {
it.returnBuffPool(o.Payload())
Owner

We must put to pool only something we have taken from it.
Also, can we panic if the pool is missing?

We must put to pool only something we have taken from it. Also, can we panic if the pool is missing?
Author
Member

Fixed

Fixed
@ -21,3 +22,3 @@
tt := new(testTarget)
target, pk := newPayloadSizeLimiter(maxSize, 0, func() ObjectWriter { return tt })
var contextTest benchContext
Owner

It is optional parameter, should not affect correctness, let's write a separate test if needed.

It is optional parameter, should not affect correctness, let's write a separate test if needed.
Owner

It is optional parameter, should not affect correctness, let's write a separate test if needed.

It is optional parameter, should not affect correctness, let's write a separate test if needed.
@ -154,3 +159,3 @@
b.ResetTimer()
for i := 0; i < b.N; i++ {
f, _ := newPayloadSizeLimiter(maxSize, uint64(sizeHint), func() ObjectWriter { return benchTarget{} })
var context benchContext
Owner

I would rather not use pool in benchmarks, or write a separate one.

I would rather not use pool in benchmarks, or write a separate one.
Author
Member

Removed the use of pool in benchmark.

Removed the use of pool in benchmark.
fyrchik marked this conversation as resolved
achuprov force-pushed buff_pool from fb97eb1106 to 2bcd4e9e52 2024-02-09 10:25:10 +00:00 Compare
achuprov force-pushed buff_pool from 2bcd4e9e52 to b7bf231269 2024-03-12 18:09:34 +00:00 Compare
achuprov requested review from fyrchik 2024-03-12 18:12:19 +00:00
achuprov force-pushed buff_pool from b7bf231269 to 74f09de408 2024-03-18 09:21:47 +00:00 Compare
fyrchik requested changes 2024-03-19 13:35:42 +00:00
@ -296,1 +305,4 @@
func (s *payloadSizeLimiter) writeHashes(chunk []byte) error {
// The buffer pool requires that the capacity must be <= MaxSize.
// Using append to increase size could lead to cap > MaxSize and len < MaxSize.
if cap(s.payload) < len(s.payload)+len(chunk) {
Owner

It is not a problem worth solving, it is rare and the code with calculations is quite complicated, while the benefits are not obvious. Consider a common case:

  1. Pool slices with 4 (or 128 kb capacity)
  2. MaxSize is 128 MiB
It is not a problem worth solving, it is rare and the code with calculations is quite complicated, while the benefits are not obvious. Consider a common case: 1. Pool slices with 4 (or 128 kb capacity) 2. `MaxSize` is 128 MiB
Author
Member

Agreed, I have removed this optimization.

Agreed, I have removed this optimization.
achuprov force-pushed buff_pool from 74f09de408 to 4695c32c6f 2024-03-25 07:40:40 +00:00 Compare
achuprov force-pushed buff_pool from 4695c32c6f to 1af9b6d18b 2024-03-25 07:47:14 +00:00 Compare
acid-ant approved these changes 2024-03-25 07:57:43 +00:00
fyrchik approved these changes 2024-03-27 08:53:21 +00:00
fyrchik merged commit 1af9b6d18b into master 2024-03-27 08:53:31 +00:00
Sign in to join this conversation.
No reviewers
TrueCloudLab/storage-services-developers
No milestone
No project
No assignees
6 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-sdk-go#197
No description provided.