Conversation
Helps get some binaries to test with
Publish dev artifacts from CI
Test out the merge queue
…hooks into a separate module
Things left to complete: * Core wasm support * Improved error handling * Wasmtime CLI integration
… completed core wasm rr
|
But essentially in terms of logic, it didn't change anything at all. It was purely a move of almost everything in |
|
Also I also think this crate should live as a separate repo (under the bytecode alliance maybe, or my personal account), but I figure we can wait till review is complete to do that. |
|
Ah, I see. IMHO that's a bigger discussion whether we want to export a set of types that other engines also use -- that's a big new public API surface. Would you mind reverting that for now on this branch at least? I want to try to keep the review target stable, without new development, until we land it. Speaking of which -- @alexcrichton I know you're extremely busy and the holidays are coming up but do you have any estimate on when you might be able to give this PR a review? |
|
Oh no worries, I ended up not having the energy on planes but I was planning to get to this beginning of next week when things are quieter with other folks on vacation |
|
I'll move any additional development henceforth on a different branch then, if necessary. But for now, do you want me to also revert the new internal |
|
Yes; that's a big change to the public API (even if it's not much of a change to code structure) and I think we'll want to have a discussion about the way we handle this. |
|
Ok, it's moved back into wasmtime, leaving this branch as stable for review and fixes |
alexcrichton
left a comment
There was a problem hiding this comment.
Ok I feel like I've read over this enough to the point where I wouldn't claim an encyclopedic knowledge but I've got a good high-level idea of what's going on. Overall my impression is that we need to figure out how to split this up to land it piecmeal and incrementally in Wasmtime. To me there's just too much changing here in the guts of the runtime to be able to effectively review.
I've got a lot of questions/comments about various abstractions/etc added here and there. These range from things like "we should push harder on not storing WasmFuncOrigin" to " there's quite a lot of #[cfg] and I think it can be reduced" to "this API I think is too pub and wants to be pub(crate)" to "I don't think the new ValRaw API is safe" and things like that.
What I'd recommend personally is someting like this:
- Merge this PR here in this fork, and continue the process of merge in
mainevery so often here in the fork. - Incrementally peel off chunks of this repo as PRs-to-wasmtime.
- After the PR on wasmtime merges, merge the new
mainbranch of Wasmtime into this repo. - Repeat until the diff from this repo and Wasmtime is small enough to be one final PR
Chunks that land in Wasmtime are unlikely to be all that useful until the end, but given the complete picture in this repo it's easier to land untested/inactive chunks in Wasmtime, review them incrementally, and then have it all working in the end. This is what we did with component-model-async, for example, and it also helps with writing tests because each incremental PR can test what's possible at least, if not the full picture.
Chunks of this PR I can see landing incrementally would be:
Configknobs/validation- Generating a sha256 of the wasm file
- Misc refactorings such as
flat_types_storage_or_pointerand movement ofFlatTypesStorage - Extending core functions with new parameters like
RRWasmFuncType - Landing an
rr_disabled.rsmodule, for example, which has all the hooks they're just all noops.
And my hope is that'd at least reduce this diff a fair amount to make it more managable and it'd be more clear how to land the rest at that point. Is this someting that you'd be up for splitting up and landing?
One point I also want to clarify is that inevitably during review there will be comments that may change some fundamental design decisions here which require refactors/rebases in this repository itself. Personally I feel that's the review process "working as intended" but I mostly want to clarify that I don't mean for the goal of this process to be to land exactly this PR as-is a file-at-a-time for example. Instead I want to use the process of incremental PRs to give time and space to review each PR and each design decision on the way, possibly tweaking/updating them. This'll be much easier with a working implementation to validate ideas against (and reject ideas against), too.
|
Yeah, okay, that's understandable. One thing though is that tests might be difficult for most of these PRs until we land the last one. Perhaps that's okay though because we have the working version here. |
|
It's probably good practice to write independent unit tests for each part (as much as feasible) anyway -- I've found that to be useful when doing the debugger implementation (for example) too. |
|
Yeah I understand that the first PRs won't be able to have like an end-to-end test and some PRs may not be testable at all. That's ok though because the full implementation lives here and has tests running/passing so I'm not too worried about that. The rough idea is to test what you can in incremental PRs and defer the rest to later (or also file issues for tests we want to write to get filled out later) |
This reverts commit 7c4a367. Post-return needs to be called unconditionally for component model.
…rn with empty args
…both threaded and non-threaded variants
Notes
Func,TypedFunc, andComponentFuncasyncStructure
rrfeature. The only things exposed without the feature arerr::hooks, which offers convenient methods to hook recording/replaying mechanisms into the rest of the engine.As a result,
rrwithouthooksshould be able to land with no impact on existing Wasmtime whatsoever.Observed overheads
Between 4-8%