copernicus
copernicus copied to clipboard
perf: use faster and lightweight base58 conversion
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:)
Why not use base58?
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 :)
@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!
@fffreedom never mind. Let's suspend it and do something important :)
@fffreedom the PR is reviewed?