Skip to content

Conversation

@zshipko
Copy link
Contributor

@zshipko zshipko commented Feb 14, 2024

  • Allows exports with any number of arguments and either 0 or 1 return value
    • These can be any native Wasm type: I32, I64, F32, F64
    • __arg_start is called to push a new call onto the argument stack (a global Vec<Vec<JSValue>>), then for each argument an arg function like __arg_i32 or __arg_f32 gets called, this pushes the value to the end of the vector at the the top of the stack, then when a function is called it pops the top Vec<JSValue> off of the stack to use as its arguments.
  • Allows imports with up to 5 I64 arguments and either 0 or 1 I64 return value
    • Since each of these signatures need to be implemented statically (__invokeHostFunc_1_1, __invokeHostFunc_2_1, ...) , there are too many combinations to allow for any type to be used
    • Adds a new prelude function: Host.invokeFunc0 to call a host function that takes 0 arguments
    • Host.invokeFunc and Host.invokeFunc0 determine the number of arguments passed and select the proper __invokeHostFunc_$nparams_$nresults function to call
  • Uses https://github.com/dylibso/wagen for code generation
  • Switches to use wasm-opt instead of binaryen crate
    • We already require a binaryen installation for wasm-merge, it seems like a lot for this to include its own copy too
    • Also gets rid of that warning that was being displayed
  • Backward compatible: none of the examples require adjusting, I think this change is mostly backward compatible.

@zshipko zshipko requested review from bhelx and nilslice February 21, 2024 21:06
@nilslice
Copy link
Member

Awesome work. I've tested this using a Go plugin to call a JS export as a host function and it works great.

I will do more testing with it tonight.. curious if you have any specific paths that you'd like me to focus on?

@zshipko
Copy link
Contributor Author

zshipko commented Feb 22, 2024

Thanks for taking a look!

curious if you have any specific paths that you'd like me to focus on?

I think just testing it out is the most helpful

One future improvement worth calling out is the ability to use types like string or object in the d.ts file, which would automatically get converted from an I64 parameter/result. I have started working on that, but it complicates things a little bit so it seems better to get these changes in first.

Copy link
Member

@nilslice nilslice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, ran through some more tests and re-ran the previous ones since the latest commits & everything works great!

Unless @bhelx wants to do a quick run though, this LGTM,

@nilslice
Copy link
Member

One future improvement worth calling out is the ability to use types like string or object in the d.ts file, which would automatically get converted from an I64 parameter/result. I have started working on that, but it complicates things a little bit so it seems better to get these changes in first.

This sounds nice!!

@zshipko zshipko merged commit 49ce86b into main Feb 26, 2024
@zshipko zshipko deleted the more-types branch February 26, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants