Skip to content

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

Merged
nvb merged 1 commit into
cockroachdb:masterfrom
nvb:nvanbenschoten/inlineSetString
Jul 5, 2022
Merged

apd: add inline fast-path to BigInt.SetString#119
nvb merged 1 commit into
cockroachdb:masterfrom
nvb:nvanbenschoten/inlineSetString

Conversation

@nvb

@nvb nvb commented Jun 16, 2022

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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)?

@nvb

nvb commented Jun 17, 2022

Copy link
Copy Markdown
Contributor 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
Copy Markdown
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.)

@nvb nvb 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