btcsuite / btcutil

Provides bitcoin-specific convenience functions and types
477 stars 410 forks source link

use fast base58 encode implementation (20~60% performance gain) #93

Closed rubensayshi closed 4 years ago

rubensayshi commented 7 years ago
$ go test -bench=. base58/base58bench_test.go 
BenchmarkBase58FastEncode-7               30      45674986 ns/op       0.11 MB/s
BenchmarkBase58SlowEncode-7               20      63573231 ns/op       0.08 MB/s
BenchmarkBase58FastEncodeSmall-7     1000000          1712 ns/op      17.52 MB/s
BenchmarkBase58SlowEncodeSmall-7      300000          5336 ns/op       5.62 MB/s
BenchmarkBase58Decode-7                  300       5276054 ns/op       1.29 MB/s
dajohi commented 7 years ago

Hi @rubensayshi

Can you please rebase and fix the Travis CI errors?

jrick commented 7 years ago

And while it's useful for the benchmarks, now that we've seen them, there's no reason to keep the slow implementation around.

stevenroose commented 7 years ago

Indeed. A FastEncode method is really not necessary. Just replace the existing encode implementation with the fast one.

davecgh commented 7 years ago

Thanks for the PR. As others have mentioned, I'd really prefer just replacing the Encode with the new faster method. If you would please do that and rebase/squash everything down, I'll get this approved and merged.

rubensayshi commented 7 years ago

@davecgh okay, glad to see it's well received, I'll cleanup the PR ;)

rubensayshi commented 7 years ago

whoops, almost forgot the important stuff!
the implementation is from https://github.com/mr-tron/go-base58 MIT license

how to properly incorperate this?

that's also why I don't know there the magic 138 number comes from

jrick commented 7 years ago

For the license, you can simply copy it into the source file.

davecgh commented 7 years ago

Oh, I didn't realize it had another license. Everything in btcsuite must be licensed under the ISC license. Unfortunately, I'm going to have to decline it as is unless you can get permission to relicense it.

stevenroose commented 7 years ago

@davecgh Isn't the ISC license totally equivalent to the MIT license?

The ISC license is a permissive free software license written by the Internet Systems Consortium (ISC). It is meant to be functionally equivalent to the simplified BSD and the MIT licenses, differing in its removal of language deemed unnecessary following the global adoption of the Berne Convention.

mr-tron commented 7 years ago

I gave permission special for this project.

rubensayshi commented 7 years ago

@davecgh @mr-tron here is the original author, is that enough or you need some more explicit?

davecgh commented 7 years ago

Great. Thanks @mr-tron.

@rubensayshi Thanks for following up. The author has provided permission. That's all we needed.

ribasushi commented 4 years ago

For those following along: I opened a new PR based on a newer iteration of pretty much the same code. Enc/Dec of b58 addresses is 4+/7+ times faster: https://github.com/btcsuite/btcutil/pull/170#issue-426748886

rubensayshi commented 4 years ago

👍 @ribasushi , I'll close my PR and trust you'll pull yours to the finish line :)

ribasushi commented 4 years ago

@rubensayshi I am sorry your PR got stuck, not sure why it happened :( If time permits and you happen to have a more extensive test suite: it would be great if you test the proposed changes a bit more. You know... just to be sure ;)

rubensayshi commented 4 years ago

no worries, it happens and I didn't bother reviving it so it's on me ;)

I don't have any more tests and no longer work with the codebase that was using this either so can't run it through that testsuite sorry