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

Support closures as host functions tracking issue #1840

Closed
MarkMcCaskey opened this issue Nov 24, 2020 · 5 comments · Fixed by #2892
Closed

Support closures as host functions tracking issue #1840

MarkMcCaskey opened this issue Nov 24, 2020 · 5 comments · Fixed by #2892

Comments

@MarkMcCaskey
Copy link
Contributor

Wasmer does not currently support closures (functions with captured environments) as host functions, this is a tracking issue for progress on this feature.

The current plan is to reject closures in 1.0 and possibly support them in the future. Please let us know if this feature is important to your use case.

@MarkMcCaskey MarkMcCaskey added the 🎉 enhancement New feature! label Nov 24, 2020
bors bot added a commit that referenced this issue Nov 30, 2020
1841: Disable closures as host functions for now + docs + tests r=syrusakbary a=MarkMcCaskey

Resolves #1811 for now

Part of #1840 

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <mark@wasmer.io>
Co-authored-by: Syrus Akbary <me@syrusakbary.com>
bors bot added a commit that referenced this issue Nov 30, 2020
1841: Disable closures as host functions for now + docs + tests r=MarkMcCaskey a=MarkMcCaskey

Resolves #1811 for now

Part of #1840 

# Review

- [x] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Mark McCaskey <mark@wasmer.io>
Co-authored-by: Syrus Akbary <me@syrusakbary.com>
@aidanhs
Copy link

aidanhs commented Jan 13, 2021

This seems like it'd help the ergonomics of environments a bunch.

Simple example:

Function::new_native_with_env(&store, script_len,
    move |&script_len_env: &u32| -> u32 { script_len_env })

vs

Function::new_native(&store,
    move || -> u32 { script_len })

Slightly more complex example:

struct MemEnv {
    #[wasmer(export)]
    memory: LazyInit<Memory>,
}
Function::new_native_with_env(&store, Default::default(),
    move |env: &MemEnv, data: WasmPtr<u8, Array>| -> () {    
          let memory = env.memory_ref().expect("memory not set up");    
          let data_cell = mytry!(data.deref(memory, 0, script_len).ok_or(()));    
          for (cell, &b) in data_cell.into_iter().zip(script.iter()) {    
              cell.set(b)    
          }    
      })

vs

struct ScriptEnv {
    script: Vec<u8>,
    script_len: u32,
    #[wasmer(export)]
    memory: LazyInit<Memory>,
}
let script_env = ScriptEnv { script, script_len, memory: Default::default() };
Function::new_native_with_env(&store, script_env,
    move |env: &ScriptEnv, data: WasmPtr<u8, Array>| -> () {    
          let memory = env.memory_ref().expect("memory not set up");    
          let data_cell = mytry!(data.deref(memory, 0, env.script_len).ok_or(()));    
          for (cell, &b) in data_cell.into_iter().zip(env.script.iter()) {    
              cell.set(b)    
          }    
      })

This latter example has a couple of points worth observing:

  1. many functions may want a Memory, and so if I could capture environments I can use the single MemoryEnv struct between them and capture the rest. With no capturing, I have to define a fresh struct for every set of things I want to carry in the environment
  2. a WasiEnv has a Clone bound, whereas a capturing Fn does not. This is relevant in the case above because script may be very large - I don't want to clone a script every time the function is called and it's not clear based on the trait bounds of WasiEnv if that might be the case. With a non-Cloneable Fn, I can be confident there's no data copying on calling my Function. Note that this point could be resolved by just documenting why the Clone bound exists on WasiEnv so that people can create data structures appropriately.

@Hywan Hywan added 📦 lib-api About wasmer 📦 lib-vm About wasmer-vm labels Jul 16, 2021
@Hywan Hywan added this to 📬 Backlog in Wasmer Runtime Issue Board via automation Jul 16, 2021
@Hywan Hywan moved this from 📬 Backlog to 🏁 Ready in Wasmer Runtime Issue Board Jul 16, 2021
@Amanieu Amanieu added the priority-medium Medium priority issue label Oct 20, 2021
@Amanieu Amanieu self-assigned this Oct 20, 2021
@syrusakbary
Copy link
Member

This should be solved by Wasmer 3.0

@syrusakbary syrusakbary moved this from 🏁 Ready to 📬 Backlog in Wasmer Runtime Issue Board Apr 27, 2022
@bors bors bot closed this as completed in #2892 Jul 19, 2022
@bors bors bot closed this as completed in 06474c1 Jul 19, 2022
@silwol
Copy link
Contributor

silwol commented Aug 1, 2022

The current master still contains an unimplemented!() call pointing to this issue at lib/api/src/js/externals/function.rs:482. Is it possible that this was changed in an inconsistent manner between lib/api/src/sys and lib/api/src/js? If the issue is not present in wasmer 3, the pointer to this issue shouldn't be necessary, I assume.
cc @syrusakbary

@epilys
Copy link
Contributor

epilys commented Aug 1, 2022

@silwol Yeah, lib/api/src/sys and lib/api/src/js need to be synced (-> in this direction). This should be a new ticket

@Hpmason
Copy link

Hpmason commented Mar 14, 2023

Docs for 3.1.1 and 3.2.0-alpha still say that Function will panic with captured env. Does that still need to be updated?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-api About wasmer 📦 lib-vm About wasmer-vm priority-medium Medium priority issue 🚧 work in progress
Projects
Development

Successfully merging a pull request may close this issue.

8 participants