alexcrichton opened PR #8451 from alexcrichton:new-c-api-for-refs
to bytecodealliance:main
:
This commit redesigns how GC references work in the C API. previously
wasmtime_{any,extern}ref_t
were both opaque pointers in the C API represented as aBox
. Wasmtime did not, however, provide the ability to deallocate just theBox
part. This was intended to be handled with unrooting APIs but unrooting requires awasmtime_context_t
parameter, meaning that destructors of types in other languages don't have a suitable means of managing the memory around the
wasmtime_{any,extern}ref_t
which might lead to leaks.This PR takes an alternate approach for the representation of these types in the C API. Instead of being an opaque pointer they're now actual structures with definitions in the header file. These structures mirror the internals in Rust and Rust is tagged to ensure that changes are reflected in the C API as well. This is similar to how
wasmtime_func_t
matcheswasmtime::Func
. This enables embedders to not need to worry about memory management of these values outside of the manual rooting otherwise required.The hope is that this will reduce the likelihood of memory leaks and otherwise not make it any harder to manage references in the C API.
<!--
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.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
alexcrichton has marked PR #8451 as ready for review.
alexcrichton requested fitzgen for a review on PR #8451.
alexcrichton requested wasmtime-core-reviewers for a review on PR #8451.
alexcrichton updated PR #8451.
fitzgen submitted PR review.
fitzgen submitted PR review.
fitzgen created PR review comment:
A
ManuallyRooted<T>
already contains a store ID, is there a way we can avoid duplicating it here?
fitzgen created PR review comment:
Ah nevermind I see now that this is a union, not a struct, and that
store_id == 0
is used asNone
.
fitzgen created PR review comment:
But, could we just make this
Option<ManuallyDrop<ManuallyRooted<ExternRef>>>
instead of a union?
fitzgen created PR review comment:
Maybe these asserts should be for
ManuallyRooted
rather thanGcRootIndex
, just so that if we add a field to the former, but leave the latter as-is, we get build errors because that mismatch would introduce unsafety.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
That's a good point about
Option
, but one caveat is that embedders need to be able to make a null reference, test for one, and ideally without calling an API functions. That's why I opted for this representation where with#[repr(C)]
we put thestore_id
first everywhere and can test it for zero here (and it'sNonZeroU64
inStoreId
).So in that sense we could indeed use
Option
here, but it would rely on having a static assertion along the lines of:union T { a: u64, b: std::mem::ManuallyDrop<Option<ManuallyRooted<ExternRef>>>, } unsafe { assert!( T { b: std::mem::ManuallyDrop::new(None) } .a == 0 ); }
which, in testing, does in fact work. I'm sort of 50/50 though on whether this is worth it or "too clever by half"... What do you think, worth it to rely on that niche optimization?
alexcrichton updated PR #8451.
alexcrichton updated PR #8451.
fitzgen submitted PR review.
fitzgen created PR review comment:
I feel like this is worth doing, and keeping the union inside just the assertion instead of permeating the rest of the Rust code. I don't think this is too niche of an optimization because I was under the impression that it was "well known" that Rust would pack discriminants into non-zero and non-null things.
fitzgen submitted PR review.
fitzgen created PR review comment:
The other option here is to have a
impl ManuallyRooted<T> { #[doc(hidden)] pub fn into_raw_parts_for_c_api(&self) -> (u64, u32) { ... } }
method and a dual
from_raw_parts_for_c_api
method.
fitzgen created PR review comment:
That might be simpler and easier to maintain and ensure remains correct long term.
fitzgen submitted PR review.
alexcrichton updated PR #8451.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Good point about the
#[doc(hidden)]
bits, I like that route too so I've implemented that
alexcrichton updated PR #8451.
alexcrichton has enabled auto merge for PR #8451.
alexcrichton updated PR #8451.
alexcrichton has enabled auto merge for PR #8451.
alexcrichton updated PR #8451.
alexcrichton updated PR #8451.
alexcrichton has enabled auto merge for PR #8451.
alexcrichton updated PR #8451.
alexcrichton merged PR #8451.
Last updated: Dec 23 2024 at 12:05 UTC