Skip to content

Conversation

@dstepanov-yadro
Copy link

  1. Code refactor: drop some wrappers, split methods to different files, extract methods
  2. Do signing/verification parallel
    benchstat compare with master:
    image

Copy link

@fyrchik fyrchik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think api-go: can be replaced with a signature: in the commit header, it should designate a component in this repo (or omitted).
Also I believe it should be Verify parts IN parallel.

Comment on lines 45 to 47
make([]byte, 0, size),
make([]byte, 0, size),
make([]byte, 0, size),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am wondering if it makes sense to allocate once and use sub-slices here. Probably not, can bite us in an unexpected way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 38 to 39
size := collection.Max(body.StableSize(), meta.StableSize(), verificationHeader.StableSize())
buf := make([]byte, 0, size)
return verifyServiceRequestRecursive(body, meta, verificationHeader, buf)
buffers := createBuffers(size)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Max was here when we had a single buffer, do we need a single size now?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

Comment on lines 6 to 17
func Max[T constraints.Ordered](items ...T) T {
if len(items) == 0 {
panic("failed to get max value: empty slice")
}
result := items[0]
for i := 1; i < len(items); i++ {
if items[i] > result {
result = items[i]
}
}
return result
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be used only from 1 package, why have you decided to exprort it?

Also, it isn't needed after parallelizations (see my later comment).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropped

@dstepanov-yadro
Copy link
Author

I think api-go: can be replaced with a signature: in the commit header, it should designate a component in this repo (or omitted). Also I believe it should be Verify parts IN parallel.

fixed

acid-ant
acid-ant previously approved these changes Feb 28, 2023
Update go version 1.17 -> 1.18

Signed-off-by: Dmitrii Stepanov <[email protected]>
Split methods to separate files, drop redundant intefaces

Signed-off-by: Dmitrii Stepanov <[email protected]>
Add benchmarks for sign and verify methods

Signed-off-by: Dmitrii Stepanov <[email protected]>
Sign request/response parts in parallel

Signed-off-by: Dmitrii Stepanov <[email protected]>
Verify request/response parts in parallel

Signed-off-by: Dmitrii Stepanov <[email protected]>
Fix test run version

Signed-off-by: Dmitrii Stepanov <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants