Stream: git-wasmtime

Topic: wasmtime / PR #7975 Wasmtime: Add a `gc` cargo feature

Wasmtime GitHub notifications bot (Feb 22 2024 at 00:04):

fitzgen opened PR #7975 from fitzgen:gc-cargo-feature to bytecodealliance:main:

This controls whether support for ExternRef and its associated deferred, reference-counting garbage collector is enabled at compile time or not. It will also be used for similarly for Wasmtime's full Wasm GC support as that gets added.

<!--
Please make sure you include the following information:

• If this work has been discussed elsewhere, please include a link to that
  conversation. If it was discussed in an issue, just mention "issue #...".

• Explain why this change is needed. If the details are in an issue already,
  this can be brief.

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

Wasmtime GitHub notifications bot (Feb 22 2024 at 00:04):

fitzgen requested wasmtime-core-reviewers for a review on PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 00:04):

fitzgen requested alexcrichton for a review on PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 00:05):

fitzgen commented on PR #7975:

I didn't add a smoke test build without the gc cargo feature to CI, but I can do that if we decide we can afford the CI time.

Wasmtime GitHub notifications bot (Feb 22 2024 at 01:03):

github-actions[bot] commented on PR #7975:

Subscribe to Label Action

cc @peterhuene

<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:config"

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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 22 2024 at 01:03):

github-actions[bot] commented on PR #7975:

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

• [ ] If you added a new Config method, you wrote extensive documentation for
  it.

  <details>

  Our documentation should be of the following form:

  ```text
  Short, simple summary sentence.

  More details. These details can be multiple paragraphs. There should be
  information about not just the method, but its parameters and results as
  well.

  Is this method fallible? If so, when can it return an error?

  Can this method panic? If so, when does it panic?

  Example

  Optional example here.
  ```

  </details>

• [ ] If you added a new Config method, or modified an existing one, you
  ensured that this configuration is exercised by the fuzz targets.

  <details>

  For example, if you expose a new strategy for allocating the next instance
  slot inside the pooling allocator, you should ensure that at least one of our
  fuzz targets exercises that new strategy.

  Often, all that is required of you is to ensure that there is a knob for this
  configuration option in [wasmtime_fuzzing::Config][fuzzing-config] (or one
  of its nested structs).

  Rarely, this may require authoring a new fuzz target to specifically test this
  configuration. See [our docs on fuzzing][fuzzing-docs] for more details.

  </details>

• [ ] If you are enabling a configuration option by default, make sure that it
  has been fuzzed for at least two weeks before turning it on by default.

[fuzzing-config]: https://github.com/bytecodealliance/wasmtime/blob/ca0e8d0a1d8cefc0496dba2f77a670571d8fdcab/crates/fuzzing/src/generators.rs#L182-L194
[fuzzing-docs]: https://docs.wasmtime.dev/contributing-fuzzing.html

---

<details>

To modify this label's message, edit the <code>.github/label-messager/wasmtime-config.md</code> file.

To add new label messages or remove existing label messages, edit the
<code>.github/label-messager.json</code> configuration file.

Learn more.

</details>

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton submitted PR review:

Nice!

My main comments below are about perhaps shifting around #[cfg] to try to reduce it. That's my main worried with these sorts of features is that it can become difficult in the limit to ensure all the #[cfg] annotations are aligned so I'm hoping we can try to reduce them as much as possible. More-or-less I think that ExternRef and probably VMExternRef should have the same API no matter the feature set (modulo constructors) and that way consumers don't have to #[cfg] which should help centralize this.

We'll also definitely want this on CI and I'd recommend putting it around here

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton submitted PR review:

Nice!

My main comments below are about perhaps shifting around #[cfg] to try to reduce it. That's my main worried with these sorts of features is that it can become difficult in the limit to ensure all the #[cfg] annotations are aligned so I'm hoping we can try to reduce them as much as possible. More-or-less I think that ExternRef and probably VMExternRef should have the same API no matter the feature set (modulo constructors) and that way consumers don't have to #[cfg] which should help centralize this.

We'll also definitely want this on CI and I'd recommend putting it around here

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton created PR review comment:

In the interest of reducing #[cfg] as much as possible (or at least centralizing it) could this and other helper methods be preserved here? That way in wasmtime-runtime itself it would have the match x {} to assert unreachability (or some method on VMExternRef itself)

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton created PR review comment:

I realize that this is what I recommended, having an uninhabited public type, but I'm second guessing myself now. This feels like it's pushing a lot of #[cfg] noise into the consumers of externref.

Could the methods on ExternRef, at least used by Wasmtime itself, be preserved? That way only ExternRef itself needs #[cfg] and consumers don't have to mind whether it's defined or not.

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton created PR review comment:

You can actually skip this documentation as rustdoc will auto-generate it with the right annotations. Elsewhere in the codebase we've got #[cfg_attr(docsrs, doc(cfg(feature = "async")))], so could all public methods gated behind the gc feature have a similar annotation?

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton created PR review comment:

In contrast with wasmtime::ExternRef this feels like the right balance of #[cfg] to me where we want to change runtime shapes which requires #[cfg] in this file. Not the best but also not overly awful.

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton created PR review comment:

For this file in particular I might recommend compiling this in both modes of with/without GC. Each method would have:

#[cfg(feature = "gc")]
{ /* current code */ }
#[cfg(not(feature = "gc"))]
{ match self.inner {} }

and only construction-related methods would be not provided

Wasmtime GitHub notifications bot (Feb 22 2024 at 15:48):

alexcrichton created PR review comment:

This'll need to be specifically inverted with GC support disabled since wasmparser has this on-by-default

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:52):

fitzgen requested wasmtime-default-reviewers for a review on PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:52):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:53):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:53):

fitzgen created PR review comment:

It requires the doc_cfg nightly feature for rustdoc to add that annotation, which docs.rs enables but local cargo doc invocations probably don't. Therefore, I think it is worth leaving the comment there.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:55):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:55):

fitzgen created PR review comment:

The problem with this specific case is that we store the VMExternRefActivationsTable directly in the store, so if it is uninhabited, then the whole Store becomes uninhabited.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:56):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 17:56):

alexcrichton created PR review comment:

True yeah, but that's why it's gated by docsrs so the docs.rs docs are the only ones that show this. I'd recommend using the #[cfg_attr] approach since that's what everything else does in the API docs right now, or otherwise we should remove all those and add manual docs there too

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:00):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:00):

fitzgen created PR review comment:

I actually think we want to keep whole implementations of ExternRef separate for GC vs not GC. Looking into the future, there are going to be a bunch more types that are going to be doing this same split, likely whole submodules of things, and I don't think it will be very nice to have all these cfgs for the whole submodule tree. I think it will be better to have (a) the GC-enabled submodule tree and (b) a single GC-disabled module that defines all the same things from (a) but with uninhabited types.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:01):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:01):

fitzgen created PR review comment:

We could make it an Option, but then we enable the potential for accidentally setting it to None when GC is enabled. I think I'd rather leave these handful of cfgs in comparison to that.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:02):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:03):

fitzgen commented on PR #7975:

Okay, I've updated this PR to stub out more VMExternRef things and methods when GC is disabled at compile time and removed a bunch of cfgs from the rest of the code base. Not all of them in store.rs, as mentioned inline above. I think this is ready for another round of review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:07):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:07):

fitzgen created PR review comment:

I guess we could have a GcOnly<T> type like

enum GcOnly<T> {
    #[cfg(feature = "gc")]
    inner: T,
}

impl<T> GcOnly<T> {
    pub fn with<U>(&self, f: impl FnMut(&T) -> U) -> Option<U> {
        #[cfg(feature = "gc")]
        return Some(f(&self.inner));
        #[cfg(not(feature = "gc"))]
        return None;
    }
}

But that seems like overkill maybe?

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:09):

fitzgen edited PR review comment.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:13):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:15):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:35):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:35):

alexcrichton created PR review comment:

I'd personally prefer to avoid GcOnly<T>, I feel like those sorts of wrappers haven't worked out well in the past or require otherwise just as much #[cfg] traffic. For VMExternRefActivationsTable I'd expect that'd become an empty struct rather than uninhabited because there are no references to track?

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:36):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:36):

alexcrichton created PR review comment:

Could the non-gc modules have methods that match on the uninhabited self then? Basically something to keep the #[cfg] rather than requiring it at all use-sites throughout the code base

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:36):

alexcrichton edited PR review comment.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen created PR review comment:

Ah yeah I could make that change, good idea.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen created PR review comment:

They do? The only ones left throughout the code base should be things that call constructors.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:45):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:45):

fitzgen created PR review comment:

(It just turns out we call constructors frequently, unfortunately)

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:55):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:55):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 18:55):

fitzgen created PR review comment:

Done!

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen requested cfallin for a review on PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen requested wasmtime-compiler-reviewers for a review on PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton created PR review comment:

Could this drop the pub(crate)?

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton created PR review comment:

Could this perhaps to _inner.assert_unreachable() or something like that? (where VMExternRef only has that method when the gc feature is disabled)

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton created PR review comment:

This is fine as-is but one trick I've done in the past is at the root of the crate:

#![cfg_attr(not(feature = "gc"), allow(irrefutable_let_patterns))]

or more often unused_imports or similar.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton created PR review comment:

more bits for #[cfg_attr(docsrs, ...)]

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen requested alexcrichton for a review on PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:22):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:23):

fitzgen has enabled auto merge for PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 19:24):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 21:44):

github-actions[bot] commented on PR #7975:

Subscribe to Label Action

cc @fitzgen, @saulecabrera

<details>
This issue or pull request has been labeled: "wasmtime:ref-types", "winch"

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

• fitzgen: wasmtime:ref-types
• saulecabrera: winch

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

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 22 2024 at 22:48):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 22 2024 at 22:48):

fitzgen has enabled auto merge for PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 01:55):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 01:56):

github-actions[bot] commented on PR #7975:

Subscribe to Label Action

cc @peterhuene

<details>
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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Feb 23 2024 at 01:59):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 01:59):

fitzgen has enabled auto merge for PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 15:48):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 16:53):

fitzgen updated PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 16:54):

fitzgen has enabled auto merge for PR #7975.

Wasmtime GitHub notifications bot (Feb 23 2024 at 17:40):

fitzgen merged PR #7975.

Last updated: Nov 22 2024 at 17:03 UTC