Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Forward wasmer-sandbox feature to sp-sandbox #10268

Merged
merged 1 commit into from
Nov 16, 2021

Conversation

athei
Copy link
Member

@athei athei commented Nov 15, 2021

This fixes an issue where the wasmer-sandbox feature wasn't properly forwarded to sp-sandbox. The effect was that even though sc-executor was providing a wasmer sandbox (instead of a wasmi one), it wasn't used because sp-sandbox was using the embedded executor (as it should in production use cases to not rely on the sandboxing host functions). The reason why we need this forwarding is because sp-sandbox is part of the runtime and features supplied on the CLI are only applied when piped through the node-runtime crate.

You can see the effects from it in this PR where there is no difference in weight. The bug was introduced by: #9592.

I reran the end-to-end benchmarks on Linux with the following CLI:

cargo run --release --features runtime-benchmarks,[wasmer-sandbox] -- --extra --dev --execution=wasm --wasm-execution=compiled -p pallet_contracts -e ink_erc20_transfer

The results now clearly indicate that they were run with different execution engines. Unfortunately, it confirmed our suspicion that wasmer is even slower than wasmi on these example contracts. Weight benchmarks suggest that slowness of host function calling might be responsible. g = {0, 1} means without or with gas metering injected.

ink_erc20_transfer, wasmi

Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    g   mean µs  sigma µs       %
    0      5529     5.575    0.1%
    1      8053     6.898    0.0%

Quality and confidence:
param     error
g         1.267

Model:
Time ~=     5529
    + g     2523
              µs



ink_er20_transfer, wasmer

Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    g   mean µs  sigma µs       %
    0      5744     152.7    2.6%
    1     12000     141.7    1.1%

Quality and confidence:
param     error
g         29.76

Model:
Time ~=     5744
    + g     6260
              µs


--------------------------------------------------------------------------------------------------


solang_erc20_transfer, wasmi

Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    g   mean µs  sigma µs       %
    0      1909     8.551    0.4%
    1      2057     10.93    0.5%

Quality and confidence:
param     error
g         1.983

Model:
Time ~=     1909
    + g    147.5
              µs



solang_erc20_transfer, wasmer

Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    g   mean µs  sigma µs       %
    0      3423     121.5    3.5%
    1      3678     61.12    1.6%

Quality and confidence:
param     error
g         19.43

Model:
Time ~=     3423
    + g    255.2
              µs

@athei athei added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit. labels Nov 15, 2021
@athei athei requested review from bkchr and 0x7CFE November 15, 2021 18:05
@athei
Copy link
Member Author

athei commented Nov 16, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 07d98e2 into master Nov 16, 2021
@paritytech-processbot paritytech-processbot bot deleted the at-fix-wasmer branch November 16, 2021 06:52
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants