Stream: git-wasmtime

Topic: wasmtime / PR #8206 Make fixed-size table base-address lo...


view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 15:31):

jameysharp requested alexcrichton for a review on PR #8206.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 21 2024 at 15:31):

jameysharp requested wasmtime-core-reviewers for a review on PR #8206.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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 readonly flag 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.

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip 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 readonly flag even for tables which might grow, and then turning table.grow requests 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, the resize call is guarded immediately above by checking that the new size does not exceed self.maximum(). So the only way it's reachable is if this function is called with a size delta of zero, and I think in that case it would be better to return earlier and always succeed.

What would you suggest doing there?

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 00:52):

jameysharp updated PR #8206.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 00:54):

jameysharp commented on PR #8206:

Okay, I've added stuff to the grow method in wasmtime-runtime. Would you give that part another look? I also rebased for other changes that have landed meanwhile.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 01:30):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 26 2024 at 01:57):

alexcrichton merged PR #8206.


Last updated: Dec 23 2024 at 12:05 UTC