Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Executor: Add create_runtime_from_artifact_bytes #14184

Merged
merged 3 commits into from
May 22, 2023

Conversation

mrcnski
Copy link

@mrcnski mrcnski commented May 21, 2023

PULL REQUEST

Overview

See title. Also export WasmtimeRuntime, returned by the create_runtime* functions.

I benchmarked the performance regression out of curiosity: bytecodealliance/wasmtime#6420. But since runtime creation is not at all a bottleneck, it makes sense to always use this new function for the reasons given in the related issue and PR.

Relevant Issues/PRs

Needed for paritytech/polkadot#7246 (comment)
Closes #13861

Also export `WasmtimeRuntime`, returned by the `create_runtime*` functions.
@mrcnski mrcnski added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”. labels May 21, 2023
@mrcnski mrcnski requested a review from bkchr May 21, 2023 16:33
@mrcnski mrcnski requested a review from koute as a code owner May 21, 2023 16:33
@mrcnski mrcnski self-assigned this May 21, 2023
Copy link
Contributor

@koute koute left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that AFAIK this can have performance implications on module instantiation, not just on module creation.

On Linux this most likely won't make a difference because when deserializing a precompiled module from a slice wasmtime will just create an internal memfd mapping for it anyway, but for macOS it cannot do so (since macOS doesn't have an equivalent of memfd) so it will prevent the copy-on-write instantiation codepath from kicking in.

@mrcnski
Copy link
Author

mrcnski commented May 22, 2023

@koute You are right, I missed that:

/// All of the CoW strategies (with `CopyOnWrite` suffix) are only supported when either:
///   a) we're running on Linux,
///   b) we're running on an Unix-like system and we're precompiling
///      our module beforehand.
/// * Linux - this feature is supported for all instances of [`Module`].
///   Modules backed by an existing mmap (such as those created by
///   [`Module::deserialize_file`]) will reuse that mmap to cow-initialize
///   memory. Other instance of [`Module`] may use the `memfd_create`
///   syscall to create an initialization image to `mmap`.
/// * Unix (not Linux) - this feature is only supported when loading modules
///   from a precompiled file via [`Module::deserialize_file`] where there
///   is a file descriptor to use to map data into the process. Note that
///   the module must have been compiled with this setting enabled as well.

I did some benchmarks here. Might make testing on Mac a little slower but we can live with it.

@mrcnski
Copy link
Author

mrcnski commented May 22, 2023

Can someone please take a look at the latest commit before I merge this in? I'm wondering if there's a technical reason we didn't already have a pooling_vanilla_precompiled benchmark. (i.e. that particular combination of options doesn't make sense for some reason.)

@bkchr
Copy link
Member

bkchr commented May 22, 2023

Can someone please take a look at the latest commit before I merge this in? I'm wondering if there's a technical reason we didn't already have a pooling_vanilla_precompiled benchmark. (i.e. that particular combination of options doesn't make sense for some reason.)

Probably just an oversight. From the Polkadot PVF validation side the instancing strategy isn't that important we only ever create one instance, not like for the runtime. However, the runtime doesn't use pre-compiled.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executor: Create runtime from artifact in-memory rather than on-disk?
3 participants