Add support for const function signatures #1911

webmaster128 · 2020-12-10T10:55:40Z

Description

FunctionType contains variable length vectors and I'm sure there is a good reason for this design. However, it makes creating them a bit annoying since it connot be done in constants. With this PR I propose a representation that uses a tuple of arrays, which can be converted into FunctionType on demand. This allows developers to use constant signatures and push them to a better place in their code.

In https://github.com/CosmWasm/cosmwasm/pull/659/files you see a demonstration of how this deduplicates code and pushes the definitions out of my way.

Since the conversion &FunctionType to FunctionType is implemented here, this should no be breaking anyone's code. In cases where you can pass an owned FunctionType instead of &FunctionType you save one clone.

Review

Add a short description of the the change to the CHANGELOG.md file

Hywan

I like the idea. I don't see exactly how it improves Wasmer itself, but I can understand how it can be convenient for the user!

I'm approving this, but I would appreciate another feedback on this from @syrusakbary, @nlewycky, @MarkMcCaskey or @jubianchi before merging :-).

lib/api/src/externals/function.rs

Hywan · 2020-12-11T09:25:45Z

lib/wasmer-types/src/types.rs

+impl From<&FunctionType> for FunctionType {
+    fn from(as_ref: &FunctionType) -> Self {
+        as_ref.clone()
+    }
+}


syrusakbary

Looks good to me!

syrusakbary · 2020-12-11T15:00:48Z

bors r+

1911: Add support for const function signatures r=syrusakbary a=webmaster128 # Description `FunctionType` contains variable length vectors and I'm sure there is a good reason for this design. However, it makes creating them a bit annoying since it connot be done in constants. With this PR I propose a representation that uses a tuple of arrays, which can be converted into `FunctionType` on demand. This allows developers to use constant signatures and push them to a better place in their code. In https://github.com/CosmWasm/cosmwasm/pull/659/files you see a demonstration of how this deduplicates code and pushes the definitions out of my way. Since the conversion `&FunctionType` to `FunctionType` is implemented here, this should no be breaking anyone's code. In cases where you can pass an owned `FunctionType` instead of `&FunctionType` you save one clone. # Review - [ ] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Simon Warta <[email protected]> Co-authored-by: Syrus Akbary <[email protected]>

bors · 2020-12-11T15:21:14Z

Build failed:

Test on linux-x64

webmaster128 · 2020-12-12T18:23:15Z

yo bors, I rebased this PR to lastest master and fixed the compile error

See wasmerio/wasmer#1911

Hywan · 2020-12-14T10:24:17Z

bors r+

bors · 2020-12-14T10:24:33Z

Merge conflict.

Hywan · 2020-12-14T10:27:05Z

bors r+

1911: Add support for const function signatures r=Hywan a=webmaster128 # Description `FunctionType` contains variable length vectors and I'm sure there is a good reason for this design. However, it makes creating them a bit annoying since it connot be done in constants. With this PR I propose a representation that uses a tuple of arrays, which can be converted into `FunctionType` on demand. This allows developers to use constant signatures and push them to a better place in their code. In https://github.com/CosmWasm/cosmwasm/pull/659/files you see a demonstration of how this deduplicates code and pushes the definitions out of my way. Since the conversion `&FunctionType` to `FunctionType` is implemented here, this should no be breaking anyone's code. In cases where you can pass an owned `FunctionType` instead of `&FunctionType` you save one clone. # Review - [x] Add a short description of the the change to the CHANGELOG.md file Co-authored-by: Simon Warta <[email protected]> Co-authored-by: Ivan Enderlin <[email protected]>

bors · 2020-12-14T10:47:08Z

Build failed:

Test on linux-x64

webmaster128 · 2020-12-14T10:51:50Z

Oh, the failing test now shows we are using some &&wasmer::FunctionType. I guess this is not intended and should be &wasmer::FunctionType, right?

error[E0277]: the trait bound `wasmer::FunctionType: std::convert::From<&&wasmer::FunctionType>` is not satisfied
   --> lib/c-api/src/wasm_c_api/externals/function.rs:81:20
    |
81  |     let function = Function::new(&store.inner, &func_sig, inner_callback);
    |                    ^^^^^^^^^^^^^ the trait `std::convert::From<&&wasmer::FunctionType>` is not implemented for `wasmer::FunctionType`
    | 
   ::: /home/runner/work/wasmer/wasmer/lib/api/src/externals/function.rs:102:13
    |
102 |         FT: Into<FunctionType>,
    |             ------------------ required by this bound in `wasmer::Function::new`
    |
    = help: the following implementations were found:
              <wasmer::FunctionType as std::convert::From<&wasmer::FunctionType>>
              <wasmer::FunctionType as std::convert::From<([wasmer::ValType; 0], [wasmer::ValType; 0])>>
              <wasmer::FunctionType as std::convert::From<([wasmer::ValType; 0], [wasmer::ValType; 1])>>
              <wasmer::FunctionType as std::convert::From<([wasmer::ValType; 0], [wasmer::ValType; 2])>>
            and 97 others
    = note: required because of the requirements on the impl of `std::convert::Into<wasmer::FunctionType>` for `&&wasmer::FunctionType`

error[E0277]: the trait bound `wasmer::FunctionType: std::convert::From<&&wasmer::FunctionType>` is not satisfied
   --> lib/c-api/src/wasm_c_api/externals/function.rs:155:20
    |
155 |     let function = Function::new_with_env(
    |                    ^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<&&wasmer::FunctionType>` is not implemented for `wasmer::FunctionType`
    | 
   ::: /home/runner/work/wasmer/wasmer/lib/api/src/externals/function.rs:178:13
    |
178 |         FT: Into<FunctionType>,
    |             ------------------ required by this bound in `wasmer::Function::new_with_env`
    |
    = help: the following implementations were found:
              <wasmer::FunctionType as std::convert::From<&wasmer::FunctionType>>
              <wasmer::FunctionType as std::convert::From<([wasmer::ValType; 0], [wasmer::ValType; 0])>>
              <wasmer::FunctionType as std::convert::From<([wasmer::ValType; 0], [wasmer::ValType; 1])>>
              <wasmer::FunctionType as std::convert::From<([wasmer::ValType; 0], [wasmer::ValType; 2])>>
            and 97 others
    = note: required because of the requirements on the impl of `std::convert::Into<wasmer::FunctionType>` for `&&wasmer::FunctionType`

error: aborting due to 2 previous errors

webmaster128 · 2020-12-14T12:05:11Z

I'll take care of this now

webmaster128 · 2020-12-14T12:48:40Z

Rebased to latest master and 1 commit added (8a8e8dc)

Hywan · 2020-12-14T13:39:10Z

bors try

bors · 2020-12-14T15:10:48Z

try

Build succeeded:

MarkMcCaskey · 2020-12-14T15:16:54Z

bors r+

webmaster128 · 2020-12-14T16:29:19Z

Could you restart this job? https://github.com/wasmerio/wasmer/runs/1551129968 Seems like it never started

bors · 2020-12-14T16:49:53Z

Build succeeded:

MarkMcCaskey · 2020-12-14T21:43:12Z

Random follow up but I wanted to ping you, @webmaster128 , to make sure you know about NativeFunc: if you know the signature of your host function at compile time, you can avoid specifying the type signature of your host function entirely by using Function::new_native and Function::new_native_with_env and it should have less runtime overhead.

We just had another issue where someone missed that, so I wanted to make sure you knew about it too in case the docs aren't clear here.

webmaster128 · 2020-12-14T21:55:03Z

Thank you @MarkMcCaskey, much appreciated. This was not clear to me indeed for a long time, especially when upgrading from statically typed Func closures in 0.17 to the new API. Now I already migrated all functions to native that do not capture variables. One function could be refactored to storing a variable in Env instead of capturing. And some are remaining to be refactored from capturing closures to native functions accessing data via Env.

webmaster128 · 2020-12-14T21:59:01Z

I'm very open to discuss if this PR is more destructive than helpful. Does it encourage users to follow the wrong path? Could all those testing dummies be replaced with statically typed closures? CosmWasm/cosmwasm@65a7d1f

MarkMcCaskey · 2020-12-14T22:07:37Z

@webmaster128 Yeah, it seems like those examples could be made into "native" functions if you wanted to! Don't worry about it, I think this PR is a good change, it's nice to have the ability to avoid heap allocations. I've updated the docs locally to make it more clear what the options are!

Just wanted to make sure you knew about it in case you didn't!

webmaster128 · 2020-12-14T22:14:23Z

Alright cool. One thought why this might be tricky for people who are migrating: it seems like the default chained from native to dynamic. To me Function::new feels like the direct replacement of Func::new. But it looks like you got a native function in 0.17 and get a dynamic function in 1.0.

webmaster128 requested review from Hywan, MarkMcCaskey and syrusakbary as code owners December 10, 2020 10:55

webmaster128 mentioned this pull request Dec 10, 2020

Upgrade to Wasmer Reborn (part 1) CosmWasm/cosmwasm#504

Merged

Hywan approved these changes Dec 11, 2020

View reviewed changes

Hywan self-assigned this Dec 11, 2020

Hywan added 🎉 enhancement New feature! 📦 lib-types About wasmer-types 🧪 tests I love tests labels Dec 11, 2020

syrusakbary approved these changes Dec 11, 2020

View reviewed changes

webmaster128 force-pushed the const-FunctionType branch from 6329073 to 8c73ec8 Compare December 12, 2020 18:21

webmaster128 added a commit to CosmWasm/cosmwasm that referenced this pull request Dec 14, 2020

Update Wasmer to version with constant signatures

a7cd3dc

See wasmerio/wasmer#1911

webmaster128 mentioned this pull request Dec 14, 2020

Const signatures CosmWasm/cosmwasm#659

Closed

webmaster128 added 3 commits December 14, 2020 13:06

Implement from pair to FunctionType

96abcb3

Accept Into<FunctionType> as signatures

69f834a

Avoid unnecessary &&wasmer::FunctionType

8a8e8dc

webmaster128 force-pushed the const-FunctionType branch from e33202a to 8a8e8dc Compare December 14, 2020 12:48

webmaster128 requested a review from jubianchi as a code owner December 14, 2020 12:48

bors bot added a commit that referenced this pull request Dec 14, 2020

Try #1911:

a6ea240

MarkMcCaskey approved these changes Dec 14, 2020

View reviewed changes

bors bot merged commit ae9a5f0 into wasmerio:master Dec 14, 2020

webmaster128 deleted the const-FunctionType branch December 14, 2020 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for const function signatures #1911

Add support for const function signatures #1911

webmaster128 commented Dec 10, 2020 •

edited by Hywan

Loading

Hywan left a comment

Hywan Dec 11, 2020

syrusakbary left a comment

syrusakbary commented Dec 11, 2020

bors bot commented Dec 11, 2020

webmaster128 commented Dec 12, 2020

Hywan commented Dec 14, 2020

bors bot commented Dec 14, 2020

Hywan commented Dec 14, 2020

bors bot commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

Hywan commented Dec 14, 2020

bors bot commented Dec 14, 2020

MarkMcCaskey commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

bors bot commented Dec 14, 2020

MarkMcCaskey commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

MarkMcCaskey commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

Add support for const function signatures #1911

Add support for const function signatures #1911

Conversation

webmaster128 commented Dec 10, 2020 • edited by Hywan Loading

Description

Review

Hywan left a comment

Choose a reason for hiding this comment

Hywan Dec 11, 2020

Choose a reason for hiding this comment

syrusakbary left a comment

Choose a reason for hiding this comment

syrusakbary commented Dec 11, 2020

bors bot commented Dec 11, 2020

webmaster128 commented Dec 12, 2020

Hywan commented Dec 14, 2020

bors bot commented Dec 14, 2020

Hywan commented Dec 14, 2020

bors bot commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

Hywan commented Dec 14, 2020

bors bot commented Dec 14, 2020

try

MarkMcCaskey commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

bors bot commented Dec 14, 2020

MarkMcCaskey commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

MarkMcCaskey commented Dec 14, 2020

webmaster128 commented Dec 14, 2020

webmaster128 commented Dec 10, 2020 •

edited by Hywan

Loading