Stream: git-wasmtime

Topic: wasmtime / PR #3266 Add a `Module::deserialize_file` method

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 15:56):

alexcrichton requested peterhuene for a review on PR #3266.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 15:56):

alexcrichton opened PR #3266 from deserialize-file to main:

This commit adds a new method to the wasmtime::Module type,
deserialize_file. This is intended to be the same as the deserialize
method except for the serialized module is present as an on-disk file.
This enables Wasmtime to internally use mmap to avoid copying bytes
around and generally makes loading a module much faster.

A C API is added in this commit as well for various bindings to use this
accelerated path now as well. Another option perhaps for a Rust-based
API is to have an API taking a File itself to allow for a custom file
descriptor in one way or another, but for now that's left for a possible
future refactoring if we find a use case.

cc #3230

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 18:44):

alexcrichton updated PR #3266 from deserialize-file to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

Same comment as above re: path to file.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

Can we include the path to the file in the context here?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

Same comment as above re: path to file.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

We'll definitely want to use OpenOptions::share_mode (with FILE_SHARE_READ probably) to open the file on Windows to prevent write access to the file so another process can't write to it while we have it mapped.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

Nit: preference for with_context here given the format!?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

I think we might need FILE_MAP_COPY here so that if we handle any relocations in the text section, the pages become private (and backed with the page file) rather than modifying the share pages which ultimately get flushed to the backed file (the precompiled module in this case).

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

Nit: preference for with_context here given the format!?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

I think we probably want this to be PAGE_EXECUTE_READ with a FILE_MAP_COPY below.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

I'm actually really surprised this would work on Windows when the mapped file was opened FILE_GENERIC_READ as changing the protection level of the mapped pages would bypass file system access rights.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:30):

peterhuene created PR review comment:

Nit: preference for with_context here given the format!?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:39):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:39):

peterhuene created PR review comment:

Doh:

When copy-on-write access is specified, the system and process commit charge taken is for the entire view
because the calling process can potentially write to every page in the view, making all pages private.

We're going to need separate mapped views for the file, one specifically for the text section that's mapped FILE_MAP_READ, FILE_MAP_COPY, and FILE_MAP_EXECUTE.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:40):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 19:51):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 20:28):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 20:55):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 20:55):

peterhuene created PR review comment:

Alternatively, I think we may be able to keep the single view, but change the protection level of the text section's pages to PAGE_EXECUTE_WRITECOPY during relocation, provided the mapping was created with PAGE_EXECUTE_READ?

I'd have to experiment a bit.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 20:56):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 30 2021 at 21:00):

peterhuene edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 13:52):

alexcrichton updated PR #3266 from deserialize-file to main.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 13:53):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 13:53):

alexcrichton created PR review comment:

Oh this is already guarded by the if so this only gets executed in the error path anyway (and the closure would always be executed in this case)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 13:53):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 13:53):

alexcrichton created PR review comment:

I opted to add this context a layer above to avoid having to include it in all the error messages here (if you're ok with that since at least for now it's only called in one location)

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 17:57):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 17:57):

peterhuene created PR review comment:

Excellent point. My brain was just over zealous in seeing work done in a context call.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 18:00):

peterhuene submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 31 2021 at 18:05):

alexcrichton merged PR #3266.

Last updated: Nov 22 2024 at 17:03 UTC