search

cockroachdb / apd

Arbitrary-precision decimals for Go
https://pkg.go.dev/github.com/cockroachdb/apd/v2
Apache License 2.0
664 stars 36 forks source link

apd: add inline fast-path to BigInt.SetString #119

Closed nvanbenschoten closed 2 years ago

nvanbenschoten commented 2 years ago

Not important, but easy enough to do and makes a difference for the performance of Decimal.setString, which #116 was interested in.

josharian commented 2 years ago

Nice. Is the speed-up primarily from avoiding big.Int's SetString method? If so, would it be worth fixing upstream instead (or in addition)?

nvanbenschoten commented 2 years ago

Is the speed-up primarily from avoiding big.Int's SetString method? If so, would it be worth fixing upstream instead (or in addition)?

Right. Beyond the reduced complexity of strconv.ParseInt compared to (*BigInt).SetString thanks to the fixed bit size, the main win here is the avoidance of a heap-allocated strings.Reader. I don't see a good way to avoid this in the standard library, given the different callers to (*BigInt).setFromScanner. At least not until the standard library adopts generics, and even then I don't recall whether the new GC shape stenciling will be enough to avoid heap allocations in cases like these. Do you happen to know?

josharian commented 2 years ago

I don't see a good way to avoid this in the standard library, given the different callers to (*BigInt).setFromScanner.

I had in mind putting it at the beginning of (*Int).SetString, not (*Int).setFromScanner, here:

https://cs.opensource.google/go/go/+/refs/tags/go1.18.3:src/math/big/int.go;l=425

(Given where we are in the Go release cycle, it'd be worth doing here as well.)