wasmtime / issue #5066 Refactor wasmtime::Func to "unspla... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / issue #5066 Refactor wasmtime::Func to "unspla...

Wasmtime GitHub notifications bot (Oct 17 2022 at 23:12):

pchickey labeled issue #5066:

Feature

PR #5065 got rid of a trait that "splatted" a type-level tuple of arguments into individual arguments to a closure. @alexcrichton and I feel this is a design choice which isn't worth its weight, and advocate for getting rid of the equivalent functionality for core wasm wasmtime::Funcs.

Benefit

The current Func::wrap signature is hard to read, and the set of functions Func::wrap{n}_async is just unsightly. This should bring us down to a much simpler Func::wrap and a single Func::wrap_async constructor.

Unfortunately, the benefits are tempered by this being a sorta annoying breaking change for downstream users. It won't break anyones use case, but all downstream users of Func::wrap will have to make some small syntactic adjustments to their code for it to build.

Implementation

You'll need to make the changes in wasmtime, then chase down the consequences in wiggle, the test suite, and any other places in the repository that invoke Func::wrap.

We haven't dug deep into the exact way this will be implemented, but it should end up looking like #5065 if you squint. If that approach doesn't look like it will work, lets discuss why here and figure out an alternative.

Wasmtime GitHub notifications bot (Oct 17 2022 at 23:12):

pchickey opened issue #5066:

Feature

PR #5065 got rid of a trait that "splatted" a type-level tuple of arguments into individual arguments to a closure. @alexcrichton and I feel this is a design choice which isn't worth its weight, and advocate for getting rid of the equivalent functionality for core wasm wasmtime::Funcs.

Benefit

The current Func::wrap signature is hard to read, and the set of functions Func::wrap{n}_async is just unsightly. This should bring us down to a much simpler Func::wrap and a single Func::wrap_async constructor.

Unfortunately, the benefits are tempered by this being a sorta annoying breaking change for downstream users. It won't break anyones use case, but all downstream users of Func::wrap will have to make some small syntactic adjustments to their code for it to build.

Implementation

You'll need to make the changes in wasmtime, then chase down the consequences in wiggle, the test suite, and any other places in the repository that invoke Func::wrap.

We haven't dug deep into the exact way this will be implemented, but it should end up looking like #5065 if you squint. If that approach doesn't look like it will work, lets discuss why here and figure out an alternative.

Wasmtime GitHub notifications bot (Nov 17 2022 at 11:56):

jwcesign commented on issue #5066:

Can I take this?

Wasmtime GitHub notifications bot (Nov 17 2022 at 11:57):

jwcesign edited a comment on issue #5066:

Can I take this?
/assign

Wasmtime GitHub notifications bot (Nov 17 2022 at 17:59):

pchickey commented on issue #5066:

Yes, please assign me as the reviewer for any PRs.

Wasmtime GitHub notifications bot (Nov 17 2022 at 17:59):

pchickey assigned issue #5066 (assigned to jwcesign):

Feature

PR #5065 got rid of a trait that "splatted" a type-level tuple of arguments into individual arguments to a closure. @alexcrichton and I feel this is a design choice which isn't worth its weight, and advocate for getting rid of the equivalent functionality for core wasm wasmtime::Funcs.

Benefit

The current Func::wrap signature is hard to read, and the set of functions Func::wrap{n}_async is just unsightly. This should bring us down to a much simpler Func::wrap and a single Func::wrap_async constructor.

Unfortunately, the benefits are tempered by this being a sorta annoying breaking change for downstream users. It won't break anyones use case, but all downstream users of Func::wrap will have to make some small syntactic adjustments to their code for it to build.

Implementation

You'll need to make the changes in wasmtime, then chase down the consequences in wiggle, the test suite, and any other places in the repository that invoke Func::wrap.

We haven't dug deep into the exact way this will be implemented, but it should end up looking like #5065 if you squint. If that approach doesn't look like it will work, lets discuss why here and figure out an alternative.

Wasmtime GitHub notifications bot (Apr 23 2023 at 17:16):

Weijun-H commented on issue #5066:

It appears that nobody is currently working on this task. Would it be possible for me to pick this up?

Wasmtime GitHub notifications bot (Apr 23 2023 at 18:02):

Weijun-H deleted a comment on issue #5066:

It appears that nobody is currently working on this task. Would it be possible for me to pick this up?

Wasmtime GitHub notifications bot (May 23 2024 at 20:19):

ssnover commented on issue #5066:

@pchickey Is this issue still valid or is the API of wasmtime intended to be more static at this point? I have something working in the wasmtime crate, but it causes a lot of breaking API changes that look like this:

Before:

linker.func_wrap(
        "spectest",
        "print_f64_f64",
        move |f1: f64, f2: f64| {
            if !suppress {
                println!("{}: f64", f1);
                println!("{}: f64", f2);
            }
        },
    )?;

After:

linker.func_wrap(
        "spectest",
        "print_f64_f64",
        move |_, (f1, f2): (f64, f64)| {
            if !suppress {
                println!("{}: f64", f1);
                println!("{}: f64", f2);
            }
        },
    )?;

Before I go through and modify examples, tests, and implementations of proc-macros like from_witx, I wanted to check that this refactor to the arguments is still desired.

Wasmtime GitHub notifications bot (May 23 2024 at 20:27):

ssnover edited a comment on issue #5066:

@pchickey Is this issue still valid or is the API of wasmtime intended to be more static at this point? I have something working in the wasmtime crate, but it causes a lot of breaking API changes that look like this:

Before:
linker.func_wrap(
    "spectest",
    "print_f64_f64",
    move |f1: f64, f2: f64| {
        if !suppress {
            println!("{}: f64", f1);
            println!("{}: f64", f2);
        }
    },
)?;
After:
linker.func_wrap(
    "spectest",
    "print_f64_f64",
    move |_, (f1, f2): (f64, f64)| {
        if !suppress {
            println!("{}: f64", f1);
            println!("{}: f64", f2);
        }
    },
)?;
Before I go through and modify examples, tests, and implementations of proc-macros like from_witx, I wanted to check that this refactor to the arguments is still desired.

Wasmtime GitHub notifications bot (May 24 2024 at 04:28):

pchickey commented on issue #5066:

Honestly, I’m glad you asked because I’m less convinced than I was 18 months ago when I filed the issue. Getting rid of all of the trait stuff that lets the faux-variadic functions work is a nice cleanup for wasmtime, but it seems like a needless breaking change for our users, who haven’t complained about it ever afaik, and would be rightfully annoyed if it changed with no benefit to them. I’m glad we changed how it worked in wasmtime::component before it was stable enough to use, but I now feel like changing one of the oldest parts of the wasmtime crates API ought to have a bigger payoff than this does.

@alexcrichton what do you think?

Wasmtime GitHub notifications bot (May 24 2024 at 16:07):

alexcrichton commented on issue #5066:

I think that's a good point yeah that it's probably not worth breaking lots of code today. That being said there might still be something to be done here perhaps. Depending on how all the trait impls and such work out I think this would still be a great internal refactoring to have. For example implementing a trait-per-tuple and have the "meat" be a simple F, P, R generics type parameters would avoid having tons of code in macros as-is today.

Ideally, if the traits work out in terms of coherence and overlapping, it'd be great to support both today's version of Func::wrap in addition to a "tupled approach", basically enabling both to work. I think it would be great to remove all the func_wrapN_async methods in favor of a single wrap_async. That'd break consumers calling wrapN_async but I think that's much rarer (outside of bindings generators) than the blocking versions.

Wasmtime GitHub notifications bot (May 24 2024 at 17:45):

ssnover commented on issue #5066:

Sounds good. I can back out some of the changes very easily in the public API. I'll go through some of the existing examples and tests and swap some of the calls to the new method so there is coverage on both. Should code generation in from_witx migrate to the new method or remain on the existing one?

For the new method on on Func do you all have any preferences? Is Func::wrap_tupled too on the nose?

Wasmtime GitHub notifications bot (May 24 2024 at 22:14):

alexcrichton commented on issue #5066:

I think it's reasonable to change from_witx yeah, but could the tuple-based impls also go through Func::wrap? That's got some weird trait bounds and everything is pretty carefully aligned there so I'm not sure if it'd work, but I'm not sure it's worth adding a new *_tupled method per se

Wasmtime GitHub notifications bot (May 24 2024 at 23:06):

ssnover commented on issue #5066:

Yeah, based on my tinkering that's totally possible; I think that means there won't be any changes required for sync APIs generated by from_witx, just async in order to do away with the *N_async methods.

Wasmtime GitHub notifications bot (May 25 2024 at 23:01):

alexcrichton commented on issue #5066:

Sounds great to me :+1:, thanks again for helping to push on this!

Wasmtime GitHub notifications bot (May 31 2024 at 14:54):

ssnover commented on issue #5066:

I've been doing more investigation here and I can't figure out a way to make the two styles of parameters work together nicely. I either get down a path that requires a lot of code duplication or I run into conflicting trait implementations in IntoFunc.

Wasmtime GitHub notifications bot (May 31 2024 at 16:10):

alexcrichton commented on issue #5066:

Want to open a PR with your work-in-progress or want to share a link here? Might be able to try to help sort this out and/or confirm it may not be possible

Wasmtime GitHub notifications bot (Jun 02 2024 at 01:36):

ssnover commented on issue #5066:

I actually managed to get something that seemed reasonable after some time away from the code, PR above ^

Wasmtime GitHub notifications bot (Jun 03 2024 at 19:57):

alexcrichton closed issue #5066:

Feature

PR #5065 got rid of a trait that "splatted" a type-level tuple of arguments into individual arguments to a closure. @alexcrichton and I feel this is a design choice which isn't worth its weight, and advocate for getting rid of the equivalent functionality for core wasm wasmtime::Funcs.

Benefit

The current Func::wrap signature is hard to read, and the set of functions Func::wrap{n}_async is just unsightly. This should bring us down to a much simpler Func::wrap and a single Func::wrap_async constructor.

Unfortunately, the benefits are tempered by this being a sorta annoying breaking change for downstream users. It won't break anyones use case, but all downstream users of Func::wrap will have to make some small syntactic adjustments to their code for it to build.

Implementation

You'll need to make the changes in wasmtime, then chase down the consequences in wiggle, the test suite, and any other places in the repository that invoke Func::wrap.

We haven't dug deep into the exact way this will be implemented, but it should end up looking like #5065 if you squint. If that approach doesn't look like it will work, lets discuss why here and figure out an alternative.

Wasmtime GitHub notifications bot (Jun 03 2024 at 19:57):

alexcrichton commented on issue #5066:

I believe that the work in https://github.com/bytecodealliance/wasmtime/pull/8732 effectively completes this, so I'm going to close it. Thanks again @ssnover!

Last updated: Nov 22 2024 at 16:03 UTC