Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Sum256 and Sum224 no-allocation functions #3

Merged
merged 4 commits into from
Aug 4, 2019

Conversation

teknico
Copy link

@teknico teknico commented Aug 4, 2019

The New and Sum methods, part of the Hash stdlib interface, allocate memory on the heap. The stdlib sha256 package uses a different method, Sum256, that does not allocate memory.

This change adds two functions, Sum256 and Sum224, and optimizes the code so that calling them does not allocate any memory on the heap. The standard New+Write+Sum API allocations are also reduced:

$ go test -benchmem -bench .
goos: linux
goarch: amd64
pkg: github.com/teknico/blake256
Benchmark1K-8          300000   3821 ns/op  267.99 MB/s  144 B/op  1 allocs/op
Benchmark8K-8           50000  28272 ns/op  289.75 MB/s  144 B/op  1 allocs/op
Benchmark64-8         3000000    550 ns/op  116.30 MB/s  144 B/op  1 allocs/op
Benchmark1KNoAlloc-8   300000   3737 ns/op  273.98 MB/s    0 B/op  0 allocs/op
Benchmark8KNoAlloc-8    50000  28334 ns/op  289.11 MB/s    0 B/op  0 allocs/op
Benchmark64NoAlloc-8  3000000    506 ns/op  126.40 MB/s    0 B/op  0 allocs/op

New tests and benchmarks are also added, and the tests package is made different from the code one. The tests do not need access to code internals anyway.

Copy link
Owner

@dchest dchest left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good apart from test (see comments)

blake256_test.go Outdated

import (
"bytes"
"fmt"
"hash"
"testing"

"github.com/teknico/blake256"
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm...

Copy link
Author

@teknico teknico Aug 4, 2019

Choose a reason for hiding this comment

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

Ouch, sorry, will fix it.

blake256_test.go Outdated
@@ -5,13 +5,15 @@
// worldwide. This software is distributed without any warranty.
// http://creativecommons.org/publicdomain/zero/1.0/

package blake256
package blake256_test
Copy link
Owner

Choose a reason for hiding this comment

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

Don't rename please.

Copy link
Author

@teknico teknico Aug 4, 2019

Choose a reason for hiding this comment

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

Will revert if you want me to. Care to mention why, though?

Copy link
Owner

Choose a reason for hiding this comment

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

I just like the simplicity of it, I guess 🤷‍♂

Copy link
Author

@teknico teknico Aug 4, 2019

Choose a reason for hiding this comment

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

Well, this way tests cannot change code internals, which makes them more effective (blackbox vs. whitebox testing, basically). But again, happy to revert if you don't like it.

Copy link
Owner

@dchest dchest Aug 4, 2019

Choose a reason for hiding this comment

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

Sure, but it's a few letters more to type for a theoretical benefit! Reading the code is the best guarantee that it can't change internals :) Whatever, let's do a separate package test if you want.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, then it would have hardcoded package path, so let's not do it.

Copy link
Owner

Choose a reason for hiding this comment

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

Interestingly, the mistake with import in PR was caused by a having a separate test package, so I guess my kind of testing wins :)

Copy link
Author

Choose a reason for hiding this comment

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

As you wish.

…nternals anyway"

This reverts commit c5d8354 as requested.
@dchest dchest merged commit 0af8a5e into dchest:master Aug 4, 2019
@dchest
Copy link
Owner

dchest commented Aug 4, 2019

Thanks again!

@teknico
Copy link
Author

teknico commented Aug 4, 2019

I just noticed I neglected to mention the two new functions in the README, sorry about that.

@teknico teknico deleted the add_sum256_and_sum224 branch August 4, 2019 20:01
@dchest
Copy link
Owner

dchest commented Aug 4, 2019

Ah, no worries. I should just like to godoc there instead of listing functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants