Stream: git-wasmtime

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

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.

• Added compile_and_serialize API function that allows to save CompilationArtifacts object in binary format (+target/config fingerprint)
• Added unsafe Module::deserialize API function that allows to read the above data and create Module object. Marked unsafe since it does not provide any guarantees for safety of the ingested code.
• Portability limitation is that producer and consumer'sConfigs must have the same host triple, codegen flags, and Tunables. (Can we ignore CompilerStrategy?)

PR opened for feedback.

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.

• Added compile_and_serialize API function that allows to save CompilationArtifacts object in binary format (+target/config fingerprint)
• Added unsafe Module::deserialize API function that allows to read the above data and create Module object. Marked unsafe since it does not provide any guarantees for safety of the ingested code.
• Portability limitation is that producer and consumer'sConfigs must have the same host triple, codegen flags, and Tunables. (Can we ignore CompilerStrategy?)

PR opened for feedback.

Wasmtime GitHub notifications bot (Jul 15 2020 at 19:16):

yurydelendik has marked PR #2020 as ready for review.

Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton submitted PR Review.

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)

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

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?

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.

Wasmtime GitHub notifications bot (Jul 15 2020 at 19:37):

alexcrichton created PR Review Comment:

s/tripple/triple/

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)

Wasmtime GitHub notifications bot (Jul 15 2020 at 19:56):

yurydelendik submitted PR Review.

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.

Wasmtime GitHub notifications bot (Jul 15 2020 at 19:59):

yurydelendik submitted PR Review.

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.

Wasmtime GitHub notifications bot (Jul 16 2020 at 10:57):

tschneidereit submitted PR Review.

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?

Wasmtime GitHub notifications bot (Jul 16 2020 at 16:04):

alexcrichton submitted PR Review.

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.

Wasmtime GitHub notifications bot (Jul 16 2020 at 16:04):

alexcrichton submitted PR Review.

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?

Wasmtime GitHub notifications bot (Jul 16 2020 at 16:22):

yurydelendik submitted PR Review.

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.

Wasmtime GitHub notifications bot (Jul 16 2020 at 16:23):

yurydelendik edited PR Review Comment.

Wasmtime GitHub notifications bot (Jul 16 2020 at 19:36):

alexcrichton submitted PR Review.

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?

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.

• Added compile_and_serialize API function that allows to save CompilationArtifacts object in binary format (+target/config fingerprint)
• Added unsafe Module::deserialize API function that allows to read the above data and create Module object. Marked unsafe since it does not provide any guarantees for safety of the ingested code.
• Portability limitation is that producer and consumer'sConfigs must have the same host triple, codegen flags, and Tunables. (Can we ignore CompilerStrategy?)

PR opened for feedback.

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.

• Added compile_and_serialize API function that allows to save CompilationArtifacts object in binary format (+target/config fingerprint)
• Added unsafe Module::deserialize API function that allows to read the above data and create Module object. Marked unsafe since it does not provide any guarantees for safety of the ingested code.
• Portability limitation is that producer and consumer'sConfigs must have the same host triple, codegen flags, and Tunables. (Can we ignore CompilerStrategy?)

PR opened for feedback.

Wasmtime GitHub notifications bot (Jul 16 2020 at 22:23):

yurydelendik submitted PR Review.

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.

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.

• Added compile_and_serialize API function that allows to save CompilationArtifacts object in binary format (+target/config fingerprint)
• Added unsafe Module::deserialize API function that allows to read the above data and create Module object. Marked unsafe since it does not provide any guarantees for safety of the ingested code.
• Portability limitation is that producer and consumer'sConfigs must have the same host triple, codegen flags, and Tunables. (Can we ignore CompilerStrategy?)

PR opened for feedback.

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.

• Added compile_and_serialize API function that allows to save CompilationArtifacts object in binary format (+target/config fingerprint)
• Added unsafe Module::deserialize API function that allows to read the above data and create Module object. Marked unsafe since it does not provide any guarantees for safety of the ingested code.
• Portability limitation is that producer and consumer'sConfigs must have the same host triple, codegen flags, and Tunables. (Can we ignore CompilerStrategy?)

PR opened for feedback.

Wasmtime GitHub notifications bot (Jul 21 2020 at 18:55):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Jul 21 2020 at 20:05):

yurydelendik merged PR #2020.

Last updated: Nov 22 2024 at 16:03 UTC