Support adding additional capacity to FreeLists
#10516
Conversation
fitzgen
force-pushed
the
freelist-add-capacity
branch
from
1a8028b to
04e7e10
Compare
| free_list | ||
| } | ||
|
|
||
| /// Add additional capacity to this free list. | ||
| #[allow(dead_code)] // TODO: becomes used in https://github.com/bytecodealliance/wasmtime/pull/10503 |
There was a problem hiding this comment.
Could the tests below exercise this new method?
| if layout.size() > self.max_size() { | ||
| let trap = crate::Trap::AllocationTooLarge; | ||
| let err = anyhow::Error::from(trap); | ||
| let err = err.context(format!( | ||
| "requested allocation's size of {} is greater than the max supported size of {}", | ||
| layout.size(), | ||
| self.max_size(), | ||
| )); | ||
| return Err(err); | ||
| } |
There was a problem hiding this comment.
To confirm, there's still a test below testing this behavior and that it fails?
There was a problem hiding this comment.
Correct, we just return an Ok(None) (meaning that the allocation might succeed after growth/gc) instead of Err(_) (meaning that the allocation will never succeed no matter what)
| // If we are adding capacity beyond what a `u32` can address, then we | ||
| // can't actually use that capacity, so don't bother adding a new block | ||
| // to the free list. | ||
| let old_cap_rounded = round_usize_down_to_pow2(old_cap, ALIGN_USIZE); |
There was a problem hiding this comment.
If allocations could previously go up to old_cap, shouldn't this round up instead of round down?
There was a problem hiding this comment.
No, because the old capacity was also rounded down the the alignment (all allocations are also a multiple of the alignment) so the first newly allocatable index for our additional capacity is that old capacity rounded down.
There was a problem hiding this comment.
Should this assert that old_cap is already aligned in that case? It mostly just seems counter-inutitive to me that you'd round down the end to figure out the index of the next block, I'd expect that you'd either round up or assert rounding isn't necessary
There was a problem hiding this comment.
If we keep self.capacity aligned down to our alignment, then a sequence of 100 add_capacity(1) calls will never actually result in the FreeList thinking it has capacity for an allocation, even though it actually should. That is, self.capacity = round_down_to_align(self.capacity + additional) will always result in self.capacity == 0 when self.capacity is initially 0 and additional is 1, even when it is called in a loop.
We have to keep track of the actual capacity, and just do the rounding when adding blocks into the free list.
There was a problem hiding this comment.
Ah ok yeah that makes sense, I was missing that subtelty. Mind adding some comments to that effect to this?
github-actions
bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:ref-types"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| // If we are adding capacity beyond what a `u32` can address, then we | ||
| // can't actually use that capacity, so don't bother adding a new block | ||
| // to the free list. | ||
| let old_cap_rounded = round_usize_down_to_pow2(old_cap, ALIGN_USIZE); |
There was a problem hiding this comment.
Ah ok yeah that makes sense, I was missing that subtelty. Mind adding some comments to that effect to this?
Split out from #10503