Enable gc feature by default#12805
Enable gc feature by default#12805tolumide-ng wants to merge 2 commits intobytecodealliance:mainfrom
Conversation
tolumide-ng
force-pushed
the
fix/wasmtime-c-api-gc-update
branch
from
0fb765b to
2569780
Compare
- Remove gc from default on c-api - Adds fallback implementation for when gc is not enabled
tolumide-ng
force-pushed
the
fix/wasmtime-c-api-gc-update
branch
from
2569780 to
ba53a43
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
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.
| #[cfg(feature = "gc")] | ||
| c.config.wasm_function_references(enable); | ||
|
|
||
| #[cfg(not(feature = "gc"))] | ||
| let _ = (c, enable); |
There was a problem hiding this comment.
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
#ifdefguard to see ifWASMTIME_FEATURE_GCis 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?
There was a problem hiding this comment.
This is an example file where when gc is disabled the entire module may wish to disappear vs having lots of #[cfg] internally.
| #[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, | ||
| ) | ||
| } |
There was a problem hiding this comment.
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_valbased on thegcfeature -- if it's enabled it's as-is, and if it's disabled the argument is justimpl AsContextMut(e.g. drop theRootScope) - For methods using
to_valuse the pattern#[cfg(feature = "gc")] let mut store = RootScope::new(&mut store);-- that way it shadows thestorelocal variable meaning thatto_valworks both with/without thegcfeature.
how's that sound?
Welcome back from your holiday @alexcrichton, and thank you for the review. |
2 participants
The
c-apicrate currently enables thegcfeature on itswasmtimedependency, even though it exposes agcfeature to control this behaviour. This prevents downstream users from disablinggc, sinceCargo.tomldoesn't allow overriding it. However, structs likeRootScopeare only available whengcis enabled. Removing it from default features, without other options, would break internal implementations.Changes
gcfeature via the wasmtime-c-api crate by default.gcfeature for thewasmtimedependency inc-api, thus enabling downstream users to specify this behaviour.Closes #12783