Repository navigation
Size new arena chunks from the previous chunk's capacity - #65031
Conversation
Arena.NewSlice computed the next chunk size from len(a.data), but it grows whenever the request does not fit in the remaining space, which usually happens before the current chunk is full. Doubling the used length rather than the chunk capacity could make the next chunk the same size as, or smaller than, the one being replaced. Arenas that mostly serve slices then kept allocating small chunks and never reached the intended steady-state size. Base the growth on cap(a.data) so each chunk doubles the previous one up to the size ceiling. Arena.New only grows when len == cap, so it already behaved correctly; it now uses cap as well for consistency. Co-authored-by: Steven Chong <25894545+teamchong@users.noreply.github.com>
|
TypeScript Bot (@typescript-bot) perf test this faster |
|
This one's definitely an oops on my part |
There was a problem hiding this comment.
🟡 Changes recommended
A regression test is still needed for growth from a partially used arena chunk, since existing assertions do not distinguish backing-capacity growth from separate allocations.
1 open issue
What changed in this PR
This PR fixes chunk growth in the native compiler’s arena allocator so slice-heavy workloads can reach the intended chunk size.
Changes:
- Base
Arena.NewSlicegrowth on the previous chunk’s capacity rather than its used length. - Use capacity in
Arena.Newfor consistency, without changing its behavior.
| File | Description |
|---|---|
tsc/internal/core/arena.go |
Uses chunk capacity when calculating the next allocation size. |
Review effort: Balanced
|
One or more custom setup steps configured for this repository failed during this Copilot code review run: Setup steps run before each review. If the review above is missing context, or no review was posted at all, the failing step above may be the cause. See the workflow run for failure details, fix your setup steps configuration, and re-request a review. Note You can configure setup steps for Copilot code review separately from Copilot cloud agent with a |
The existing arena tests only check the slices NewSlice returns, and those checks pass whether a slice is carved from a new chunk or allocated on its own. So they could not catch next-chunk sizing based on the used length instead of the chunk capacity. The new package-internal test starts from a partially used chunk and requests more than the remaining space. It checks that the replacement chunk grows from the previous capacity and that the slice is carved from it.
The test asserted the arena's private chunk capacity, pinning one implementation detail rather than observable behavior. It adds maintenance cost without guarding anything users can see, so drop it.
|
Jake Bailey (@jakebailey) Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Ryan Cavanaugh (RyanCavanaugh)
left a comment
There was a problem hiding this comment.
The NewSlice growth calculation correctly needs the previous capacity, not its partially consumed length; New remains equivalent because it only grows at length equal to capacity. Existing arena tests pass. No issues found.

Arena.NewSlice computed the next chunk size from len(a.data), but it grows whenever the request does not fit in the remaining space, which usually happens before the current chunk is full. Doubling the used length rather than the chunk capacity could make the next chunk the same size as, or smaller than, the one being replaced. Arenas that mostly serve slices then kept allocating small chunks and never reached the intended steady-state size.
Base the growth on cap(a.data) so each chunk doubles the previous one up to the size ceiling. Arena.New only grows when len == cap, so it already behaved correctly; it now uses cap as well for consistency.
Thanks Steven (@teamchong) for finding this!