TrueCloudLab
/
frostfs-api-go
21
0
Fork 13
Code Issues 9 Pull Requests Actions Projects Releases Activity

Refactor sign/verify message #8

Merged
fyrchik merged 1 commits 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
dstepanov-yadro commented 2023-03-07 07:35:07 +00:00
Collaborator
  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
8e357496d8 [#3] api-go: Go version up 
Update go version 1.17 -> 1.18

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
f10ecb7fb4 [#3] signature: Refactor sign and verify 
Split methods to separate files, drop redundant intefaces

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
57045b8df4 [#3] signature: Add benchmarks 
Add benchmarks for sign and verify methods

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
635fac42ed [#3] signature: Sign parts in parallel 
Sign request/response parts in parallel

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
d11531446b [#3] signature: Verify parts in parallel 
Verify request/response parts in parallel

Signed-off-by: Dmitrii Stepanov <d.stepanov@yadro.com>
1d97ed1b95 [#3] .github: Fix github action 
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

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

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
Poster
Collaborator

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

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
Poster
Collaborator

Fixed.

Fixed.
carpawell commented 2023-03-09 16:32:54 +00:00
Collaborator

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
Collaborator

... 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

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

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
Poster
Collaborator

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

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
Poster
Collaborator

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
Poster
Collaborator

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

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

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

fixed

fixed
carpawell commented 2023-03-10 10:22:51 +00:00
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Collaborator

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
Poster
Collaborator

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
Collaborator

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
Poster
Collaborator

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 Label
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 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
There is no content yet.
Delete Branch "dstepanov-yadro/frostfs-api-go:feat/refactor-signservicemessage"

Deleting a branch is permanent. Although the deleted branch may exist for a short time before cleaning up, in most cases it CANNOT be undone. Continue?