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

Simplify decoding (no functional changes) #14

Merged
merged 4 commits into from
May 29, 2020
Merged

Simplify decoding (no functional changes) #14

merged 4 commits into from
May 29, 2020

Conversation

ribasushi
Copy link
Contributor

@ribasushi ribasushi commented May 22, 2020

Hi! It looks like the original implementation back in trezor/trezor-crypto@89a7d7797b806fac left a few too many sanity checks in the source. Adjusted and simplified to appease the go compiler.

Thank you for the excellent starting base, and the extensive test suite!

  • Before
base58$ go test -bench=. -count=1 -benchmem ./
goos: darwin
goarch: amd64
pkg: github.com/mr-tron/base58
BenchmarkTrivialBase58Encoding-8   	  182612	      7054 ns/op	    1911 B/op	      90 allocs/op
BenchmarkFastBase58Encoding-8      	  928771	      1245 ns/op	     144 B/op	       2 allocs/op
BenchmarkTrivialBase58Decoding-8   	  359553	      3370 ns/op	     513 B/op	      48 allocs/op
BenchmarkFastBase58Decoding-8      	 1558393	       838 ns/op	     368 B/op	       3 allocs/op
PASS
ok  	github.com/mr-tron/base58	58.666s
base58$ go test -bench=. -count=1 -benchmem ./
goos: darwin
goarch: amd64
pkg: github.com/mr-tron/base58
BenchmarkTrivialBase58Encoding-8   	  155588	      7466 ns/op	    1911 B/op	      90 allocs/op
BenchmarkFastBase58Encoding-8      	  966726	      1245 ns/op	      96 B/op	       2 allocs/op
BenchmarkTrivialBase58Decoding-8   	  350648	      3447 ns/op	     513 B/op	      48 allocs/op
BenchmarkFastBase58Decoding-8      	 2276010	       523 ns/op	     127 B/op	       2 allocs/op
PASS
ok  	github.com/mr-tron/base58	57.326s

/cc @prusnak @Stebalien

@mr-tron
Copy link
Owner

mr-tron commented May 23, 2020

Thank you. I don't have time today, but I will check and merge your pull requests tomorrow.

ribasushi referenced this pull request in njones/base58 May 24, 2020
The port is based off of the github.com/trezor/trezor-crypto/base58.c code. There can be some more optimizations around buffering but it works with the tests.
Updated the tests to test Fast and Trivial decode as well
Updated the benchmarks to include decoding (seems to be just over a 6x improvement)
@ribasushi
Copy link
Contributor Author

I don't have time today, but I will check and merge your pull requests tomorrow.

@mr-tron no worries, I clearly wasn't done anyway :) I think now I am done with tweaking all of this, the 3 PRs are good to look at, whenever time permits.

Thanks!

@ribasushi
Copy link
Contributor Author

Hi there @mr-tron! Do you think you will have a chance to look at these before next Monday? Thanks!

@mr-tron mr-tron merged commit e9607df into mr-tron:master May 29, 2020
@mr-tron
Copy link
Owner

mr-tron commented May 29, 2020

I merged. You can also add your name into LICENSE file if you want.

@ribasushi ribasushi deleted the faster_decode branch May 29, 2020 14:59
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