Optimize NumDigits

[serprex]

Dividing BitLen by math.Log2(10) is what math/big does underneath

Not including the Int64/Uint64 check makes this slightly slower than old method for small integers

Included 2 benchmarks, for 10 digit numbers & 100 digit numbers:

-- before
> go test -bench=NumDigit -run=NumDigit
goos: linux
goarch: amd64
pkg: github.com/shopspring/decimal
cpu: AMD Ryzen 7 7840U w/ Radeon  780M Graphics
BenchmarkDecimal_NumDigits10-16     	18317293	        63.87 ns/op
BenchmarkDecimal_NumDigits100-16    	 3645015	       329.6 ns/op

-- after
...
BenchmarkDecimal_NumDigits10-16     	143781325	         8.488 ns/op
BenchmarkDecimal_NumDigits100-16    	 5931247	       207.4 ns/op

[mwoss]

Thanks again @serprex for your contribution! :3 Optimizing NumDigits was the next task on the roadmap, so I'm really happy that you worked on that :D

Unfortunately, I've spotted a small bug. The new implementation doesn't work for all potential decimals. For example decimal with 500 digits in base and 500 digits in fractional part, instead of returning 1000 it returns 999. By looking at your implementation I think an error correction check should be only applied when check >= 0, thus I think we should do something like this:

abs := new(big.Int).Abs(d.value)

estimatedNumDigits := int(float64(abs.BitLen()) / math.Log2(10))

// estimatedNumDigits (lg10) may be off by 1, need to verify
digitsBigInt := big.NewInt(int64(estimatedNumDigits))
errorCorrectionUnit := digitsBigInt.Exp(tenInt, digitsBigInt, nil)

if abs.Cmp(errorCorrectionUnit) >= 0 {
  return estimatedNumDigits + 1
}

return estimatedNumDigits

What do you think?

[serprex]

Updated with your suggestion. Some fuzzing against previous algo would be ideal, but seems good. Adding Cmp result was a bit too cute it seems

[mwoss]

Updated with your suggestion. Some fuzzing against previous algo would be ideal, but seems good. Adding Cmp result was a bit too cute it seems

Locally, I've run this function against 100k random decimals and compared it with the old implementation. It seems working fine after adjustments :3 But yea, I agree that we need to introduce a more sophisticated test suite one day. Simple unit testing is not enough if we operate on decimals with high precision.

[mwoss]

After the CI fail, I realized that CI only runs against the main branch and is not executed in PRs. I will need to take a look at that.

[mwoss]

Oh :< We overlooked one aspect @serprex. big.Int.IsInt64 and big.Int.CmpAbs are not available in Go 1.7 that we are aiming to support. They were introduced in 1.10.

[serprex]

@mwoss not sure what constraints determine your minversion, but 1.10 was released 6 years ago & the go compiler works to have backwards compatibility based on go.mod value that I'd be surprised if it's a problem raising minversion

[mwoss]

I just wanted to support the oldest Go version possible, this was the main reason. But maybe you're right and I'm exaggerating a bit here. When I started working on this library Go1.7 was only 3 years old, now it's already 8. Rather few people use such an old version of Go, plus what you said about go compiler backwards compatibility.