Home Original page

Enable gc feature by default by tolumide-ng · Pull Request #12805 · bytecodealliance/wasmtime

@tolumide-ng

The c-api crate currently enables the gc feature on its wasmtime dependency, even though it exposes a gc feature to control this behaviour. This prevents downstream users from disabling gc, since Cargo.toml doesn't allow overriding it. However, structs like RootScope are only available when gc is enabled. Removing it from default features, without other options, would break internal implementations.

Changes

  1. Enable the gc feature via the wasmtime-c-api crate by default.
  2. Removes the explicitly set gc feature for the wasmtime dependency in c-api, thus enabling downstream users to specify this behaviour.

Closes #12783

@tolumide-ng

alexcrichton

@tolumide-ng 

- Remove gc from default on c-api
- Adds fallback implementation for when gc is not enabled

alexcrichton

Choose a reason for hiding this comment

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

Returning now from vacation, thanks for your patience @tolumide-ng. Also I'll apologize that the scope of this issue is broadening to be a bit larger than I had originally anticipated. I thought there would be some minor edits required to get the c-api building without GC, but there's a number of API design decisions to make here which affect how exactly this works. I'm happy to help guide through these, but if you feel this is getting too broad in scope that's also totally fine.

Comment on lines +106 to +110

#[cfg(feature = "gc")]
c.config.wasm_function_references(enable);

#[cfg(not(feature = "gc"))]
let _ = (c, enable);

Choose a reason for hiding this comment

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

We generally prefer to avoid having conditionally-noop functions like this, so this'll want some different treatment to handle the GC feature being disabled. The general shape of things is:

  • Rust-defined functions are entirely #[cfg]'d away, aka there's a #[cfg] on the method itself (or the containing module if applicable)
  • The C header needs a #ifdef guard to see if WASMTIME_FEATURE_GC is enabled.

That way when the gc feature is disabled the functions "disappear" as opposed to being present but not actually doing anything (which can be confusing). Would you be up for refactoring in that style?

Choose a reason for hiding this comment

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

This is an example file where when gc is disabled the entire module may wish to disappear vs having lots of #[cfg] internally.

Comment on lines +234 to +255

#[cfg(feature = "gc")]
{
let mut scope = RootScope::new(&mut store);
handle_result(
val.to_val(&mut scope)
.ref_()
.ok_or_else(|| format_err!("wasmtime_table_grow value is not a reference"))
.and_then(|val| table.grow(scope, delta, val)),
|prev| *prev_size = prev,
)
}

#[cfg(not(feature = "gc"))]
{
handle_result(
val.to_val_unscoped(&mut store)
.ref_()
.ok_or_else(|| format_err!("wasmtime_table_grow value is not a reference"))
.and_then(|val| table.grow(&mut store, delta, val)),
|prev| *prev_size = prev,
)
}

Choose a reason for hiding this comment

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

I feel it would be best to avoid the duplication here (and in a number of locations). Handling this will be a bit tricky though due to to_val wanting a RootScope. Thinking a bit about this, I think a way to thread this needle would be:

  • Conditionally define to_val based on the gc feature -- if it's enabled it's as-is, and if it's disabled the argument is just impl AsContextMut (e.g. drop the RootScope)
  • For methods using to_val use the pattern #[cfg(feature = "gc")] let mut store = RootScope::new(&mut store); -- that way it shadows the store local variable meaning that to_val works both with/without the gc feature.

how's that sound?

@tolumide-ng

Returning now from vacation, thanks for your patience @tolumide-ng. Also I'll apologize that the scope of this issue is broadening to be a bit larger than I had originally anticipated. I thought there would be some minor edits required to get the c-api building without GC, but there's a number of API design decisions to make here which affect how exactly this works. I'm happy to help guide through these, but if you feel this is getting too broad in scope that's also totally fine.

Welcome back from your holiday @alexcrichton, and thank you for the review.
I'm happy to continue with the expanded scope. It looks like an informative introduction to the codebase.
I'd definitely appreciate your guidance as questions come up.