Wasmtime GitHub notifications bot (Jun 18 2020 at 19:10): Wasmtime GitHub notifications bot (Jun 18 2020 at 19:10):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>
Wasmtime GitHub notifications bot (Jun 18 2020 at 20:01):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
refsmember of the validation context, but do you know if we actually retain this info anywhere? Not sure this is exposed bywasmparserorcranelift_wasm.
Wasmtime GitHub notifications bot (Jun 18 2020 at 20:05):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.copyandtable.initonfuncreftables I feel like we're gonna be jumping through a lot of hoops trying to convert back and forth betweenVMCallerCheckedAnyfuncandVMFuncRef. 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
refsmember isn't exposed, so cranelift-wasm would likely need to construct this.
Wasmtime GitHub notifications bot (Jun 18 2020 at 20:16):fitzgen commented on Issue #1901:
My one concern with your proposed design is just figuring out where to stick the new side tables for
VMCallerCheckedAnyfuncs and how to plumb that through everywhere. Its the kind of thing that seems like it might run into the annoyingwasmtime/wasmtime-runtimecrate boundary again. I'll investigate a little more and see if I can't make it work.
Wasmtime GitHub notifications bot (Jun 18 2020 at 20:25):alexcrichton commented on Issue #1901:
Perhaps we could temporarily allocate a
VMCallerCheckedAnyfuncfor 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.
Wasmtime GitHub notifications bot (Jun 18 2020 at 23:44):fitzgen commented on Issue #1901:
Pushed a wip commit that switches to using
*const VMCallerCheckedAnyfuncforfuncrefs. It compiles at least, but tests are failing.
Wasmtime GitHub notifications bot (Jun 22 2020 at 22:08):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>
Wasmtime GitHub notifications bot (Jun 22 2020 at 22:52):fitzgen commented on Issue #1901:
@alexcrichton okay I think this is ready for review again.
Wasmtime GitHub notifications bot (Jun 22 2020 at 23:17):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>
Wasmtime GitHub notifications bot (Jun 23 2020 at 18:55):fitzgen commented on Issue #1901:
@alexcrichton okay, I've deduped the null func refs into the store, and made
Extern::Funcnon-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.
Wasmtime GitHub notifications bot (Jun 23 2020 at 19:09):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_indirectin terms of number of checks, but I think that it's the same number of checks as before, we're just testinganyfunc_ptrfor whether it's null instead offunc_addr, which we assume is always non-null.With that in place translation of
ref.nullwould be super simple, just a bag of zeros all the time!
Wasmtime GitHub notifications bot (Jun 23 2020 at 20:55):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_indirectin terms of number of checks, but I think that it's the same number of checks as before, we're just testinganyfunc_ptrfor whether it's null instead offunc_addr, which we assume is always non-null.With that in place translation of
ref.nullwould be super simple, just a bag of zeros all the time!The concern wasn't about the path for
call_indirectwith 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_indirectbefore 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.nullwill be cheap enough either way. But this actually made me discover a latent bug:cranelift-wasmdoesn't allow you customizeref.null's translation (currently). It also always assumes that all references are of the reference type (which is not true forfuncrefs for us). So I have a bit more work to do here either way.
Wasmtime GitHub notifications bot (Jun 23 2020 at 21:18):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.nullfelt clearer and easier to understand.Could you elaborate on the cache misses, though? With this current incantation a
call_indirectis: 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?
Wasmtime GitHub notifications bot (Jun 23 2020 at 21:24):fitzgen commented on Issue #1901:
The extra potential cache miss is from changing table elements from
VMCallerCheckedAnyfuncto 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_ptrmember of an element in the table.
Wasmtime GitHub notifications bot (Jun 23 2020 at 21:30):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.
Wasmtime GitHub notifications bot (Jun 23 2020 at 23:41):fitzgen commented on Issue #1901:
phew. okay.
- I made
ref.null funcbe a null pointer, rather than aVMCallerCheckedAnyfuncwith a nullfunc_ptrmember.- similarly,
VMCallerCheckedAnyfunc::func_ptris nowNonNull.- I added the extra
translate_*hooks tocranelift_wasm::FuncEnvironmentto 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: Nov 22 2024 at 16:03 UTC