Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add ability to dynamically link wasm modules #596

Merged
merged 16 commits into from
Jul 5, 2024
Merged

add ability to dynamically link wasm modules #596

merged 16 commits into from
Jul 5, 2024

Conversation

munjalpatel
Copy link
Contributor

@munjalpatel munjalpatel commented Jul 3, 2024

Fixes #437

Link un-compiled wasm module:

calculator_wasm = File.read!(TestHelper.wasm_link_test_file_path())
utils_wasm = File.read!(TestHelper.wasm_test_file_path())

links = %{utils: %{bytes: utils_wasm}}

{:ok, pid} = Wasmex.start_link(%{bytes: calculator_wasm, links: links})
Wasmex.call_function(pid, "sum_range", [1, 5])
# {:ok, [15]}

Link compiled wasm module:

calculator_wasm = File.read!(TestHelper.wasm_link_test_file_path())
utils_wasm = File.read!(TestHelper.wasm_test_file_path())

{:ok, store} = Wasmex.Store.new()
{:ok, utils_module} = Wasmex.Module.compile(store, utils_wasm)

links = %{utils: %{module: utils_module}}

{:ok, pid} = Wasmex.start_link(%{bytes: calculator_wasm, links: links, store: store})
Wasmex.call_function(pid, "sum_range", [1, 5])
# {:ok, [15]}

Note:
This PR doesn't solve dynamic-linking dependency chain. Meaning, linking Module A to Module B and then linking Module B to the Final Module. That's a much bigger undertaking.

A potential solution for now is to link both Module A and Module B to the Final Module. This should work in most cases as long as there are no module name collisions.

@munjalpatel
Copy link
Contributor Author

@tessi would appreciate your review!

@munjalpatel munjalpatel changed the title add wasm-link test modules and setup add ability to dynamically link wasm modules Jul 3, 2024
test/test_helper.exs Outdated Show resolved Hide resolved
lib/wasmex.ex Outdated Show resolved Hide resolved
lib/wasmex.ex Outdated Show resolved Hide resolved
@tessi
Copy link
Owner

tessi commented Jul 3, 2024

hey @munjalpatel lot's of 💛 for taking this on! The code looks real solid and easy enough to understand, love it!

I understand that dynamic linking is a problem we might want to solve later - good call. However, we might want to hint at that in the docs.

I love that you started implementing test infrastructure, but if I'm not mistaken the actual test for linking modules isn't in yet.

To make this an easy merge, I'd appreciate if you could add this new (great!) feature to the CHANGELOG.md file - if not I can easily do it myself on the next release too. no worries :)

@tessi tessi mentioned this pull request Jul 3, 2024
@munjalpatel
Copy link
Contributor Author

hey @munjalpatel lot's of 💛 for taking this on! The code looks real solid and easy enough to understand, love it!

I understand that dynamic linking is a problem we might want to solve later - good call. However, we might want to hint at that in the docs.

I love that you started implementing test infrastructure, but if I'm not mistaken the actual test for linking modules isn't in yet.

To make this an easy merge, I'd appreciate if you could add this new (great!) feature to the CHANGELOG.md file - if not I can easily do it myself on the next release too. no worries :)

Yes, tests are missing. I wanted to get an early review before I polish it up.
Great call-outs and suggestions. I'll go over them :)

@munjalpatel
Copy link
Contributor Author

@tessi I think the PR is ready for another review!

lib/wasmex.ex Outdated Show resolved Hide resolved
@tessi
Copy link
Owner

tessi commented Jul 4, 2024

somehow on older OTP versions one of the linking tests fails:

1) test linking multiple modules to satisfy dependencies (WasmLinkTest)
Error:      test/wasm_link_test.exs:29
     ** (EXIT from #PID<0.505.0>) bad return value: {:error, "Could not instantiate linked module: unknown import: `utils::sum` has not been defined"}

specifically on:

  • Elixir Compatibility / OTP 25.2 / Elixir 1.15.6
  • Elixir Compatibility / OTP 25.2 / Elixir 1.16.1
  • Elixir Compatibility / OTP 25.2 / Elixir 1.17.0

It might be an OTP 25 problem (although I fail to see how) or a flaky test (although suspiciously not failing on newer OTP versions). Do you have an idea what's going on here?

@tessi
Copy link
Owner

tessi commented Jul 4, 2024

Just had another look and your work looks great. Love the introduced API and how the link-map has different keys (bytes, module, module_resource) depending on how far the map was processed.

It still gives us the option to maybe introduce an "easier" api later (linking from module-name to module-bytes without a map) if this feature gets lots of use and there is demand. but no need to optimise for that future now :)

only got a few 💅 polishing suggestions and the only real blocker is CI complaining.

munjalpatel and others added 2 commits July 4, 2024 02:02
Co-authored-by: Philipp Tessenow <philipp@tessenow.org>
Co-authored-by: Philipp Tessenow <philipp@tessenow.org>
@munjalpatel
Copy link
Contributor Author

somehow on older OTP versions one of the linking tests fails:

1) test linking multiple modules to satisfy dependencies (WasmLinkTest)
Error:      test/wasm_link_test.exs:29
     ** (EXIT from #PID<0.505.0>) bad return value: {:error, "Could not instantiate linked module: unknown import: `utils::sum` has not been defined"}

specifically on:

  • Elixir Compatibility / OTP 25.2 / Elixir 1.15.6
  • Elixir Compatibility / OTP 25.2 / Elixir 1.16.1
  • Elixir Compatibility / OTP 25.2 / Elixir 1.17.0

It might be an OTP 25 problem (although I fail to see how) or a flaky test (although suspiciously not failing on newer OTP versions). Do you have an idea what's going on here?

No, I haven't seen this error before. Will have to investigate.

@tessi
Copy link
Owner

tessi commented Jul 4, 2024

I saw this error when I was developing this -- specifically when there was a mismatch in store for linked modules vs the main module.

hmm, thinking about what you said. do you think it's worth/possible adding an extra check on the store being equal and raise a helpful error instead of this weird one? (given that this is also the error in case of mismatching stores it would be hard to identify what the problem is just from reading the error mesage)

@munjalpatel
Copy link
Contributor Author

I saw this error when I was developing this -- specifically when there was a mismatch in store for linked modules vs the main module.

hmm, thinking about what you said. do you think it's worth/possible adding an extra check on the store being equal and raise a helpful error instead of this weird one? (given that this is also the error in case of mismatching stores it would be hard to identify what the problem is just from reading the error mesage)

My bad, that was a different error about mis-match in stores, not the same error we are experiencing here. I'll take look what's up with this error.

* fix nested dynamic link issue

* add test for compiled dynamically linked module deps
@munjalpatel
Copy link
Contributor Author

@tessi we now have support for dynamically linked module dependencies and that should also resolve failing tests.

@tessi
Copy link
Owner

tessi commented Jul 5, 2024

@munjalpatel looks great! I guess we can spend time to implement the rust side "proper" when someone needs it. Sooner or later - I guess - linked modules will be replaced by the component model anyways. So maybe we never have to build it ;)

thanks again for your work here! ❤️

@tessi tessi merged commit 2c4ac3e into tessi:main Jul 5, 2024
13 checks passed
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.

Support for dynamic linking
3 participants