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 and simplify component trampolines #6751

Merged

Conversation

alexcrichton
Copy link
Member

Note that this is based on #6691

In poking at #6696 I felt that the amount of boilerplate for defining
new kinds of trampolines in the component model was getting a bit
excessive. There was already 6 different types and I was adding two more
and I had to touch just a few too many places to get this done. In the
end I decided to refactor how trampolines are handled in the component
model to make it much easier to add new kinds of trampolines.

To that end the type-specific counts/lists/etc are all gone now in favor
of a single concept of a trampoline. This means components now track
trampolines in-bulk rather than individually by type. For example
compiling trampolines is now a loop over "compile this list of
trampolines" where previously it was N loops for the N types of
trampolines. The Trampoline definition is where the enum and dispatch
happens where that contains all possible trampolines that a component
could require.

This ended up being a large refactor to the Cranelift component
integration, but there is not intended to be any functional change from
this refactoring. Additionally all trampolines are now removed from the
global initializers list since there's nothing preventing them from
being initialized earlier on during the instantiation process.

Overall this should drastically reduce the number of locations that need
to have trampoline-specific knowledge to translation, the dataflow
graph, and compilation. Nearly everything else can operate over
everything in bulk and forward between these systems.

@github-actions github-actions bot added fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself labels Jul 20, 2023
@github-actions
Copy link

Subscribe to Label Action

cc @fitzgen, @peterhuene

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

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

  • fitzgen: fuzzing
  • peterhuene: wasmtime:api

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

Learn more.

In poking at bytecodealliance#6696 I felt that the amount of boilerplate for defining
new kinds of trampolines in the component model was getting a bit
excessive. There was already 6 different types and I was adding two more
and I had to touch just a few too many places to get this done. In the
end I decided to refactor how trampolines are handled in the component
model to make it much easier to add new kinds of trampolines.

To that end the type-specific counts/lists/etc are all gone now in favor
of a single concept of a trampoline. This means components now track
trampolines in-bulk rather than individually by type. For example
compiling trampolines is now a loop over "compile this list of
trampolines" where previously it was N loops for the N types of
trampolines. The `Trampoline` definition is where the enum and dispatch
happens where that contains all possible trampolines that a component
could require.

This ended up being a large refactor to the Cranelift component
integration, but there is not intended to be any functional change from
this refactoring. Additionally all trampolines are now removed from the
global initializers list since there's nothing preventing them from
being initialized earlier on during the instantiation process.

Overall this should drastically reduce the number of locations that need
to have trampoline-specific knowledge to translation, the dataflow
graph, and compilation. Nearly everything else can operate over
everything in bulk and forward between these systems.
@alexcrichton alexcrichton force-pushed the component-trampolines branch from 667004b to 0df191a Compare July 24, 2023 15:07
@alexcrichton alexcrichton requested a review from fitzgen July 24, 2023 15:07
@alexcrichton alexcrichton marked this pull request as ready for review July 24, 2023 15:07
@alexcrichton alexcrichton requested a review from a team as a code owner July 24, 2023 15:07
@alexcrichton
Copy link
Member Author

Ok this should be good to go now that #6691 has landed

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.

Nice!

@fitzgen fitzgen added this pull request to the merge queue Jul 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Jul 24, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Jul 24, 2023
@alexcrichton
Copy link
Member Author

cc @pchickey on that last failure, it may be a spurious failure?

test preview2::pipe::test::sink_write_stream ... FAILED

failures:

---- preview2::pipe::test::sink_write_stream stdout ----
thread 'preview2::pipe::test::sink_write_stream' panicked at 'assertion failed: `(left == right)`
  left: `1024`,
 right: `0`', crates/wasi/src/preview2/pipe.rs:671:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/std/src/panicking.rs:593:5
   1: core::panicking::panic_fmt
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/panicking.rs:67:14
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
   4: wasmtime_wasi::preview2::pipe::test::sink_write_stream::{{closure}}
   5: tokio::runtime::context::runtime::enter_runtime
   6: wasmtime_wasi::preview2::pipe::test::sink_write_stream
   7: core::ops::function::FnOnce::call_once
   8: core::ops::function::FnOnce::call_once
             at /rustc/8ede3aae28fe6e4d52b38157d7bfe0d3bceef225/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    preview2::pipe::test::sink_write_stream

test result: FAILED. 13 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.59s

error: test failed, to rerun pass `-p wasmtime-wasi --lib`

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jul 24, 2023
@alexcrichton alexcrichton added this pull request to the merge queue Jul 24, 2023
Merged via the queue into bytecodealliance:main with commit f4eeb02 Jul 24, 2023
@alexcrichton alexcrichton deleted the component-trampolines branch July 24, 2023 22:56
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request Jul 25, 2023
This fills out support in FACT in Wasmtime to support
component-to-component calls that use resources. This ended up being
relatively simple as it's "just" a matter of moving resources between
tables which at this time bottoms out in calls to the host. These new
trampolines are are relatively easy to add after bytecodealliance#6751 which helps keep
this change contained.

Closes bytecodealliance#6696
github-merge-queue bot pushed a commit that referenced this pull request Jul 25, 2023
* Implement component-to-component calls with resources

This fills out support in FACT in Wasmtime to support
component-to-component calls that use resources. This ended up being
relatively simple as it's "just" a matter of moving resources between
tables which at this time bottoms out in calls to the host. These new
trampolines are are relatively easy to add after #6751 which helps keep
this change contained.

Closes #6696

* Review comments
geekbeast pushed a commit to geekbeast/wasmtime that referenced this pull request Aug 1, 2023
* main: (47 commits)
  Add core dump support to the runtime (bytecodealliance#6513)
  Resource table tracks child relationships (bytecodealliance#6779)
  Wasmtime: Move `OnDemandInstanceAllocator` to its own module (bytecodealliance#6790)
  wasi: Test the stdio streams implementation (bytecodealliance#6764)
  Don't generate same-named imports in fact modules (bytecodealliance#6783)
  Wasmtime: Add support for Wasm tail calls (bytecodealliance#6774)
  Cranelift: Fix `ABIMachineSpec::gen_add_imm` for riscv64 (bytecodealliance#6780)
  Update the wasm-tools family of crates, disallow empty component types (bytecodealliance#6777)
  Fix broken link to WASI API documentation (bytecodealliance#6775)
  A bunch of cleanups for cranelift-codegen-meta (bytecodealliance#6772)
  Implement component-to-component calls with resources (bytecodealliance#6769)
  Ignore async_stack_size if async_support is disabled (bytecodealliance#6771)
  A bunch of minor cleanups (bytecodealliance#6767)
  Fix flaky tests in preview2 streams (bytecodealliance#6763)
  Refactor and simplify component trampolines (bytecodealliance#6751)
  Cranelift: Implement tail calls on riscv64 (bytecodealliance#6749)
  WASI Preview 2: rewrite streams and pollable implementation (bytecodealliance#6556)
  cranelift-wasm: Add support for translating Wasm tail calls to CLIF (bytecodealliance#6760)
  Cranelift: Get tail calls working on aarch64 (bytecodealliance#6723)
  Implement component model resources in Wasmtime (bytecodealliance#6691)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fuzzing Issues related to our fuzzing infrastructure wasmtime:api Related to the API of the `wasmtime` crate itself
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants