fix: Saturate NativeBuffer growth instead of overflowing past half of int.MaxValue - #419
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes NativeBuffer<TItem, TTracker>.Grow so its exponential (2x) growth strategy saturates at the largest addressable element count instead of throwing an OverflowException once the buffer length exceeds int.MaxValue / 2. It keeps amortized growth behavior up to the existing size ceiling, while preserving the prior failure mode for genuinely unaddressable requests (overflow at the byte-size calculation).
Changes:
- Refactors growth count arithmetic into
NativeBuffer<,>.ComputeGrowCount(...)to avoid overflow and saturate at the maximum addressable element count. - Updates
Growto use the new saturating arithmetic before computing byte size in acheckedcontext. - Adds focused unit tests for boundary behavior without allocating multi-gigabyte buffers.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Apache.Arrow/Memory/NativeBuffer.cs |
Introduces saturating growth-count calculation and updates Grow to use it. |
test/Apache.Arrow.Tests/NativeBufferTests.cs |
Adds unit tests covering doubling behavior, saturation near limits, and per-element-size ceilings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| internal static int ComputeGrowCount(int length, int newElementCount, int elementSize) | ||
| { | ||
| int maxCount = int.MaxValue / elementSize; | ||
| long doubled = (long)length * 2; | ||
| return (int)Math.Max(newElementCount, Math.Min(doubled, maxCount)); | ||
| } |
| /// past the largest buffer that can be addressed, and never below what the caller asked for. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// Doubling used to be unconditional and checked, so once the buffer passed half of the maximum |
There was a problem hiding this comment.
Okay, that's just a dumb comment. I should have told the AI not to submit the PR until after I'd reviewed it.
|
The failing All the C# checks are green — Lint, Source, Happy to rebase if |
… int.MaxValue Grow doubled the current length in a checked context without saturating, so once a buffer passed half of the addressable maximum its next grow threw OverflowException however small the requested increase and even though the requested size still fit. For a byte buffer that was a hard ceiling near 1 GiB. The TODO those lines carried described exactly this. Growth now saturates at the largest addressable element count, keeping it amortised right up to the ceiling. A request that genuinely cannot be addressed still fails at the byte-size calculation, as before, so behaviour is unchanged for anything that could not have worked. The count arithmetic is extracted so it can be tested at the boundary without allocating more than a gigabyte in a unit test, including the per-element-size ceiling: the limit is a byte count, so a wider element type saturates at proportionally fewer elements. Closes apache#418.
Addresses review feedback on apache#419. `elementSize` is only ever `Unsafe.SizeOf<TItem>()` for an unmanaged `TItem`, so it cannot be zero or negative and the division cannot fault; the parameter exists so the boundary can be tested without allocating a buffer of that size. A `Debug.Assert` states that invariant for callers of the internal helper without adding a runtime check to the growth path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…remarks Grow already throws ObjectDisposedException on entry, so re-checking _owner before Reallocate was dead code. The <remarks> block narrated the history of the bug being fixed, which belongs in the commit message and the pull request rather than in the API documentation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
62effc1 to
bedad88
Compare
What's Changed
Growdoubled the current length in acheckedcontext without saturating:So once a buffer passed half of the addressable maximum, its next grow threw
OverflowExceptionhowever small the requested increase, and even though the requested size still fit. For a
NativeBuffer<byte, …>that is a hard ceiling near 1 GiB, with no way for a caller to work around it:asking for a smaller increment does not help, because the overflow is in the doubling rather than in
the request.
Growth now saturates at the largest addressable element count, so it stays amortised right up to the
ceiling. A request that genuinely cannot be addressed still fails at the byte-size calculation, as it
did before — behaviour is unchanged for anything that could not have worked.
This is what the
TODOthose lines carried proposed:On testing it
Reaching the boundary through
Growmeans allocating more than a gigabyte, which does not belong ina unit test. The count arithmetic is extracted to
ComputeGrowCountso the boundary can be testeddirectly and exhaustively, including the per-element-size ceiling — the limit is a byte count, so a
wider element type saturates at proportionally fewer elements.
Verified the new tests fail against the previous arithmetic before fixing it: with
checked(Length * 2)restored,ComputeGrowCountSaturatesInsteadOfOverflowingandComputeGrowCountSaturatesPerElementSizeboth fail; the rest pass either way.Apache.Arrow.Testsis green on net8.0: 1870 passed, 28 skipped (the Python interop cases).Scope
This does not change the 2 GiB ceiling on
ArrowBufferitself (ReadOnlyMemory<byte>,int Length)— it only stops buffers failing at half of it.
Closes #418.