Move most module initialization to compiled code #13487
Conversation
github-actions
Bot
added
wasmtime:api
Subscribe to Label Actioncc @fitzgen DetailsThis 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 |
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
This looks largely good to me -- nice simplification! Some thoughts below but no blockers at all.
| .module | ||
| .global_initializers | ||
| .iter() | ||
| .find(|(def_index, _)| *def_index == index) |
There was a problem hiding this comment.
Would it make sense to sort global_initializers by def_index and do a binary search here? I know it's in the compilation path but I always have a vague worry when I see a linear search like this...
There was a problem hiding this comment.
Good point! It's already amortized a bit where I think make_global happens at most once-per-global-per-function, but the array is also already sorted so I added a binary search.
| expr: &ConstExpr, | ||
| ) -> WasmResult<ir::Value> { | ||
| let mut stack = Vec::new(); | ||
| for op in expr.ops() { |
There was a problem hiding this comment.
It's a little unfortunate that we have duplication between this logic and the same opcodes compiled in ordinary Wasm function bodies. I guess the compile-time state is different enough that we can't "just impl From<ConstOp> for Op and use the existing lowerings" but I wonder how far off we are from that. In any case, anything complicated below (e.g. struct.new) is delegated to a translate helper so maybe this is fine.
There was a problem hiding this comment.
Good point yeah, and looking into this I think the main blocker is the need for a FuncValidator<_> when translating an operator. That's not easily constructible for const-exprs currently from wasmparser's perspective. I'll file an issue on this though because it's a nice avenue for code deduplication (e.g. around fuel handling)
fitzgen
left a comment
fitzgen
left a comment
There was a problem hiding this comment.
Nothing to add on top of Chris's comments.
Instead return `None`. This helps prevent corruption of the GC heap from causing panics.
Even `ref.null func` is intern'd, so nothing is candidate for memset on a `funcref` array. Overall just ban memset on all reference-typed arrays for now to optimize the easy case of scalar data, but leave optimizing reference types for later.
This commit is a large refactoring of how modules are initialized in Wasmtime. Notably all of the work done post-allocation, but pre-start, is now done in compiled code instead. This means that global initialization, active table segments, passive segment evaluation, etc, now all happens in compiled code. The primary motivation for this is to resolve some GC-related fuzz-bugs where initialization on the host is taking an excessively long time. A secondary motivation is to apply fuel metering and epoch yielding to these constructs in the same manner that normal wasm code has these applied. Much refactoring was needed in this commit to achieve this goal. Many primitives were transitioned from runtime state to exclusively compile-time state for example. Infrastructure was additionally added for a new kind of `FuncKey` corresponding to this one-off-use startup function. Overall though the net effect of this change is to mostly delete code since so much of the runtime is now no longer necessary. An example of this is that const-eval is now completely removed from the runtime as the fully-general const-evaluation now happens exclusively through compiled code. Special care was needed here for the static table and memory initialization that Wasmtime performs. For example there's a small dance between compile-time and run-time where at compile-time we don't know if static data segments should be applied, and it's only at run-time where we know if CoW is in effect. Additionally care was taken throughout this refactoring to avoid generating this new startup function unless it's necessary. It's hypothesized that skipping this function is going to be a worthwhile optimization, which means that one mode of startup is configured as "only necessary if memories say `needs_init()`". This is a bit tricky to document and it's a bit non-standard, but it should get the job done (and existing tests exercise this already).
alexcrichton
force-pushed
the
jit-module-init
branch
from
9d650b6 to
1f3f64a
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
3 participants
This commit is a large refactoring of how modules are initialized in
Wasmtime. Notably all of the work done post-allocation, but pre-start,
is now done in compiled code instead. This means that global
initialization, active table segments, passive segment evaluation, etc,
now all happens in compiled code. The primary motivation for this is to
resolve some GC-related fuzz-bugs where initialization on the host is
taking an excessively long time. A secondary motivation is to apply fuel
metering and epoch yielding to these constructs in the same manner that
normal wasm code has these applied.
Much refactoring was needed in this commit to achieve this goal. Many
primitives were transitioned from runtime state to exclusively
compile-time state for example. Infrastructure was additionally added
for a new kind of
FuncKeycorresponding to this one-off-use startupfunction. Overall though the net effect of this change is to mostly
delete code since so much of the runtime is now no longer necessary. An
example of this is that const-eval is now completely removed from the
runtime as the fully-general const-evaluation now happens exclusively
through compiled code.
Special care was needed here for the static table and memory
initialization that Wasmtime performs. For example there's a small dance
between compile-time and run-time where at compile-time we don't know if
static data segments should be applied, and it's only at run-time where
we know if CoW is in effect. Additionally care was taken throughout this
refactoring to avoid generating this new startup function unless it's
necessary. It's hypothesized that skipping this function is going to be
a worthwhile optimization, which means that one mode of startup is
configured as "only necessary if memories say
needs_init()". This is abit tricky to document and it's a bit non-standard, but it should get
the job done (and existing tests exercise this already).