TrueCloudLab/frostfs-api-go
21
0
Fork 16
Code Issues 10 Pull requests 1 Projects Releases Activity Actions

Refactor sign/verify message #8

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-api-go:feat/refactor-signservicemessage into master 2023-07-26 21:08:03 +00:00
Conversation 4 Commits 1 Files changed 5 +67 -58
dstepanov-yadro commented 2023-03-07 07:35:07 +00:00
Member
Copy link
  1. Code refactor: drop some wrappers, split methods to different files, extract methods
  2. Do signing/verification parallel
  3. Use buffers for slices
benchstat master.bench pool.bench
goos: linux
goarch: amd64
pkg: github.com/TrueCloudLab/frostfs-api-go/v2/signature
cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz
                │ master.bench │              pool.bench               │
                │    sec/op    │    sec/op     vs base                 │
SignRequest-8     98.15µ ± ∞ ¹   54.78µ ± ∞ ¹        ~ (p=1.000 n=1) ²
VerifyRequest-8   300.3µ ± ∞ ¹   148.7µ ± ∞ ¹        ~ (p=1.000 n=1) ²
geomean           171.7µ         90.25µ        -47.43%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                │ master.bench  │              pool.bench               │
                │     B/op      │     B/op       vs base                │
SignRequest-8     15.03Ki ± ∞ ¹   15.20Ki ± ∞ ¹       ~ (p=1.000 n=1) ²
VerifyRequest-8   11.02Ki ± ∞ ¹   10.87Ki ± ∞ ¹       ~ (p=1.000 n=1) ²
geomean           12.87Ki         12.85Ki        -0.12%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05

                │ master.bench │             pool.bench              │
                │  allocs/op   │  allocs/op   vs base                │
SignRequest-8      158.0 ± ∞ ¹   159.0 ± ∞ ¹       ~ (p=1.000 n=1) ²
VerifyRequest-8    129.0 ± ∞ ¹   130.0 ± ∞ ¹       ~ (p=1.000 n=1) ²
geomean            142.8         143.8        +0.70%
¹ need >= 6 samples for confidence interval at level 0.95
² need >= 4 samples to detect a difference at alpha level 0.05
1. Code refactor: drop some wrappers, split methods to different files, extract methods 2. Do signing/verification parallel 3. Use buffers for slices ``` benchstat master.bench pool.bench goos: linux goarch: amd64 pkg: github.com/TrueCloudLab/frostfs-api-go/v2/signature cpu: 11th Gen Intel(R) Core(TM) i5-1135G7 @ 2.40GHz │ master.bench │ pool.bench │ │ sec/op │ sec/op vs base │ SignRequest-8 98.15µ ± ∞ ¹ 54.78µ ± ∞ ¹ ~ (p=1.000 n=1) ² VerifyRequest-8 300.3µ ± ∞ ¹ 148.7µ ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 171.7µ 90.25µ -47.43% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 │ master.bench │ pool.bench │ │ B/op │ B/op vs base │ SignRequest-8 15.03Ki ± ∞ ¹ 15.20Ki ± ∞ ¹ ~ (p=1.000 n=1) ² VerifyRequest-8 11.02Ki ± ∞ ¹ 10.87Ki ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 12.87Ki 12.85Ki -0.12% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 │ master.bench │ pool.bench │ │ allocs/op │ allocs/op vs base │ SignRequest-8 158.0 ± ∞ ¹ 159.0 ± ∞ ¹ ~ (p=1.000 n=1) ² VerifyRequest-8 129.0 ± ∞ ¹ 130.0 ± ∞ ¹ ~ (p=1.000 n=1) ² geomean 142.8 143.8 +0.70% ¹ need >= 6 samples for confidence interval at level 0.95 ² need >= 4 samples to detect a difference at alpha level 0.05 ```
dstepanov-yadro added 6 commits 2023-03-07 07:35:08 +00:00 
Update go version 1.17 -> 1.18

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Split methods to separate files, drop redundant intefaces

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Add benchmarks for sign and verify methods

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Sign request/response parts in parallel

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Verify request/response parts in parallel

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
Fix test run version

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
dstepanov-yadro requested review from fyrchik 2023-03-07 07:35:34 +00:00
dstepanov-yadro requested review from carpawell 2023-03-07 07:35:34 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-03-07 07:35:45 +00:00
fyrchik requested review from storage-core-committers 2023-03-07 07:47:14 +00:00
fyrchik reviewed 2023-03-07 07:55:13 +00:00
fyrchik left a comment
Owner
Copy link

Speaking of buffers pool, do we need a local benchmark for this?

