Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

nvanbenschoten
Copy link
Member

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

Not important, but easy enough to do and makes a difference for the
performance of `Decimal.setString`, which cockroachdb#116 was interested in.
@josharian
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Contributor

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.)

@nvanbenschoten nvanbenschoten merged commit 06ef971 into cockroachdb:master Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants