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

perf: use faster and lightweight base58 conversion #101

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

detailyang
Copy link
Contributor

Recently I found there is faster and lightweight base58 conversion in https://github.com/trezor/trezor-crypto/blob/master/base58.c. And the original base58 implement now is based on the bigint which is more expensive especially in memory allocated.

Below is the benchmark by go test -v -bench=. -benchmem -count=3 ./util/... :

goos: darwin
goarch: amd64
pkg: github.com/detailyang/go-bcrypto
BenchmarkBigintBase58EncodeAndDecode-8   	     500	   2816419 ns/op	 1022868 B/op	    1466 allocs/op
BenchmarkBigintBase58EncodeAndDecode-8   	     500	   2925519 ns/op	 1022864 B/op	    1466 allocs/op
BenchmarkBigintBase58EncodeAndDecode-8   	     500	   2888019 ns/op	 1022864 B/op	    1466 allocs/op
BenchmarkTrezorBase58EncodeAndDecode-8   	    1000	   1452371 ns/op	    6400 B/op	       5 allocs/op
BenchmarkTrezorBase58EncodeAndDecode-8   	    1000	   1452100 ns/op	    6400 B/op	       5 allocs/op
BenchmarkTrezorBase58EncodeAndDecode-8   	    1000	   1440593 ns/op	    6400 B/op	       5 allocs/op
BenchmarkOriginBase58EncodeAndDecode-8   	     500	   2492761 ns/op	  118752 B/op	    1472 allocs/op
BenchmarkOriginBase58EncodeAndDecode-8   	     500	   2480872 ns/op	  118752 B/op	    1472 allocs/op
BenchmarkOriginBase58EncodeAndDecode-8   	     500	   2491194 ns/op	  118752 B/op	    1472 allocs/op

Now the test coverage is 100% in util/base58.go and it looks like can be updated on the fly:)

@detailyang detailyang changed the title perf: use more fast and lightweight base58 conversion perf: use faster and lightweight base58 conversion Sep 26, 2018
@jjz
Copy link
Member

jjz commented Sep 28, 2018

Why not use base58?

@detailyang
Copy link
Contributor Author

Hello @jjz

There is no difference in the bigint implement and fast implement. The PR is an attempt trying to do performance optimization.
I'm not sure it should be merged or not and It depends on how we care to base58 encode/decode performance and we can suspend this PR until we need it :)

@fffreedom
Copy link
Contributor

@detailyang.
I carefully compared the C code and your code, there are many difference between them and I am not sure it's right or not of your code in short time.

Now, we have many other more important things(chain, transaction pool and etc.)to do and don't have enough time to verify this pr. Of course, we will verify it after October.

Thanks a lot!

@detailyang
Copy link
Contributor Author

@fffreedom never mind. Let's suspend it and do something important :)

@jjz
Copy link
Member

jjz commented Oct 10, 2018

@fffreedom the PR is reviewed?

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.

3 participants