[#81] node: Add basic read/write benchmarks for substorages #84

Merged
fyrchik merged 1 commit from ale64bit/frostfs-node:fix/81-substorage_benchmark into master 2023-03-15 16:37:05 +00:00
Member

Signed-off-by: Alejandro Lopez a.lopez@yadro.com

Close #81

Signed-off-by: Alejandro Lopez <a.lopez@yadro.com> Close #81
fyrchik requested review from storage-core-committers 2023-03-07 07:56:36 +00:00
fyrchik requested review from storage-core-developers 2023-03-07 07:56:36 +00:00
fyrchik requested changes 2023-03-07 10:17:11 +00:00
@ -0,0 +1,168 @@
// memstore implements a memory-backed common.Storage for testing purposes.
Owner

Package comment should start with a Package https://tip.golang.org/doc/comment

Package comment should start with a `Package ` https://tip.golang.org/doc/comment
Author
Member

done

done
fyrchik marked this conversation as resolved
@ -0,0 +190,4 @@
func addressFromObject(obj *objectSDK.Object) oid.Address {
var addr oid.Address
if id, isSet := obj.ID(); isSet {
Owner

Both object ID and container ID MUST always be set, we can ignore the second return value or panic here.

Both object ID and container ID MUST always be set, we can ignore the second return value or panic here.
Author
Member

done. Can we force it to be constructed properly then? (maybe hide it under an interface and have a New function that ensures those things are actually set or return an error).

done. Can we force it to be constructed properly then? (maybe hide it under an interface and have a `New` function that ensures those things are actually set or return an error).
Owner

The problem is that object starts being constructed before we set an actual ID, so not having ID is a possibility from the POV of code.

The problem is that object starts being constructed _before_ we set an actual ID, so not having ID is a possibility from the POV of code.
fyrchik marked this conversation as resolved
@ -0,0 +258,4 @@
}
// seqObjGenerator is an objectGenerator that generates entries with random payloads of size objSize and sequential IDs.
type seqObjGenerator struct {
Owner

I doubt this has any value: object ID is a hash, so sequential load is not something we could see in the real life.
However, we could write a benchmark (somewhat sequential) for putting objects in the same container vs putting to multiple containers.

I doubt this has any value: object ID is a hash, so `sequential` load is not something we could see in the real life. However, we could write a benchmark (somewhat sequential) for putting objects in the same container vs putting to multiple containers.
Author
Member

True, but having the sequential benchmark can provide a hint to whether the substorage should do something about the sequentiality of the keys in case sequential writes happen to be beneficial for a particular implementation. I think it's a good comparison point in either case.

True, but having the sequential benchmark can provide a hint to whether the substorage should do something about the sequentiality of the keys in case sequential writes happen to be beneficial for a particular implementation. I think it's a good comparison point in either case.
Owner

My point is that there is no sequential load in our system, so we do not need to optimize for this case. Because of this the value of the benchmark itself is limited: another storage can have it's different view on what is sequential, for example.

My suggestion is to use different OID but the same CID as an approximation of possible "sequential" load -- the load is real and storage component can choose to store objects by CID_OID key to make PUTs to the same container "more sequential".

My point is that there is no sequential load in our system, so we do not need to optimize for this case. Because of this the value of the benchmark itself is limited: another storage can have it's different view on what is sequential, for example. My suggestion is to use different OID but the same CID as an approximation of possible "sequential" load -- the load is real and storage component can choose to store objects by CID_OID key to make PUTs to the same container "more sequential".
fyrchik marked this conversation as resolved
@ -0,0 +331,4 @@
}
// Generates an object with random payload and the given address and size.
// TODO(ale64bit): there's some testing-related dupes in many places. Probably worth
Owner

Let's accompany all TODOs with a link to some issue?

Let's accompany all TODOs with a link to some issue?
Author
Member

done

done
fyrchik marked this conversation as resolved
ale64bit force-pushed fix/81-substorage_benchmark from 30ec5fedeb to 4aecd3f79a 2023-03-07 10:47:22 +00:00 Compare
ale64bit requested review from fyrchik 2023-03-07 10:51:58 +00:00
ale64bit force-pushed fix/81-substorage_benchmark from 4aecd3f79a to 1b58f60772 2023-03-13 10:00:04 +00:00 Compare
fyrchik approved these changes 2023-03-14 06:57:22 +00:00
go.mod Outdated
@ -39,6 +39,8 @@ require (
gopkg.in/yaml.v3 v3.0.1
)
require golang.org/x/exp v0.0.0-20230224173230-c95f2b4c22f2
Owner

Was it addded manually? I am wondering why it wasn't included in a previous require group.

Was it addded manually? I am wondering why it wasn't included in a previous `require` group.
Author
Member

nope. I guess from go mod tidy. Maybe it was only indirect before.

nope. I guess from `go mod tidy`. Maybe it was only indirect before.
fyrchik marked this conversation as resolved
fyrchik requested review from storage-core-committers 2023-03-14 06:57:30 +00:00
acid-ant approved these changes 2023-03-14 07:16:21 +00:00
carpawell reviewed 2023-03-14 08:22:32 +00:00
@ -0,0 +46,4 @@
// Decompress the data.
var err error
if data, err = s.compression.Decompress(data); err != nil {
return common.GetRes{}, logicerr.Wrap(fmt.Errorf("could not decompress object data: %w", err))
Contributor

is that a logic error?

is that a logic error?
Author
Member

I guess they aren't. Fixed.

I guess they aren't. Fixed.
@ -0,0 +76,4 @@
}
return common.GetRangeRes{
Data: payload[from:to],
Contributor

can we have some problems here because of mu not being held?

can we have some problems here because of `mu` not being held?
Author
Member

I'm not sure what kind of problem you have in mind. Can you elaborate? (mu is held during Get).

I'm not sure what kind of problem you have in mind. Can you elaborate? (`mu` is held during `Get`).
@ -0,0 +121,4 @@
if _, exists := s.objs[key]; exists {
delete(s.objs, key)
return common.DeleteRes{}, nil
} else {
Contributor

redundant else?

redundant `else`?
Author
Member

done

done
ale64bit force-pushed fix/81-substorage_benchmark from 1b58f60772 to c7d231f064 2023-03-14 09:37:13 +00:00 Compare
fyrchik approved these changes 2023-03-15 05:15:07 +00:00
@ -0,0 +274,4 @@
return addr
}
func TestSeqAddrGenerator(t *testing.T) {
Owner

Should we remove this now?

Should we remove this now?
Author
Member

I removed the sequential test for write benchmark, but the generator is still used (it's useful for example, when you want to fill an instance and execute a workload where you want to control the ratio of not_found to found, or know exactly which keys are present more generally, without storing them somewhere).

I removed the sequential test for write benchmark, but the generator is still used (it's useful for example, when you want to fill an instance and execute a workload where you want to control the ratio of not_found to found, or know exactly which keys are present more generally, without storing them somewhere).
dstepanov-yadro approved these changes 2023-03-15 14:12:56 +00:00
fyrchik merged commit 724debfdcd into master 2023-03-15 16:37:05 +00:00
ale64bit deleted branch fix/81-substorage_benchmark 2023-03-16 06:53:05 +00:00
Sign in to join this conversation.
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-node#84
No description provided.