Stream: git-wasmtime

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


view this post on Zulip 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:

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
-->

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 00:04):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 00:04):

fitzgen requested alexcrichton for a review on PR #7975.

view this post on Zulip 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.

view this post on Zulip 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:

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

Learn more.
</details>

view this post on Zulip 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:

[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>

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip 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)

view this post on Zulip 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.

view this post on Zulip 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?

view this post on Zulip 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.

view this post on Zulip 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

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 17:52):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 17:52):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 17:53):

fitzgen submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 17:55):

fitzgen submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 17:56):

alexcrichton submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:00):

fitzgen submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:01):

fitzgen submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:02):

fitzgen updated PR #7975.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:07):

fitzgen submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:09):

fitzgen edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:13):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:15):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:35):

alexcrichton submitted PR review.

view this post on Zulip 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?

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:36):

alexcrichton submitted PR review.

view this post on Zulip 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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:36):

alexcrichton edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen created PR review comment:

Ah yeah I could make that change, good idea.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:44):

fitzgen submitted PR review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:45):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:45):

fitzgen created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:55):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:55):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 18:55):

fitzgen created PR review comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen requested cfallin for a review on PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton created PR review comment:

Could this drop the pub(crate)?

view this post on Zulip 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)

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

alexcrichton created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:16):

fitzgen requested alexcrichton for a review on PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:22):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:23):

fitzgen has enabled auto merge for PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 19:24):

fitzgen updated PR #7975.

view this post on Zulip 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:

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 22:48):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 22 2024 at 22:48):

fitzgen has enabled auto merge for PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 01:55):

fitzgen updated PR #7975.

view this post on Zulip 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:

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 01:59):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 01:59):

fitzgen has enabled auto merge for PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 15:48):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 16:53):

fitzgen updated PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 16:54):

fitzgen has enabled auto merge for PR #7975.

view this post on Zulip Wasmtime GitHub notifications bot (Feb 23 2024 at 17:40):

fitzgen merged PR #7975.


Last updated: Dec 23 2024 at 13:07 UTC