Optimize DecodeString and Decode methods.

[kevina]

In the DecodeString case it avoids unnecessary intermediate strings.
In the Decode case it just avoids bytes.Map, which is likely slow due
to having to make a function call for each character.

In the benchmark in ipfs/kubo#3376 is was able to save about 40 ms of cpu time.

[kevina]

Note: I manually ran go test ./... and everything passed.

[whyrusleeping]

use for _, c := range s {. Its faster because it optimizes out bounds checks.

[kevina]

I can fix this one. But see below.

[whyrusleeping]

same comment as above

[kevina]

The problem is it returns a rune and not a byte when using range for a string. Is there a way to do use range on the raw bytes or a string.

[whyrusleeping]

You would have to basically cast it to a []byte first. At which point you might as well just call Decode([]byte(s)) in the first place.

[kevina]

Right. So I was trying to avoid any necessary allocation so is this one okay as is?

[whyrusleeping]

heh, one thing you could do is convert the string to bytes, then overwrite that same array.

[kevina]

I could do that. It will require an extra pass and seams like an overkill. Eventually the go compiler should be smart enough to eliminate the bounds checks. Most other optimizing compilers will in simple cases like this.

Do you really think it could make that much of a difference? If so I will benchmark it and let you know.

[whyrusleeping]

It wont take an extra pass:

strb := []byte(s)
off := 0
for i, b := range strb {
  if b == '\n' || b == '\r' {
    continue
  }
  strb[off] = b
  off++
}

n, _, err := enc.decode(strb, strb[:off])
if err != nil {
  return nil, err
}
return strb[:n], nil

[kevina]

Yes it will. The extra pass is in the conversion from the string to a []byte.

I think this is just adding complexity but since you seam to really want it I will run some benchmarks and report back. It may be tomorrow through before I do it.

[whyrusleeping]

converting a string to a byte array is pretty well optimized. If youre going to count that as an extra pass then you have to count allocating a byte buffer as an extra pass as go zeros every buffer you allocate.

[kevina]

Like I said, I will run some benchmarks. In many cases you can allocate zeroed memory without require a pass to zero it.

[kevina]

@whyrusleeping Okay, I used "range" in Decode() but left DecodeString() alone.

If you are still not happy with this i can run some more tests to see if the index checking is really costing anything and if it would be better to convert to a []byte first.

Thanks

[kevina]

@whyrusleeping I ran some benchmarks. It turns out the index check is either optimized out or otherwise does not matter. Your version is faster because it only allocates once instead of twice (something that I missed yesterday, sorry), not because it avoids the index check. It also turns out that for i,c := range []byte(s) {} is optimized to avoid allocating an array (see https://github.com/golang/go/wiki/CompilerOptimizations#range-over-bytes ) Here are some numbers from the benchmark branch at https://github.com/kevina/base32 (commit 84ef217)

$ go test -bench 'BenchmarkKeyDecode.*'
BenchmarkKeyDecodeKevOrig-4      3000000               513 ns/op
BenchmarkKeyDecodeKevNew-4       3000000               449 ns/op
BenchmarkKeyDecodeOpt-4          3000000               452 ns/op
BenchmarkKeyDecodeDeOpt-4        3000000               553 ns/op
BenchmarkKeyDecodeWhy-4          3000000               470 ns/op
BenchmarkKeyDecodeFinal-4        3000000               482 ns/op

KevOrig is my orignal version, KevNew avoids an extra allocation, 'Opt' takes advantage of the for i,c := range []byte(s) {}optimization. DeOpt uses b := byte(s); for i,c := range b {} instead. Why is your original version. Final is a slightly refactored version of yours.

[kevina]

I just pushed a commit that uses the version of DecodeString uses by the DecodeFinal benchmark tests.

I'm fine if we just go with that (basically your version). The time between the different versions (that only use a single alloc) is very small and various by 20ns each time I run it.

[kevina]

For compassion I added the original implementation in the benchmarking code (a5bba06). Here are the numbers from another run

BenchmarkKeyDecodeOrig-4         2000000               892 ns/op
BenchmarkKeyDecodeKevOrig-4      3000000               498 ns/op
BenchmarkKeyDecodeKevNew-4       3000000               434 ns/op
BenchmarkKeyDecodeOpt-4          3000000               433 ns/op
BenchmarkKeyDecodeDeOpt-4        3000000               528 ns/op
BenchmarkKeyDecodeWhy-4          3000000               462 ns/op
BenchmarkKeyDecodeFinal-4        3000000               464 ns/op

Orig is the original implementation.

[whyrusleeping]

This LGTM, thanks @kevina!