Speaking of buffers pool, do we need a local benchmark for this?
signature/sign.go Outdated
@ -17,1 +13,4 @@
var wrapperPool = sync.Pool{
New: func() any {
return &StableMarshalerWrapper{}
fyrchik commented 2023-03-07 07:52:18 +00:00
Owner
Copy link

Can we instead just use StableMarshalerWrapper directly and make method receiver pointerless (it seems they already are)?

Can we instead just use `StableMarshalerWrapper` directly and make method receiver pointerless (it seems they already are)?
dstepanov-yadro commented 2023-03-07 08:34:03 +00:00
Author
Member
Copy link

Don't understand you. StableMarshalerWrapper is used to convert stableMarshaller interface to DataSource integface.

Don't understand you. StableMarshalerWrapper is used to convert stableMarshaller interface to DataSource integface.
fyrchik commented 2023-03-07 09:12:37 +00:00
Owner
Copy link

I've meant &StableMarshalerWrapper{SM: msg} allocates but with StableMarshalerWrapper{SM: msg} we, probably, do not need pool at all.

I've meant `&StableMarshalerWrapper{SM: msg}` allocates but with `StableMarshalerWrapper{SM: msg}` we, probably, do not need pool at all.
dstepanov-yadro commented 2023-03-07 10:06:08 +00:00
Author
Member
Copy link

Fixed.

Fixed.
carpawell commented 2023-03-09 16:32:54 +00:00
Contributor
Copy link

mb a separate commit for that then? freezed me a little when saw that change in the "Add buffer pool"

mb a separate commit for that then? freezed me a little when saw that change in the "Add buffer pool"
carpawell commented 2023-03-09 16:35:02 +00:00
Contributor
Copy link

... or not according to @fyrchik's:

The last 4 commits are really a single commit -- "add buffer pool", can we squash them?

up to you then

... or not according to @fyrchik's: > The last 4 commits are really a single commit -- "add buffer pool", can we squash them? up to you then
fyrchik commented 2023-03-10 05:53:38 +00:00
Owner
Copy link

I missed that part -- in Drop marshaler wrapper pool we actually also removed a pointer wrom the wrapper.

I missed that part -- in `Drop marshaler wrapper pool` we actually also removed a pointer wrom the wrapper.
util/signature/buffer.go Outdated
@ -0,0 +10,4 @@
var buffersPool = sync.Pool{
New: func() any {
return &buffer{}
fyrchik commented 2023-03-07 07:50:13 +00:00
Owner
Copy link

Why do you return *buffer and not []byte?

Why do you return `*buffer` and not `[]byte`?
dstepanov-yadro commented 2023-03-07 08:23:55 +00:00
Author
Member
Copy link

This makes easier to create new buffer here:

	if cap(result.buf) < size {
		result.buf = make([]byte, size)

It just creates a new slice in case of current capacity does not meet the requested size.

This makes easier to create new buffer here: ``` if cap(result.buf) < size { result.buf = make([]byte, size) ``` It just creates a new slice in case of current capacity does not meet the requested size.
fyrchik commented 2023-03-07 09:13:50 +00:00
Owner
Copy link

But how is cap(buf) < size and buf = make... different? Besides being able to implement free method on them (could be function as well).

But how is `cap(buf) < size` and `buf = make...` different? Besides being able to implement `free` method on them (could be function as well).
dstepanov-yadro commented 2023-03-07 10:00:22 +00:00
Author
Member
Copy link

You are right. Fixed.

You are right. Fixed.
fyrchik marked this conversation as resolved
dstepanov-yadro commented 2023-03-07 08:36:02 +00:00
Author
Member
Copy link

Speaking of buffers pool, do we need a local benchmark for this?

There are two benchmarks already (sign and verify) that use pool.

> Speaking of buffers pool, do we need a local benchmark for this? There are two benchmarks already (sign and verify) that use pool.
fyrchik commented 2023-03-07 09:16:45 +00:00
Owner
Copy link

Speaking of buffers pool, do we need a local benchmark for this?

There are two benchmarks already (sign and verify) that use pool.

They are single-threaded now (and I think we need them written this way).

If using pool provides some improvements, could you attach benchmark diff to a commit message?

> > Speaking of buffers pool, do we need a local benchmark for this? > > There are two benchmarks already (sign and verify) that use pool. They are single-threaded now (and I think we need them written this way). If using pool provides some improvements, could you attach benchmark diff to a commit message?
dstepanov-yadro force-pushed feat/refactor-signservicemessage from 7f9809b7eb to 311ce95cb3 2023-03-07 15:08:14 +00:00 Compare
fyrchik commented 2023-03-09 07:59:58 +00:00
Owner
Copy link

The last 4 commits are really a single commit -- "add buffer pool", can we squash them?
Introducing and dropping marshaler pool is not something we would like to see in history. May be we could mention about trying it in the PR message, though.
Adding a buffer pool and fixing it could become more readable too as an atomic change.

The last 4 commits are really a single commit -- "add buffer pool", can we squash them? Introducing and dropping marshaler pool is not something we would like to see in history. May be we could mention about trying it in the PR message, though. Adding a buffer pool and fixing it could become more readable too as an atomic change.
dstepanov-yadro force-pushed feat/refactor-signservicemessage from 311ce95cb3 to 18631efdaf 2023-03-09 08:36:16 +00:00 Compare
dstepanov-yadro commented 2023-03-09 08:36:51 +00:00
Author
Member
Copy link

The last 4 commits are really a single commit -- "add buffer pool", can we squash them?
Introducing and dropping marshaler pool is not something we would like to see in history. May be we could mention about trying it in the PR message, though.
Adding a buffer pool and fixing it could become more readable too as an atomic change.

Done

> The last 4 commits are really a single commit -- "add buffer pool", can we squash them? > Introducing and dropping marshaler pool is not something we would like to see in history. May be we could mention about trying it in the PR message, though. > Adding a buffer pool and fixing it could become more readable too as an atomic change. Done
fyrchik approved these changes 2023-03-09 11:10:49 +00:00
carpawell reviewed 2023-03-09 16:31:04 +00:00
.github/workflows/go.yml Outdated
@ -18,3 +18,3 @@
strategy:
matrix:
go: [ '1.17.x', '1.18.x', '1.19.x' ]
go: [ '1.18.x', '1.19.x', '1.20.x' ]
carpawell commented 2023-03-09 16:23:32 +00:00
Contributor
Copy link

i think that should be either removed or changed in the same commit that updates go version

i think that should be either removed or changed in the same commit that updates `go` version
dstepanov-yadro commented 2023-03-10 07:39:51 +00:00
Author
Member
Copy link

fixed

fixed
carpawell commented 2023-03-10 10:22:51 +00:00
Contributor
Copy link

well, PR that removes that file has been merged first :)

well, PR that removes that file has been merged first :)
carpawell marked this conversation as resolved
signature/sign.go Outdated
@ -175,0 +47,4 @@
body := serviceMessageBody(v)
meta := v.GetMetaHeader()
header := v.GetVerificationHeader()
if err := signMessageParts(key, body, meta, header, header != nil, result); err != nil {
carpawell commented 2023-03-09 15:44:05 +00:00
Contributor
Copy link

why do we pass both header and header != nil?

why do we pass both `header` and `header != nil`?
dstepanov-yadro commented 2023-03-10 07:16:24 +00:00
Author
Member
Copy link

because of go:

package main

import (
	"fmt"
)

type typeI interface{
	Do() error
}

type typeT struct {
}

func (v *typeT) Do() error {
	return nil
}

func main() {
	var vI typeI
	var vT *typeT
	isNill(vI) //true
	isNill(nil) //true
	isNill(vT) //false
}


func isNill(v typeI) {
	fmt.Println(v == nil)
}
because of go: ```golang package main import ( "fmt" ) type typeI interface{ Do() error } type typeT struct { } func (v *typeT) Do() error { return nil } func main() { var vI typeI var vT *typeT isNill(vI) //true isNill(nil) //true isNill(vT) //false } func isNill(v typeI) { fmt.Println(v == nil) } ```
👍 1
carpawell commented 2023-03-10 10:15:19 +00:00
Contributor
Copy link

oh, it is an interface in the func's definition, missed that

oh, it is an interface in the func's definition, missed that
carpawell marked this conversation as resolved
signature/sign_test.go Outdated
@ -73,0 +79,4 @@
for i := 0; i < b.N; i++ {
b.StopTimer()
dec := new(accounting.Decimal)
carpawell commented 2023-03-09 15:56:33 +00:00
Contributor
Copy link

why do we create the same message inside every bench cycle?

why do we create the same message inside every bench cycle?
dstepanov-yadro commented 2023-03-10 07:31:28 +00:00
Author
Member
Copy link

because sign methods creates origin at every call, so it's like matryoshka. so every cycle step signs not one message.

because sign methods creates origin at every call, so it's like matryoshka. so every cycle step signs not one message.
carpawell marked this conversation as resolved
util/signature/buffer.go Outdated
@ -0,0 +22,4 @@
func returnBufferToPool(buf []byte) {
if cap(buf) > poolSliceMaxSize {
buf = nil
carpawell commented 2023-03-09 16:30:57 +00:00
Contributor
Copy link

is there any practical case when we put nil slice and it helps us?

is there any practical case when we put `nil` slice and it helps us?
dstepanov-yadro commented 2023-03-10 07:22:33 +00:00
Author
Member
Copy link

Fixed

Fixed
carpawell marked this conversation as resolved
dstepanov-yadro force-pushed feat/refactor-signservicemessage from 18631efdaf to fe59664de1 2023-03-10 07:21:54 +00:00 Compare
dstepanov-yadro force-pushed feat/refactor-signservicemessage from fe59664de1 to 7655227773 2023-03-10 07:39:34 +00:00 Compare
carpawell approved these changes 2023-03-10 10:23:06 +00:00
carpawell reviewed 2023-03-10 10:25:45 +00:00
dstepanov-yadro force-pushed feat/refactor-signservicemessage from 7655227773 to ec0d0274fa 2023-03-10 10:36:41 +00:00 Compare
fyrchik merged commit ec0d0274fa into master 2023-03-10 10:54:48 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
fyrchik
carpawell
TrueCloudLab/storage-core-developers
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
good first issue

Good for newcomers

  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

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
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 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#8
No description provided.
Delete branch "dstepanov-yadro/frostfs-api-go:feat/refactor-signservicemessage"

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?