Stream: git-wasmtime

Topic: wasmtime / PR #8451 c-api: Remove allocations from `wasmt...

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

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 a Box. Wasmtime did not, however, provide the ability to deallocate just the Box part. This was intended to be handled with unrooting APIs but unrooting requires a wasmtime_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 matches wasmtime::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:

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 (Apr 23 2024 at 22:06):

alexcrichton has marked PR #8451 as ready for review.

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

alexcrichton requested fitzgen for a review on PR #8451.

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

alexcrichton requested wasmtime-core-reviewers for a review on PR #8451.

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

alexcrichton updated PR #8451.

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

fitzgen submitted PR review.

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

fitzgen submitted PR review.

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

fitzgen created PR review comment:

A ManuallyRooted<T> already contains a store ID, is there a way we can avoid duplicating it here?

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

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 as None.

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

fitzgen created PR review comment:

But, could we just make this Option<ManuallyDrop<ManuallyRooted<ExternRef>>> instead of a union?

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

fitzgen created PR review comment:

Maybe these asserts should be for ManuallyRooted rather than GcRootIndex, 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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 14:47):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 14:47):

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 the store_id first everywhere and can test it for zero here (and it's NonZeroU64 in StoreId).

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?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 14:47):

alexcrichton updated PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 15:07):

alexcrichton updated PR #8451.

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

fitzgen submitted PR review.

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

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 15:24):

fitzgen submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 15:24):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 15:24):

fitzgen created PR review comment:

That might be simpler and easier to maintain and ensure remains correct long term.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 15:24):

fitzgen submitted PR review.

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

alexcrichton updated PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 18:46):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 18:46):

alexcrichton created PR review comment:

Good point about the #[doc(hidden)] bits, I like that route too so I've implemented that

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 18:46):

alexcrichton updated PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 18:46):

alexcrichton has enabled auto merge for PR #8451.

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

alexcrichton updated PR #8451.

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

alexcrichton has enabled auto merge for PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 20:05):

alexcrichton updated PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 20:05):

alexcrichton updated PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 20:05):

alexcrichton has enabled auto merge for PR #8451.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 24 2024 at 20:49):

alexcrichton updated PR #8451.

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

alexcrichton merged PR #8451.

Last updated: Nov 22 2024 at 16:03 UTC