Wasmtime GitHub notifications bot (Apr 02 2025 at 20:59): Wasmtime GitHub notifications bot (Apr 02 2025 at 20:59):fitzgen requested dicej for a review on PR #10516.
Wasmtime GitHub notifications bot (Apr 02 2025 at 20:59):fitzgen opened PR #10516 from fitzgen:freelist-add-capacity to bytecodealliance:main:
Split out from https://github.com/bytecodealliance/wasmtime/pull/10503
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (Apr 02 2025 at 20:59):fitzgen requested wasmtime-core-reviewers for a review on PR #10516.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:16):FrankReh submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:16):FrankReh created PR review comment:
"and free_list way we" I was not able to parse.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:23):fitzgen updated PR #10516.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:25):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:25):fitzgen created PR review comment:
I just reversed these changes, so that it still calls
reset, sinceresetcan't be removed until the rest of the changes from #10503 are present. Will make sure I don't accidentally remove words from the comment somehow when eventually inlining what used to beresetin that PR.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:27):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:27):alexcrichton created PR review comment:
Could the tests below exercise this new method?
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:27):alexcrichton created PR review comment:
To confirm, there's still a test below testing this behavior and that it fails?
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:27):alexcrichton created PR review comment:
If allocations could previously go up to
old_cap, shouldn't this round up instead of round down?
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:30):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:30):fitzgen created PR review comment:
Correct, we just return an
Ok(None)(meaning that the allocation might succeed after growth/gc) instead ofErr(_)(meaning that the allocation will never succeed no matter what)
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:38):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:38):fitzgen created PR review 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.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:43):fitzgen updated PR #10516.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:43):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:43):fitzgen created PR review comment:
Done
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:45):fitzgen updated PR #10516.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:55):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Apr 02 2025 at 21:55):alexcrichton created PR review 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
Wasmtime GitHub notifications bot (Apr 02 2025 at 23:45):github-actions[bot] commented on PR #10516:
Subscribe to Label Action
cc @fitzgen
<details>
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:
- fitzgen: wasmtime:ref-types
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 05 2025 at 05:13):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 05 2025 at 05:13):fitzgen created PR review comment:
If we keep
self.capacityaligned down to our alignment, then a sequence of 100add_capacity(1)calls will never actually result in theFreeListthinking 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 inself.capacity ==0whenself.capacityis initially0andadditionalis1, 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.
Wasmtime GitHub notifications bot (Apr 05 2025 at 05:13):fitzgen edited PR review comment.
Wasmtime GitHub notifications bot (Apr 07 2025 at 14:43):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Apr 07 2025 at 14:43):alexcrichton created PR review comment:
Ah ok yeah that makes sense, I was missing that subtelty. Mind adding some comments to that effect to this?
Wasmtime GitHub notifications bot (Apr 07 2025 at 17:15):fitzgen updated PR #10516.
Wasmtime GitHub notifications bot (Apr 07 2025 at 17:15):fitzgen submitted PR review.
Wasmtime GitHub notifications bot (Apr 07 2025 at 17:15):fitzgen created PR review comment:
Added
Wasmtime GitHub notifications bot (Apr 07 2025 at 17:16):fitzgen has enabled auto merge for PR #10516.
Wasmtime GitHub notifications bot (Apr 07 2025 at 17:30):fitzgen updated PR #10516.
Wasmtime GitHub notifications bot (Apr 07 2025 at 18:03):fitzgen merged PR #10516.
Last updated: Apr 17 2025 at 16:03 UTC