cryptocoinjs / bs58

Base58 encoding/decoding for Bitcoin
http://cryptocoinjs.com/modules/misc/bs58/
MIT License
216 stars 48 forks source link

Replace bigi with digit-array #4

Closed deckar01 closed 10 years ago

deckar01 commented 10 years ago

bigi has a lot of code that is not needed for base conversion. digit-array is a much lighter library that specializes in converting between arbitrary bases.

This change should also make encoding and decoding faster, because bigi uses a large intermediate base internally. digit-array performs direct base conversion, which should cut the conversion computation in half.

This PR replaces bigi with a small amount of pure JavaScript.

Fixes #2.

jprichardson commented 10 years ago

Hmm, I'm not sure how I feel about this. On one hand, it's definitely smaller code, which is awesome. On the other hand, I've decided that I'm going to maintain and release the cryptocoin module with a 1.0.0 (api stable); this will bring in another dependency into the ecosystem. That's not inherently bad, but it does add a degree of risk (albeit small).

This looks like it may be slower than bigi since it's doing division and multiplication on 8 bit values as opposed to 53 bit values... am I wrong on this? I don't suppose you have any benchmarks?

Thanks for the interest btw.

deckar01 commented 10 years ago

I will benchmark tonight. bytes -> bigi -> base58 should theoretically always have more divide operations than bytes -> base58.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling c1c0ab4ac759ec186f76bafbbf6e3104c4ec765d on deckar01:digit-array into 3bcbb506ebf497ac0ec71c2f2acf67f36bbec826 on cryptocoinjs:master.

deckar01 commented 10 years ago

Benchmarks (Intel core i7, OSX 10.9, Chrome 35)

Encode:

Decode: http://jsperf.com/bigi-vs-digit-array-decode

Combined:

I think the performance is strong enough for production, but I am willing to investigate decode()'s performance further. Strangely, on mobile Chrome digit-array was twice as fast as bigi for decode().

jprichardson commented 10 years ago

Hey @dcousens, do you have any thoughts on including digit-array in this over bigi? Anything I might be missing?

dcousens commented 10 years ago

I personally don't think the margins are worth it. We can do a lot better if we just push our efforts to testing/fixing already optimized number libraries like bn.js. The cost of a new dependency is pretty high in terms of risk/reward. However, if this test data was actually verified... I guess it'd hold a stronger case in the short term...

Turns out you do the require and tests at the bottom of the test file.

@deckar01 please don't add the minified to the repository, aside from bad security practice, it is absolutely impossible to review diffs (esp. since even GitHub spits the dummy on the commit page).

deckar01 commented 10 years ago

I closed this PR in favor of #5.

5 is a pure js implementation that leaves as much original code intact as possible and conforms to the existing code style.