Wasmtime GitHub notifications bot (Nov 24 2024 at 04:05): Wasmtime GitHub notifications bot (Nov 24 2024 at 04:05):sunshowers requested alexcrichton for a review on PR #9668.
Wasmtime GitHub notifications bot (Nov 24 2024 at 04:05):sunshowers requested wasmtime-core-reviewers for a review on PR #9668.
Wasmtime GitHub notifications bot (Nov 24 2024 at 04:05):sunshowers opened PR #9668 from sunshowers:memory-pool-aligned to bytecodealliance:main:
Part of work to centralize memory management within Mmap instances -- some of that work becomes easier if the byte counts are known to be aligned.
There were a few overflow cases that I added checks for, and I also added a proptest impl for
HostAlignedByteCount.
Wasmtime GitHub notifications bot (Nov 24 2024 at 04:12):sunshowers submitted PR review.
Wasmtime GitHub notifications bot (Nov 24 2024 at 04:12):sunshowers created PR review comment:
Hm I guess it can be added but it would have to be up to the last representable page, not
usize::MAX. Haven't seen a good use case for it though.
Wasmtime GitHub notifications bot (Nov 25 2024 at 15:36):alexcrichton submitted PR review:
Looks great to me, thanks! Just some minor things.Looks great to me, thanks! Just some minor things.
Wasmtime GitHub notifications bot (Nov 25 2024 at 15:36):alexcrichton created PR review comment:
I think this is fine to stay as infallible since the
SlabLayoutstructure means "everything has been validated" and thecalculatemethod below should have returned an error if this were otherwise to fail
Wasmtime GitHub notifications bot (Nov 25 2024 at 15:36):alexcrichton created PR review comment:
Could this use
new_uncheckedto get the debug assert there? Naively it feels like there's probably some corner case that produces an un-aligned count but I would also readily believe that some amount of math can be applied here to prove that the output is always aligned as well. In that sense I don't doubt this, but it might be good to have a debug-assert though? (or otherwise if you wouldn't mind writing out some math to show why the output is always aligned)
Wasmtime GitHub notifications bot (Nov 25 2024 at 15:36):alexcrichton created PR review comment:
True yeah, but agreed it's reasonable to skip the implementation for now.
Wasmtime GitHub notifications bot (Nov 25 2024 at 15:36):alexcrichton submitted PR review:
Looks great to me, thanks! Just some minor things.
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:45):sunshowers submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:45):sunshowers created PR review comment:
mmm actually I don't know why I wrote this, I thought I returned usize for this... sorry!
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:47):sunshowers submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:47):sunshowers created PR review comment:
Oh, hmm, actually this is fine I think. N * page size % M * page size will always be some multiple of the page size.
Wasmtime GitHub notifications bot (Nov 25 2024 at 17:48):sunshowers edited PR review comment.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:27):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 18:27):alexcrichton created PR review comment:
Yeah I suspect that this is always aligned now too, but I think it'd be good to have a comment or debug assert to help readers agree too
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:28):sunshowers updated PR #9668.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:28):sunshowers submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:28):sunshowers created PR review comment:
DOne.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:28):sunshowers edited PR review comment.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:28):sunshowers submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:28):sunshowers created PR review comment:
Attempted to explain this -- does that look good?
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:37):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:37):alexcrichton has enabled auto merge for PR #9668.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:43):sunshowers commented on PR #9668:
Oh haha, some of the old functions are no longer in use. Fixing.
Wasmtime GitHub notifications bot (Nov 25 2024 at 19:47):sunshowers updated PR #9668.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:14):sunshowers commented on PR #9668:
Should be good to go again -- I copied the feature flag for the only use. I'll remove this function entirely in a subsequent PR.
Wasmtime GitHub notifications bot (Nov 25 2024 at 20:33):alexcrichton merged PR #9668.
Last updated: Dec 23 2024 at 12:05 UTC