github-actions[bot] commented on Issue #1901:
Subscribe to Label Action
cc @bnjbvr, @peterhuene
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api"Thus the following users have been cc'd because of the following labels:
- bnjbvr: cranelift
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on Issue #1901:
That definitely sounds like a better design, albeit more implementation effort. How strongly do you feel vs waiting until we actually care about the performance of
funcref
?We statically know all possible indices that can be referenced by
ref.func
,I know that this is true at the spec level with the
refs
member of the validation context, but do you know if we actually retain this info anywhere? Not sure this is exposed bywasmparser
orcranelift_wasm
.
alexcrichton commented on Issue #1901:
I think it's fine to take the shortest path for now regardless of perf to a complete implementation of reference types, but looking forward to things like
table.copy
andtable.init
onfuncref
tables I feel like we're gonna be jumping through a lot of hoops trying to convert back and forth betweenVMCallerCheckedAnyfunc
andVMFuncRef
. I'm fine deferring to you, but it feels to me that it'll be basically equivalent the amount of work to take either strategy (in that I'm not sure one is that much more onerous than the other).Currently AFAIK the
refs
member isn't exposed, so cranelift-wasm would likely need to construct this.
fitzgen commented on Issue #1901:
My one concern with your proposed design is just figuring out where to stick the new side tables for
VMCallerCheckedAnyfunc
s and how to plumb that through everywhere. Its the kind of thing that seems like it might run into the annoyingwasmtime
/wasmtime-runtime
crate boundary again. I'll investigate a little more and see if I can't make it work.
alexcrichton commented on Issue #1901:
Perhaps we could temporarily allocate a
VMCallerCheckedAnyfunc
for every function in the module, and shove that into theVMContext
? That way the index of the function is the same as the index into that table. We could later optimize that to not allocate such a big table for functions that aren't referenced.
fitzgen commented on Issue #1901:
Pushed a wip commit that switches to using
*const VMCallerCheckedAnyfunc
forfuncref
s. It compiles at least, but tests are failing.
github-actions[bot] commented on Issue #1901:
Subscribe to Label Action
cc @fitzgen
<details>
This issue or pull request has been labeled: "fuzzing"Thus the following users have been cc'd because of the following labels:
- fitzgen: fuzzing
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on Issue #1901:
@alexcrichton okay I think this is ready for review again.
github-actions[bot] commented on Issue #1901:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:c-api"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:c-api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
fitzgen commented on Issue #1901:
@alexcrichton okay, I've deduped the null func refs into the store, and made
Extern::Func
non-nullable.Still need to write a couple tests that pass in
Val::Func(None)
to various things, but I think you can take another look a this while I do that.
alexcrichton commented on Issue #1901:
Looks great to me! I've got one final question though before landing, which I just thought of after reading this. Could we be even more clever and make the null funcref a literal null pointer? It looks like you were worried about the cost of
call_indirect
in terms of number of checks, but I think that it's the same number of checks as before, we're just testinganyfunc_ptr
for whether it's null instead offunc_addr
, which we assume is always non-null.With that in place translation of
ref.null
would be super simple, just a bag of zeros all the time!
fitzgen commented on Issue #1901:
Looks great to me! I've got one final question though before landing, which I just thought of after reading this. Could we be even _more_ clever and make the null funcref a literal null pointer? It looks like you were worried about the cost of
call_indirect
in terms of number of checks, but I think that it's the same number of checks as before, we're just testinganyfunc_ptr
for whether it's null instead offunc_addr
, which we assume is always non-null.With that in place translation of
ref.null
would be super simple, just a bag of zeros all the time!The concern wasn't about the path for
call_indirect
with a null funcref (which should be a super rare event, and leads to a trap anyways, so it isn't ever gonna be fast). The concern was introducing an extra potential cache miss in front of every non-trappingcall_indirect
before we actually end up calling the function.Originally, we couldn't use a null pointer for a null func ref, but after all the refactorings this PR has been through, that might not be the case anymore. Let me double check. Either way, I think
ref.null
will be cheap enough either way. But this actually made me discover a latent bug:cranelift-wasm
doesn't allow you customizeref.null
's translation (currently). It also always assumes that all references are of the reference type (which is not true forfuncref
s for us). So I have a bit more work to do here either way.
alexcrichton commented on Issue #1901:
Oh I'm not actually worried about the cost of
ref.null
, it's more that using a null pointer forref.null
felt clearer and easier to understand.Could you elaborate on the cache misses, though? With this current incantation a
call_indirect
is: load anyref, load funcptr, check null, load type, check match, call. With a null pointer representation it would instead be load anyref, check null, load type, check match, load funcptr, call. I'm not sure how the cache misses would be different there?
fitzgen commented on Issue #1901:
The extra potential cache miss is from changing table elements from
VMCallerCheckedAnyfunc
to a pointer to aVMCallerCheckedAnyfunc
(regardless whether that is a nullable pointer or not).It makes it so that there is an extra pointer chase to get to the
func_ptr
member of an element in the table.
alexcrichton commented on Issue #1901:
Ah ok, I remember talking about that yeah. I thought you meant that the extra cache miss was about the representation of the null function in the sense of is it a null pointer or is it a sentinel.
fitzgen commented on Issue #1901:
phew. okay.
- I made
ref.null func
be a null pointer, rather than aVMCallerCheckedAnyfunc
with a nullfunc_ptr
member.- similarly,
VMCallerCheckedAnyfunc::func_ptr
is nowNonNull
.- I added the extra
translate_*
hooks tocranelift_wasm::FuncEnvironment
to allow for non-uniform reference types representations.@alexcrichton I think this is ready for another (and hopefully the last!) round of review. Thanks for your patience with this one!
Last updated: Dec 23 2024 at 12:05 UTC