Allocate with headroom when BigDigits spills to the heap - #355
Conversation
`BigDigits::push` moved from `Inline` to `Heap` with `[*x, y].to_vec()`, which allocates capacity exactly 2, so growth then ran 2 -> 4 -> 8. `Vec::push` starts at 4 for `BigDigit` at either width, which is what the pre-rust-num#307 code got. Values growing past one digit did one more realloc than before, with each growth step one push earlier. Adds benchmarks for accumulating a decimal integer 18 digits at a time, bracketing the Inline -> Heap transition.
| // Capacity 2 here would make growth run 2 -> 4 -> 8, one realloc more than | ||
| // `Vec::push` does on its own: its minimum non-zero capacity is 4 for | ||
| // `BigDigit`, at either digit width. | ||
| let mut xs = Vec::with_capacity(4); |
There was a problem hiding this comment.
Let's put this 4 in a constant and cite the upstream min here:
https://github.com/rust-lang/rust/blob/e71c0f1e3395b10a8c331317be1a5c107bdf7b2e/library/alloc/src/raw_vec/mod.rs#L153-L166
Then we should also consider this in other methods that affect capacity, like shrink, normalize, extend, etc. ... even from_slice, but from_vec can leave its allocation alone.
There was a problem hiding this comment.
Done. (except shrink and normalize which look a bit more complicated)
Review feedback: `4` is `RawVec::MIN_NON_ZERO_CAP`, so give it that name and link to where upstream defines it rather than leaving the value bare. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KLR7FhfVNh2Qmd8WS7NwHD
24fcbb5 to
88ba75a
Compare
|
@cuviper could you at least allow CI to run on this. It would be great to get this merged and released - it would be great to reverse the ~55% performance regression introduced in 0.4.7. |
Let's not overstate this -- only your new microbenchmarks show so much difference. The rest of our benchmarks were mostly improved by the change, some drastically. And your own CI showed 12-18% regression in only 3 out of 70 benchmarks, the rest unchanged. That said, I do appreciate that you (and your AI) found this and provided a fix. Will it suffice to publish a 0.5.x update, or do you really need this in 0.4.x? |
At the risk of continuing an argument that didn't need to start: it's a JSON parser, most tests don't touch big ints at all. We only found this because codspeed's ci tool caught the regression, and their AI pointed me in the right direction. I think 55% is a fair description of the performance regression in this library's code in the worst case. Anyway, 0.5.x is fine. Thanks so much. |
Can you bench this on macOS before #307? If the 2-digit result is similar to this PR, we're probably ok. My Fedora (glibc) results for this PR are steady on the 2-digit case, as you predicted. |
|
Here are my results: on on this branch: on |
|
Thanks, that looks good! |
The below description is written with AI, but I've researched this at length manually, and I think on balance this change smoothes out the changes. See pydantic/jiter#264 (comment) for a severe example which I chanced upon.
BigDigits::pushmoves fromInlinetoHeapwith[*x, y].to_vec(), which allocates capacity exactly 2, so growth then runs 2 -> 4 -> 8. Before #307 the first allocation came fromVec::push, whose minimum non-zero capacity is 4 forBigDigitat either digit width, so growth ran 4 -> 8. Values that grow past one digit do one morereallocthan they did in 0.4.6, and each growth step happens one push earlier.Measured with the benchmarks this PR adds, Apple M3 Max, rustc 1.97.1-nightly:
scalar_accumulate_2_digitsscalar_accumulate_3_digitsscalar_accumulate_4_digitsscalar_accumulate_6_digitsscalar_accumulate_20_digitsThe 2-digit case gets slower. A value that reaches two digits and stops now takes a 32-byte allocation instead of 16, and nothing at the transition distinguishes it from a value that keeps growing. That measurement is macOS system malloc; 16 and 32 bytes may fall in the same chunk size class on glibc.
Found from jiter, which parses integers as repeated
x *= 10^18; x += chunk. Itsmassive_ints_arraybenchmark, 1000 integers of ~60 decimal digits or 4BigDigits, is 46-55% slower on 0.4.8 than on 0.4.6, and returns to 0.4.6 timings with this change.