fix: remove incorrect debug assertion in BatchCoalescer

[Tim-53]

Which issue does this PR close?

• Closes debug_assert_eq! in BatchCoalescer panics in debug mode when batch_size < 4 #9506

Rationale for this change

Vec::reserve(n) does not guarantee exact capacity, Rust's MIN_NON_ZERO_CAP optimization means reserve(2) gives capacity = 4 for most numeric types, causing debug_assert_eq!(capacity, batch_size) to panic in debug mode when batch_size < 4.

What changes are included in this PR?

Replace reserve with reserve_exact in ensure_capacity in both InProgressPrimitiveArray and InProgressByteViewArray. reserve_exact skips the amortized growth optimization and allocates exactly the requested capacity, making the assertion correct.

Are these changes tested?

No. This only fixes an incorrect debug assertion.

Are there any user-facing changes?

No

[cetra3]

This should be a change to the debug_assert to be >= rather than here I think? Reserving exact will increase memory fragmentation

[alamb]

This should be a change to the debug_assert to be >= rather than here I think? Reserving exact will increase memory fragmentation

I agree fixing the assert might be best. The idea with the assert as I recall was to ensure that the memory growth is as expected. Maybe we can find a way to write a test instead that does the check and remove the debug assert entirely 🤔

[Dandandan]

I agree, updating the debug (or even removing with the test) should be the fix.

[mbutrovich]

Thanks for working on this, I'm hitting it as well :(

[Tim-53]

Just updated the PR to remove the debug_assert_eq! instead of weakening it to >=.
The assertion was originally added in #9132 as part of an anti-over-allocation optimization. Changing to >= silently lose that invariant. But i agree removing the assertion here is the right call.

I'm not sure what a meaningful replacement test would look like here, since the assertion was guarding internal memory behavior rather than observable output. Happy to add a test if someone has a good idea what to check here.

[Dandandan]

LGTM

[Dandandan]

This should be a change to the debug_assert to be >= rather than here I think? Reserving exact will increase memory fragmentation

Btw - I didn't really understand this part? In this code using reserve_exact shouldn't lead to memory fragmentation?
Note that we always target max self.batch_size in this code (it will never exceed this).

[cetra3]

The standard reservation strategy for Vec is powers of two, which plays nicer with allocators when you are allocating a lot of memory. If you start allocating exact sizes, then it is has to "slot" in an allocation with the rest of the system, and so you end up allocating more than that anyway.

[Dandandan]

The standard reservation strategy for Vec is powers of two, which plays nicer with allocators when you are allocating a lot of memory. If you start allocating exact sizes, then it is has to "slot" in an allocation with the rest of the system, and so you end up allocating more than that anyway.

Ah good one, yeah that is true. Although batch_size is in practice always set to a power of 2 as well...

[alamb]

I agree -- this looks good to me too now. Removing the over zealous assert 👍

[Tim-53]

This should be a change to the debug_assert to be >= rather than here I think? Reserving exact will increase memory fragmentation

Btw - I didn't really understand this part? In this code using reserve_exact shouldn't lead to memory fragmentation? Note that we always target max self.batch_size in this code (it will never exceed this).

I think when I wrote this, I was thinking of cases where the allocation size could change over time and trigger multiple reallocations.