Stream: git-wasmtime

Topic: wasmtime / PR #2020 Serialize and deserialize compilation...


view this post on Zulip Wasmtime GitHub notifications bot (Jul 14 2020 at 22:19):

yurydelendik opened PR #2020 from serde-module-example to main:

Fixes #1779

This is final patch in series to allow direct access to the compiled code.

PR opened for feedback.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 18:49):

yurydelendik updated PR #2020 from serde-module-example to main:

Fixes #1779

This is final patch in series to allow direct access to the compiled code.

PR opened for feedback.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:16):

yurydelendik has marked PR #2020 as ready for review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

Would it be possible to add this as a method as Module::serialize instead of a top-level function?

Additionally, I think it may be best to stick to in-memory buffers for now since the entire Module has to be resident in memory anyway. Using Read and Write can be somewhat tricky so I'm not sure we want to go there just yet (we could always add support later though if needed)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

Also like serialize I think it may be best to be conservative and start out with a byte slice instead of a Read

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

Could this include env!("CARGO_PKG_VERSION") as well to catch accidental bugs of reusing across wasmtime versions?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

Is this intended to be unsafe? Ideally we'd have this be a safe function I think.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

s/tripple/triple/

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

IIRC the C API already has wasm_module_{serialize,deserialize} so could those be implemented instead? (although these may also be good to keep around for better error messages)

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:56):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:56):

yurydelendik created PR Review Comment:

Would it be possible to add this as a method as Module::serialize instead of a top-level function?

Currently, I'm discarding some artifacts once CompiledModule is created due to memory usage. Keeping them around maybe wasteful at the moment. Also it is why I chose wasmtime_compile_and_serialize instead of wasmtime_module_serialize.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:59):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 15 2020 at 19:59):

yurydelendik created PR Review Comment:

Is this intended to be unsafe? Ideally we'd have this be a safe function I think.

I mentioned that it is marked unsafe since it does not provide any guarantees for safety of the ingested code. We don't want deal with code signing and currently rely on caller to that.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:57):

tschneidereit submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 10:57):

tschneidereit created PR Review Comment:

I wonder if we really want to mark this as unsafe. I get the intent behind it, but it's worth noting that the current API that transparently loads a cache entry if that exists has the same issue: since anybody could put whatever they want into the cache, the same "unsafety" exists.

@sunfishcode what's your take on this, do we have enough of a secure setup in place for the case where we are fully in control of the pipeline from compiling a wasm file to running it that we should call out the gap in safety like this?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:04):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:04):

alexcrichton created PR Review Comment:

Oh sorry I missed the comment in the PR description! I would personally feel though that we shouldn't have unsafe here. I think it's a good point to document that arbitrary bytes here is not truly safe and this shouldn't be fed untrusted code, but that feels like such a far away enough use case that it's not worth making this unsafe as a result.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:04):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:05):

alexcrichton created PR Review Comment:

Could you expand a bit on what's not kept in memory? What's the scale of the memory usage improvement as well?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:22):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:22):

yurydelendik created PR Review Comment:

Could you expand a bit on what's not kept in memory?

Currently the answer is ELF image is modified by fixing relocation, so I'm just patching data directly. That makes image non-serializable as is. It needs to be re-cloned and "unpatched" to be serializable. Also there unwind_info we don't need to keep after CompiledModule. FWIW I'm currently working on both of these problems, but there are require some time and code to address.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 16:23):

yurydelendik edited PR Review Comment.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 19:36):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 19:36):

alexcrichton created PR Review Comment:

Hm ok from an API perspective I think it'd be best to have Module::serialize for now, and perhaps we can address the performance issues later?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 21:34):

yurydelendik updated PR #2020 from serde-module-example to main:

Fixes #1779

This is final patch in series to allow direct access to the compiled code.

PR opened for feedback.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 22:21):

yurydelendik updated PR #2020 from serde-module-example to main:

Fixes #1779

This is final patch in series to allow direct access to the compiled code.

PR opened for feedback.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 22:23):

yurydelendik submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 22:23):

yurydelendik created PR Review Comment:

I took shortcut and start keeping data in CompiledModule that allows implement Module::serialize. I'll continue working on optimizing this part independent of this PR.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 23:20):

yurydelendik updated PR #2020 from serde-module-example to main:

Fixes #1779

This is final patch in series to allow direct access to the compiled code.

PR opened for feedback.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 16 2020 at 23:21):

yurydelendik updated PR #2020 from serde-module-example to main:

Fixes #1779

This is final patch in series to allow direct access to the compiled code.

PR opened for feedback.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2020 at 18:55):

alexcrichton submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2020 at 20:05):

yurydelendik merged PR #2020.


Last updated: Dec 23 2024 at 12:05 UTC