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

feat: make Module::imports be pub #573

Merged
merged 7 commits into from
Nov 24, 2022

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Nov 24, 2022

If we want to create stub function, we must need to know the imports info.

I think substrate also need this method to do prepare_import like wasmtime.

Copy link
Member

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

LGTM Also in Wasmtime this exact same API is also public.

@Robbepop
Copy link
Member

This PR is missing re-exporting the type ModuleImportsIter as public at the lib.rs. Should be a single line addition.

@paritytech-cicd-pr
Copy link

paritytech-cicd-pr commented Nov 24, 2022

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
bare_call_0
1.01ms 1.01ms ⚪ 0.05% 991.51µs 960.61µs 🔴 -2.74% 🟢 -5%
execute/
bare_call_0/typed
525.57µs 525.40µs ⚪ -0.07% 447.57µs 455.53µs 🔴 1.84% 🟢 -13%
execute/
bare_call_1
1.05ms 1.05ms 🟢 -0.35% 1.22ms 1.18ms 🔴 -3.27% 🟢 13%
execute/
bare_call_16
2.15ms 2.11ms 🟢 -2.11% 4.73ms 4.70ms 🔴 -1.02% 🔴 122%
execute/
bare_call_16/typed
1.50ms 1.50ms ⚪ -0.11% 2.34ms 2.51ms 🔴 7.08% 🟡 67%
execute/
bare_call_1/typed
607.98µs 607.96µs ⚪ -0.02% 736.56µs 738.81µs ⚪ 0.38% 🟢 22%
execute/
bare_call_4
1.19ms 1.24ms 🔴 3.76% 1.96ms 1.94ms 🔴 -0.96% 🟡 56%
execute/
bare_call_4/typed
619.03µs 630.79µs 🔴 1.93% 926.20µs 964.42µs 🔴 5.29% 🟡 53%
execute/
br_table
620.40µs 620.44µs ⚪ 0.00% 945.85µs 946.09µs ⚪ -0.09% 🟡 52%
execute/
count_until
710.50µs 711.06µs ⚪ 0.02% 2.19ms 2.18ms ⚪ -0.03% 🔴 207%
execute/
factorial_iterative
369.97µs 304.95µs 🟢 -17.28% 867.48µs 864.36µs ⚪ -0.19% 🔴 183%
execute/
factorial_recursive
604.79µs 609.61µs ⚪ 0.80% 1.30ms 1.28ms ⚪ -1.09% 🔴 111%
execute/
fib_iterative
1.49ms 1.43ms 🟢 -3.90% 4.62ms 4.61ms ⚪ -0.01% 🔴 222%
execute/
fib_recursive
5.75ms 6.04ms 🔴 5.98% 11.90ms 11.78ms ⚪ -1.09% 🟡 95%
execute/
global_bump
1.01ms 1.01ms ⚪ -0.01% 3.14ms 3.13ms ⚪ -0.18% 🔴 211%
execute/
global_const
802.72µs 716.49µs 🟢 -10.75% 2.59ms 2.48ms 🟢 -3.96% 🔴 246%
execute/
host_calls
29.95µs 29.01µs 🟢 -3.59% 41.77µs 41.08µs 🟢 -1.44% 🟢 42%
execute/
memory_fill
1.33ms 1.30ms 🟢 -2.15% 4.18ms 4.23ms 🔴 1.50% 🔴 225%
execute/
memory_sum
1.34ms 1.30ms 🟢 -3.14% 4.23ms 4.24ms ⚪ 0.12% 🔴 226%
execute/
memory_vec_add
2.68ms 2.58ms 🟢 -3.54% 8.46ms 8.50ms ⚪ 0.31% 🔴 230%
execute/
recursive_is_even
1.09ms 1.09ms ⚪ -0.29% 2.10ms 2.10ms ⚪ -0.13% 🟡 92%
execute/
recursive_ok
142.26µs 146.58µs 🔴 3.16% 298.39µs 298.72µs ⚪ 0.04% 🔴 104%
execute/
recursive_scan
178.34µs 178.24µs ⚪ -0.19% 381.61µs 382.62µs ⚪ 0.36% 🔴 115%
execute/
recursive_trap
13.92µs 13.94µs ⚪ 0.22% 30.01µs 30.03µs ⚪ 0.05% 🔴 115%
execute/
regex_redux
544.46µs 549.70µs ⚪ 1.01% 1.50ms 1.50ms ⚪ -0.08% 🔴 173%
execute/
rev_complement
507.95µs 499.95µs 🟢 -1.59% 1.49ms 1.49ms ⚪ 0.13% 🔴 198%
execute/
tiny_keccak
371.37µs 360.99µs 🟢 -3.01% 1.21ms 1.20ms ⚪ -0.25% 🔴 234%
execute/
trunc_f2i
916.85µs 911.09µs ⚪ -0.58% 2.52ms 2.51ms ⚪ -0.06% 🔴 176%
instantiate/
wasm_kernel
61.68µs 61.52µs ⚪ 0.38% 75.01µs 95.69µs 🔴 27.08% 🟡 56%
translate/
erc1155
205.57µs 207.87µs ⚪ 1.15% 407.18µs 408.46µs ⚪ 0.17% 🟡 96%
translate/
erc20
102.20µs 101.95µs ⚪ -0.14% 199.34µs 201.28µs ⚪ 0.94% 🟡 97%
translate/
erc721
145.82µs 146.04µs ⚪ 0.29% 290.25µs 289.22µs ⚪ -0.68% 🟡 98%
translate/
spidermonkey
0.00ns 0.00ns ⚪ -0.49% 0.00ns 0.00ns ⚪ -0.04% 🟢 0%
translate/
wasm_kernel
3.76ms 3.74ms ⚪ -0.43% 7.71ms 7.60ms ⚪ -1.04% 🔴 103%

Link to pipeline

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Nov 24, 2022

@Robbepop Hi, I will export it later.
But I have another point to mention, which is the same as the purpose of this PR. At present, Func::wrap does not support params in vec style now, which is different from wasmtime, will make it more troublesome to support feature such likeallow_miss_host_function.

@Robbepop
Copy link
Member

I will create a PR to fix the clippy warnings, then you need to merge it so that we can finally merge your PR.

Copy link
Member

@Robbepop Robbepop left a comment

Choose a reason for hiding this comment

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

Changes I'd like to see before we merge:

Since ModuleImport is going to be pub now we should clean up its API, too.
We should make its new constructor as well as its name method pub(crate). This way we can also remove ModuleImportName from the public re-exports and shrink the wasmi API surface again. The field and module method getters should be sufficient.

@codecov-commenter
Copy link

codecov-commenter commented Nov 24, 2022

Codecov Report

Merging #573 (8c5e2e6) into master (d9f5724) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #573   +/-   ##
=======================================
  Coverage   80.04%   80.04%           
=======================================
  Files          78       78           
  Lines        6323     6323           
=======================================
  Hits         5061     5061           
  Misses       1262     1262           
Impacted Files Coverage Δ
crates/wasmi/src/module/mod.rs 85.08% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@yjhmelody
Copy link
Contributor Author

Ok. I'm updating it.

@Robbepop
Copy link
Member

@Robbepop Hi, I will export it later. But I have another point to mention, which is the same as the purpose of this PR. At present, Func::wrap does not support params in vec style now, which is different from wasmtime, will make it more troublesome to support feature such likeallow_miss_host_function.

I don't understand what you mean by this? What are "params in vec style" for Func::wrap? What is the allow_miss_host_function feature about exactly? Please elaborate!

This might be better done in a separate PR if at all.

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Nov 24, 2022

@Robbepop Hi, I will export it later. But I have another point to mention, which is the same as the purpose of this PR. At present, Func::wrap does not support params in vec style now, which is different from wasmtime, will make it more troublesome to support feature such likeallow_miss_host_function.

I don't understand what you mean by this? What are "params in vec style" for Func::wrap? What is the allow_miss_host_function feature about exactly? Please elaborate!

This might be better done in a separate PR if at all.

Ok.
Sorry, I thought that you would have to deal with related upgrades of the substrate in the future, and the implementation may be similar to wasmtime.
https://github.com/paritytech/substrate/blob/8016da9ddf1c10a5a74cc5a273905cfeb60ed82c/client/executor/wasmtime/src/imports.rs#L69-L68

Yes, I'll try it first and then make a new PR

@yjhmelody
Copy link
Contributor Author

yjhmelody commented Nov 24, 2022

@Robbepop I do not know how to fix it:

342 |     /// An imported [`Global`].
    |                       ^^^^^^ this item is private
    |
    = note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings`

I think it's not private.

@Robbepop
Copy link
Member

@Robbepop I do not know how to fix it:

342 |     /// An imported [`Global`].
    |                       ^^^^^^ this item is private
    |
    = note: `-D rustdoc::private-intra-doc-links` implied by `-D warnings`

I think it's not private.

You should be able to fix it by adding the following two lines of doc comments:

///
/// [`Global`]: [`crate::Global`]

@Robbepop Robbepop merged commit 6ad8915 into wasmi-labs:master Nov 24, 2022
@yjhmelody yjhmelody deleted the feat-module-import-export branch November 24, 2022 15:25
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.

4 participants