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::Func
s.Benefit
The current
Func::wrap
signature is hard to read, and the set of functionsFunc::wrap{n}_async
is just unsightly. This should bring us down to a much simplerFunc::wrap
and a singleFunc::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 inwiggle
, the test suite, and any other places in the repository that invokeFunc::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.
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::Func
s.Benefit
The current
Func::wrap
signature is hard to read, and the set of functionsFunc::wrap{n}_async
is just unsightly. This should bring us down to a much simplerFunc::wrap
and a singleFunc::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 inwiggle
, the test suite, and any other places in the repository that invokeFunc::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.
jwcesign commented on issue #5066:
Can I take this?
jwcesign edited a comment on issue #5066:
Can I take this?
/assign
pchickey commented on issue #5066:
Yes, please assign me as the reviewer for any PRs.
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::Func
s.Benefit
The current
Func::wrap
signature is hard to read, and the set of functionsFunc::wrap{n}_async
is just unsightly. This should bring us down to a much simplerFunc::wrap
and a singleFunc::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 inwiggle
, the test suite, and any other places in the repository that invokeFunc::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.
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?
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?
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 thewasmtime
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.
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 thewasmtime
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.
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?
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 thefunc_wrapN_async
methods in favor of a singlewrap_async
. That'd break consumers callingwrapN_async
but I think that's much rarer (outside of bindings generators) than the blocking versions.
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? IsFunc::wrap_tupled
too on the nose?
alexcrichton commented on issue #5066:
I think it's reasonable to change
from_witx
yeah, but could the tuple-based impls also go throughFunc::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
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.
alexcrichton commented on issue #5066:
Sounds great to me :+1:, thanks again for helping to push on this!
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
.
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
ssnover commented on issue #5066:
I actually managed to get something that seemed reasonable after some time away from the code, PR above ^
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::Func
s.Benefit
The current
Func::wrap
signature is hard to read, and the set of functionsFunc::wrap{n}_async
is just unsightly. This should bring us down to a much simplerFunc::wrap
and a singleFunc::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 inwiggle
, the test suite, and any other places in the repository that invokeFunc::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.
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: Dec 23 2024 at 12:05 UTC