boxed the wasmer_instance field of Instance to make it safe to reference #306
Conversation
There was a problem hiding this comment.
Thank you for taking the time to look into this. This does look like a valid way to hold a reference.
In the meantime, we got pre-allocated fields working well with up to 128kB and minimal gas cost, which should cover any foreseeable use case. We also have great unit test coverage of all the context callbacks. (See cosmwasm_vm::imports). This uses a stub instance that holds context data, but we never execute the wasm code. Instead we set items in the wasm memory space and simulate a callback. This is much easier to test than the other integration code where we need to recompile the wasm testdata/contract_0.8.wasm to properly test it.
Unless you see a large benefit without interfering with the ease of unit testing, I would just put this on ice. I don't see the advantage of using this in CosmWasm. But it would be nice to add this link to the wasmer instance to demonstrate how it is possible. (Other people are also wondering, not just for memory allocation).
| @@ -18,7 +18,6 @@ use cosmwasm_std::{ | |||
| QuerierResponse, QueryRequest, Storage, | |||
| }; | |||
|
|
|||
| #[cfg(feature = "iterator")] | |||
There was a problem hiding this comment.
The imports under it are needed in your PR, but they are not available without this feature enabled. this caused a compilation fail when not using this feature.
| @@ -121,7 +121,14 @@ where | |||
| gas_limit: u64, | |||
| ) -> Self { | |||
| set_gas(&mut wasmer_instance, gas_limit); | |||
| let instance_ptr = NonNull::from(&mut wasmer_instance); | |||
|
|
|||
| // The pointer shenanigans below are sound because: | |||
There was a problem hiding this comment.
Seems like a safe way to get a reference, but not tested with actually calling it.
There was a problem hiding this comment.
true. still requires work, of course
|
This is a super interesting learning, thank you @reuvenpo When I'm done with a few other things, I'll integrate this into my PoC PR and update my PR to latest master. Even if this is low priority, I'd like to build up knowledge about accessing the Wasmer instance from the Wasmer context, which we might need for other purposes as well.
This works for us just because the singlepass metering system charges the same amount of gas for the |
webmaster128
force-pushed
the
read_db_allocation
branch
2 times, most recently
from
776411e to
61d6089
Compare
This PR modifies the original draft PR to add boxing of the wasmer instance. This makes creating a pointer to it valid, as long as it is not shared between thread (Which isn't possible for the
Instanceanyways).For now I opened this just as a demonstration. A lot of APIs have changed and i'm too tired to fix up all the conflicts with master at the moment.