Wasmtime GitHub notifications bot (Mar 21 2024 at 15:31): Wasmtime GitHub notifications bot (Mar 21 2024 at 15:31):jameysharp opened PR #8206 from jameysharp:table-base-readonly to bytecodealliance:main:
Fixes #8200
I'd appreciate careful review of my claim that the table base-address won't change as long as the table declaration has a fixed size. If I'm wrong about that then this change isn't safe.
Wasmtime GitHub notifications bot (Mar 21 2024 at 15:31):jameysharp requested alexcrichton for a review on PR #8206.
Wasmtime GitHub notifications bot (Mar 21 2024 at 15:31):jameysharp requested wasmtime-core-reviewers for a review on PR #8206.
Wasmtime GitHub notifications bot (Mar 21 2024 at 15:59):alexcrichton submitted PR review:
I believe this is correct, yes. Can you add an assertion here with a comment indicating why it's important to preserve the base pointer? That's I believe the only possible method where a table can change its base pointer, so having a comment there referencing this optimization I think would also be good.
Wasmtime GitHub notifications bot (Mar 21 2024 at 20:43):fitzgen commented on PR #8206:
Note that it is theoretically possible for the base pointer to remain constant even while supporting growth, if we are just growing within one large pre-allocated or pre-reserved virtual memory region. It may or may not be worth adding support for this subtlety to
cranelift-wasm.
Wasmtime GitHub notifications bot (Mar 21 2024 at 20:44):fitzgen edited a comment on PR #8206:
Note that it is theoretically possible for the base pointer to remain constant even while supporting growth, if we are just growing within one large pre-allocated or pre-reserved memory region. It may or may not be worth adding support for this subtlety to
cranelift-wasm.
Wasmtime GitHub notifications bot (Mar 21 2024 at 22:12):jameysharp commented on PR #8206:
Yeah, Nick, when I looked at the code Alex pointed at, I noticed that it looks like the pooling allocator never resizes the backing store for tables. So maybe we can thread that information through?
The decision of whether to use the
readonlyflag or not is in the wasmtime-cranelift crate so it's not a big deal to rely on Wasmtime internal details in making that decision. I haven't dug into how well connected those parts are though.
Wasmtime GitHub notifications bot (Mar 22 2024 at 00:00):alexcrichton commented on PR #8206:
What you're describing is I believe one property of "static" vs "dynamic" memories, namely that static memories never change their base pointer but dynamic memories are allowed. There's lots of knobs for that in Wasmtime but the problem here is we'd have to be able to classify static/dynamic at compile time and then make sure it matches the settings at runtime. Currently the pooling allocator isn't consulted at compile time, only runtime, so pooling-vs-not wouldn't be suitable for this optimization.
We could of course, though, add more flags and more config options like memories but for tables to control this.
Wasmtime GitHub notifications bot (Mar 22 2024 at 13:52):fitzgen commented on PR #8206:
Yep, I think that's something we may want to do in the long term.
Wasmtime GitHub notifications bot (Mar 22 2024 at 18:51):jameysharp commented on PR #8206:
Oh right, causality goes the wrong way here. I do like the idea of eventually adding a compile-time option for "no table backing-store resizes" that lets us set the
readonlyflag even for tables which might grow, and then turningtable.growrequests that overflow the backing store into runtime errors.Until we do that, though: Alex, I'm not sure what assertion I can add at that call to
Vec::resize. I'm happy to add comments. But in the case where this PR requires non-moving backing stores, theresizecall is guarded immediately above by checking that the new size does not exceedself.maximum(). So the only way it's reachable is if this function is called with a sizedeltaof zero, and I think in that case it would be better to return earlier and always succeed.What would you suggest doing there?
Wasmtime GitHub notifications bot (Mar 22 2024 at 19:28):alexcrichton commented on PR #8206:
Hm no you're right, I was assuming we had more context there than we actually do. I was envisioning an assert along the lines of "assert the type doesn't have max == Some(min)" but that can't be done.
In lieu of that I think it's ok to leave a comment on the check for max and say that this is needed by the spec but also for soundness in cranelift to enable this optimization. It's a bit redundant but it does seem good to have a mention of this optimization somewhere in
grow
Wasmtime GitHub notifications bot (Mar 26 2024 at 00:52):jameysharp updated PR #8206.
Wasmtime GitHub notifications bot (Mar 26 2024 at 00:54):jameysharp commented on PR #8206:
Okay, I've added stuff to the
growmethod in wasmtime-runtime. Would you give that part another look? I also rebased for other changes that have landed meanwhile.
Wasmtime GitHub notifications bot (Mar 26 2024 at 01:30):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Mar 26 2024 at 01:57):alexcrichton merged PR #8206.
Last updated: Nov 22 2024 at 17:03 UTC