Refactor vm #2281
Refactor vm #2281
Conversation
|
bors try |
tryBuild failed: |
This reverts commit 7e7b930.
|
bors try- |
|
bors try |
tryBuild failed: |
|
bors try |
| @@ -1285,6 +1167,7 @@ mod inner { | |||
|
|
|||
| /// An empty struct to help Rust typing to determine | |||
| /// when a `HostFunction` does not have an environment. | |||
| #[derive(Clone)] | |||
There was a problem hiding this comment.
Cloning an empty struct? Why? Is the struct just used in the type system? Or is it compared by pointer?
There was a problem hiding this comment.
It's required for WasmerEnv to be clonable
| std::ptr::copy_nonoverlapping(src_pointer, | ||
| rets_list, | ||
| num_rets); |
There was a problem hiding this comment.
Indentation looks wrong? Does this pass lint?
There was a problem hiding this comment.
Yeah, weird. This change was made by the linter
There was a problem hiding this comment.
The body of macros are not formatted by rustfmt. If the formatting looks wrong, it has to be fixed manually. It was likely caused by your editor if you didn't do it
| #[derive(Debug, Clone)] | ||
| pub struct VMExportMemory { | ||
| #[derive(Debug, Clone, MemoryUsage)] | ||
| pub struct VMMemory { |
There was a problem hiding this comment.
No action required. FWIW I think that naming these things VM* is a bug. It's not a new bug introduced in your PR. VM* should mean "used by the VM" as in "the generated code uses this" which implies "must be repr(C) and contain things that are available in repr(C)" which would exclude Arc for instance (I've been arguing we should make a VMArc).
|
bors r+ |
|
Build succeeded: |
There was a problem hiding this comment.
Some of the refactoring here looks really good, I didn't realize some of that was possible. The large PR and touching so many critical pieces made me concerned that this PR introduced a large breaking change that got past our tests but upon further review I don't think that's the case (at least my initial concerns were wrong). I haven't been able to fully review all the changes in this PR yet though.
| FunctionDefinition::Wasm(wasm) => { | ||
| self.call_wasm(&wasm, params, &mut results)?; | ||
| } | ||
| // TODO: we can trivially hit this, look into it |
There was a problem hiding this comment.
Probably shouldn't remove this comment unless it's been fixed by this PR
| std::ptr::copy_nonoverlapping(src_pointer, | ||
| rets_list, | ||
| num_rets); |
There was a problem hiding this comment.
The body of macros are not formatted by rustfmt. If the formatting looks wrong, it has to be fixed manually. It was likely caused by your editor if you didn't do it
Description
This PR simplifies the VM code. and improves it's resiliency:
Review