Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor module instantiation in the runtime. #2454

Closed
wants to merge 24 commits into from

Conversation

peterhuene
Copy link
Member

@peterhuene peterhuene commented Nov 26, 2020

This PR refactors module instantiation in the runtime to allow for
different instance allocation strategy implementations.

It adds an InstanceAllocator trait with the current implementation put behind
the OnDemandInstanceAllocator struct.

The Wasmtime API has been updated to allow a Config to have an instance
allocation strategy set which will determine how instances get allocated.

This change is in preparation for an alternative pooling instance allocator
that can reserve all needed host process address space in advance.

This PR also makes changes to the wasmtime_environ crate to represent
compiled modules in a way that reduces copying at instantiation time.

See bytecodealliance/rfcs#5 about these proposed changes.

This PR depends on #2434.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm wasmtime:api Related to the API of the `wasmtime` crate itself labels Nov 26, 2020
@github-actions
Copy link

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:wasm", "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@peterhuene
Copy link
Member Author

This PR also tries to reduce allocations and copying:

  • wasmtime_environ::Module now stores passive element and data segments differently so that instances can track dropped segments using an EntitySet rather than cloning the previous HashMaps and removing an entry when dropped.

  • The module field of CompilationArtifacts have been made Arc so that CompiledModule doesn't need to clone the underlying module during compilation.

  • Similarly, the data_initializers field of CompilationArtifacts has been made Arc so that, in the future, a pooling instance allocator can take a reference on the data initializers for on-demand initialization of linear memory pages. This change also prevents having to allocate a new Vec of data initializers to complete initialization for every instantiation.

Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning up the passive data/element segments stuff!

Overall this looks great and is a good step forward for wasmtime. I have a few questions inline below, however.

crates/runtime/src/instance.rs Outdated Show resolved Hide resolved
crates/runtime/src/instance/allocator/default.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/config.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/instance.rs Outdated Show resolved Hide resolved
crates/wasmtime/src/instance/allocator.rs Outdated Show resolved Hide resolved
@peterhuene peterhuene force-pushed the instance-pooling branch 3 times, most recently from dc78ba1 to 2ede8e4 Compare December 1, 2020 20:55
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looks good to me! I didn't review allocator.rs too too closely since I figured it's all just code movement and refactoring of previous functionality.

crates/runtime/src/instance.rs Outdated Show resolved Hide resolved
@peterhuene peterhuene marked this pull request as ready for review December 3, 2020 21:05
@peterhuene peterhuene force-pushed the instance-pooling branch 4 times, most recently from 4a02a11 to 17ef987 Compare December 9, 2020 18:32
@peterhuene peterhuene force-pushed the instance-pooling branch 5 times, most recently from 489e00a to a2813b1 Compare December 16, 2020 21:27
@peterhuene peterhuene force-pushed the instance-pooling branch 4 times, most recently from af4b3d4 to bb33260 Compare January 29, 2021 19:52
@peterhuene peterhuene force-pushed the instance-pooling branch 3 times, most recently from 0f7431c to c08be37 Compare February 4, 2021 01:21
alexcrichton and others added 18 commits February 4, 2021 13:15
This commit implements APIs on `Store` to periodically yield execution
of futures through the consumption of fuel. When fuel runs out a
future's execution is yielded back to the caller, and then upon
resumption fuel is re-injected. The goal of this is to allow cooperative
multi-tasking with futures.
Turns out this is another caller-saved register!
Take a leaf out of aarch64's playbook and don't have extra memory to
load/store these arguments, instead leverage how `wasmtime_fiber_switch`
already loads a bunch of data into registers which we can then
immediately start using on a fiber's start without any extra memory
accesses.
With fuel it's probably best to not provide any way to inject infinite
fuel.
This commit refactors module instantiation in the runtime to allow for
different instance allocation strategy implementations.

It adds an `InstanceAllocator` trait with the current implementation put behind
the `OnDemandInstanceAllocator` struct.

The Wasmtime API has been updated to allow a `Config` to have an instance
allocation strategy set which will determine how instances get allocated.

This change is in preparation for an alternative *pooling* instance allocator
that can reserve all needed host process address space in advance.

This commit also makes changes to the `wasmtime_environ` crate to represent
compiled modules in a way that reduces copying at instantiation time.
This commit introduces two new methods on `InstanceAllocator`:

* `validate_module` - this method is used to validate a module after
  translation but before compilation. It will be used for the upcoming pooling
  allocator to ensure a module being compiled adheres to the limits of the
  allocator.

* `adjust_tunables` - this method is used to adjust the `Tunables` given the
  JIT compiler.  The pooling allocator will use this to force all memories to
  be static during compilation.
This commit refactors `Table` in the runtime such that it can be created from a
pointer to existing table data.

The current `Vec` backing of the `Table` is considered to be "dynamic" storage.

This will be used for the upcoming pooling allocator where table memory is
managed externally to the instance.

The `table.copy` implementation was improved to use slice primitives for doing
the copying.

Fixes bytecodealliance#983.
This commit changes how memories and tables are stored in `Instance`.

Previously, the memories and tables were stored as a `BoxedSlice`. Storing it
this way requires an allocation to change the length of the memories and
tables, which is desirable for a pooling instance allocator that is reusing an
`Instance` structure for a new instantiation.

By storing it instead as `PrimaryMap`, the memories and tables can be resized
without any allocations (the capacity of these maps will always be the
configured limits of the pooling allocator).
This commit changes `Instance` such that memories can be stored statically,
with just a base pointer, size, maximum, and a callback to make memory
accessible.

Previously the memories were being stored as boxed trait objects, which would
require the pooling allocator to do some unpleasant things to avoid
allocations.

With this change, the pooling allocator can simply define a memory for the
instance without using a trait object.
Handles created with `create_handle` need to be deallocated with the default
(on-demand) instance allocator.

This commit changes Store such that handles can be added with a flag that is
used to force deallocation via the default instance allocator when the Store is
dropped.
With the change to artificially limit unbounded memories based on Tunables,
it's possible to hit the assert where the minimum might exceed the static
memory bound.

This commit removes the assert in favor of a check to see if the minimum also
fits within the static memory bound. It also corrects the maximum bounding to
ensure the minimum between the memory's maximum and the configured maximum is
used.

If it does not fit, the memory will be treated as dynamic.  In the case of the
pooling instance allocator, the bounds will be checked again during translation
and an appropriate error will be returned as dynamic memories are not supported
for that allocator.
@peterhuene peterhuene force-pushed the instance-pooling branch 5 times, most recently from 5e34198 to 697f4c0 Compare February 5, 2021 21:37
This commit implements allocating fiber stacks in an instance allocator.

The on-demand instance allocator doesn't support custom stacks, so the
implementation will use the allocation from `wasmtime-fiber` for the fiber
stacks.

In the future, the pooling instance allocator will return custom stacks to use
on Linux and macOS.

On Windows, the native fiber implementation will always be used.
@github-actions github-actions bot added the wasmtime:c-api Issues pertaining to the C API. label Feb 5, 2021
@github-actions
Copy link

github-actions bot commented Feb 5, 2021

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:c-api"

Thus the following users have been cc'd because of the following labels:

  • peterhuene: wasmtime:c-api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

@peterhuene
Copy link
Member Author

Closing this PR in favor of #2518.

@peterhuene peterhuene closed this Feb 11, 2021
@peterhuene peterhuene deleted the instance-pooling branch February 12, 2021 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:wasm cranelift Issues related to the Cranelift code generator wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:c-api Issues pertaining to the C API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants