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(Linker): remove externals from Linker and make instantiate to use &self #512

Merged
merged 4 commits into from
Oct 14, 2022

Conversation

yjhmelody
Copy link
Contributor

Just a previous PR, but I mainly want to generalize instantiate method

@yjhmelody
Copy link
Contributor Author

🤔 Maybe I should not alter this method and just add a new one?

@paritytech-cicd-pr
Copy link

BENCHMARKS

NATIVEWASMTIME
BENCHMARKMASTERPRDIFFMASTERPRDIFFWASMTIME OVERHEAD
execute/
count_until
947.12µs 918.74µs 🟢 -3.17% 2.38ms 2.38ms ⚪ 0.00% 🔴 159%
execute/
factorial_iterative
401.70ns 398.65ns ⚪ -0.80% 997.16ns 998.89ns ⚪ 0.26% 🔴 151%
execute/
factorial_recursive
763.97ns 783.94ns 🔴 2.67% 1.55µs 1.57µs ⚪ 0.70% 🟡 100%
execute/
fib_iterative
1.80ms 1.87ms 🔴 3.74% 5.11ms 5.11ms ⚪ -0.04% 🔴 174%
execute/
fib_recursive
7.01ms 7.16ms 🔴 2.22% 14.03ms 14.29ms 🔴 1.93% 🟡 99%
execute/
global_bump
1.48ms 1.36ms 🟢 -7.84% 3.64ms 4.63ms 🔴 27.16% 🔴 240%
execute/
host_calls
32.00µs 32.32µs ⚪ 0.19% 45.20µs 45.11µs ⚪ -0.21% 🟢 40%
execute/
memory_fill
1.67ms 1.70ms 🔴 1.64% 4.33ms 4.33ms ⚪ -0.18% 🔴 155%
execute/
memory_sum
1.57ms 1.59ms 🔴 1.28% 4.36ms 4.36ms ⚪ -0.03% 🔴 174%
execute/
memory_vec_add
3.25ms 3.16ms 🟢 -2.72% 8.94ms 8.94ms ⚪ -0.02% 🔴 183%
execute/
recursive_is_even
1.35ms 1.35ms ⚪ -0.11% 2.55ms 2.57ms ⚪ 0.68% 🟡 90%
execute/
recursive_ok
179.57µs 181.76µs 🔴 1.29% 349.32µs 356.84µs 🔴 2.19% 🟡 96%
execute/
recursive_scan
219.62µs 216.80µs ⚪ -1.47% 485.37µs 460.62µs 🟢 -5.20% 🔴 112%
execute/
recursive_trap
17.73µs 17.21µs 🟢 -2.97% 36.89µs 35.75µs 🟢 -3.28% 🔴 108%
execute/
regex_redux
676.86µs 720.77µs 🔴 6.83% 1.60ms 1.62ms 🔴 1.60% 🔴 125%
execute/
rev_complement
625.28µs 618.27µs 🟢 -1.24% 1.55ms 1.56ms ⚪ 0.74% 🔴 153%
execute/
tiny_keccak
485.53µs 514.71µs 🔴 6.75% 1.48ms 1.42ms 🟢 -4.28% 🔴 175%
execute/
trunc_f2i
1.01ms 1.02ms ⚪ 0.69% 2.74ms 2.72ms ⚪ -0.51% 🔴 166%
instantiate/
wasm_kernel
67.73µs 67.77µs ⚪ 0.79% 126.62µs 94.25µs 🟢 -25.43% 🟢 39%
translate/
wasm_kernel
4.83ms 4.83ms ⚪ -0.12% 9.58ms 9.27ms 🟢 -3.30% 🟡 92%

Link to pipeline

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.

Hmmm this is already the 2nd time you requested this PR.
However, I took another look and the Wasmtime API also takes a &self reference for the Linker::instantiate method. So this is indeed bad since we ideally want to mirror the Wasmtime API as closely as possible.
Since your changes do not display significant performance differences I am inclined to actually merging it. However, it is not ideal since we are (again) very dependent on Rust and LLVM optimizing the allocations away. If those optimizations no longer apply we will see significant regressions.

@Robbepop Robbepop merged commit 753adce into wasmi-labs:master Oct 14, 2022
@yjhmelody
Copy link
Contributor Author

@Robbepop you could keep the inner buf, but add a &mut self version.

@yjhmelody yjhmelody deleted the chore-rm-externals branch October 14, 2022 09:16
@Robbepop
Copy link
Member

@Robbepop you could keep the inner buf, but add a &mut self version.

As I said we want to closely mirror the Wasmtime API and since the Wasmtime API does not have 2 APIs for that functionality we won't introduce them either.

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