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

Wasm-builder 3.0 #7532

Merged
merged 13 commits into from
Nov 24, 2020
Merged

Wasm-builder 3.0 #7532

merged 13 commits into from
Nov 24, 2020

Conversation

bkchr
Copy link
Member

@bkchr bkchr commented Nov 12, 2020

It's time for a new wasm-builder era :P

This pr adds the following notable changes:

  • Instead of building all wasm binaries in one big workspace, we now build each wasm binary in its own project. Is this guy crazy? No! Why have I done this?

    1. It eliminates some problems with wasm-builder are being stuck: wasm-builder stalls build #7375. Before we always had to lock the whole workspace when doing any change to build a wasm binary. This is now gone.
    2. When updating the git reference, it could happen that the build was failing the first time because there was a race condition updating all the project of the wasm workspace: Collator build fails on the first run cumulus#174
    3. Because we don't do 1 anymore we can build all wasm binaries in parallel, this lead to a build time decrease in Polkadot (release build) of around 40-50 seconds on my machine. It may happens that on machines with a small number of cores, the compile time could increase because we now build the dependencies for the wasm binaries multiple times.
  • The next big change is the removal of the wasm-builder-runner. This is achieved by using the unstable host_deps feature of cargo. Luckily we are using nightly anyway and being able to use this new feature. This simplifies the build process extremely, because we can include the wasm-builder directly as dependency and aren't forced anymore to do a special dependency handling for the wasm-builder in the build.rs file.

  • wasm-builder and wasm-builder-runner are updated to 3.0.0 on crates.io when this pr is approved. While the later one will be an empty crate that just points on the former one saying that people now only need to use this one.

Fixes: #7375
Fixes: paritytech/cumulus#174

Building all wasm crates in one workspace was a nice idea, however it
just introduced problems:

1. We needed to prune old members, but this didn't worked for old git
deps.
2. We locked the whole wasm workspace while building one crate. This
could lead to infinitely locking the workspace on a crash.

Now we just build every crate in its own project, this means we will
build the dependencies multiple times. While building the dependencies
multiple times, we still decrease the build time by around 30 seconds
for Polkadot and Substrate because of the new parallelism ;)
This removes the requirement on wasm-builder-runner by using the new
`build_dep` feature of cargo. We use nightly anyway and that enables us
to use this feature. This solves the problem of not mixing
build/proc-macro deps with normal deps. By doing this we get rid off
this complicated project structure and can depend directly on
`wasm-builder`. This also removes all the code from wasm-builder-runner
and mentions that it is deprecated.
@bkchr bkchr 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. labels Nov 12, 2020
@bkchr bkchr requested review from andresilva and pepyakin November 12, 2020 19:00
@pepyakin
Copy link
Contributor

I hate to say it, but initial testing indicates that the build time increased from 13m 55s (bf78c16) to 14m 36s (63d7c09, the last commit of this PR at the moment). This is after cargo clean. My machine has I9-9880H, 8 cores (16 threads).

@bkchr
Copy link
Member Author

bkchr commented Nov 13, 2020

I hate to say it, but initial testing indicates that the build time increased from 13m 55s (bf78c16) to 14m 36s (63d7c09, the last commit of this PR at the moment). This is after cargo clean. My machine has I9-9880H, 8 cores (16 threads).

What have you run? Just a cargo build?

Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

👍

frame/support/test/tests/decl_module_ui.rs Outdated Show resolved Hide resolved
frame/support/test/tests/decl_storage_ui.rs Outdated Show resolved Hide resolved
frame/support/test/tests/derive_no_bound_ui.rs Outdated Show resolved Hide resolved
frame/support/test/tests/reserved_keyword.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/builder.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/builder.rs Outdated Show resolved Hide resolved
utils/wasm-builder/src/builder.rs Show resolved Hide resolved
utils/wasm-builder/src/wasm_project.rs Show resolved Hide resolved
utils/wasm-builder/src/wasm_project.rs Show resolved Hide resolved
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

As far as I can see, code looks good.

there is still some occurences of SKIP_WASM_BINARY:

./primitives/runtime-interface/test-wasm-deprecated/src/lib.rs:29:/// Wasm binary unwrapped. If built with `SKIP_WASM_BINARY`, the function panics.
./frame/support/test/tests/reserved_keyword.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./frame/support/test/tests/derive_no_bound_ui.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./frame/support/test/tests/decl_module_ui.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./frame/support/test/tests/decl_storage_ui.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./client/executor/runtime-test/src/lib.rs:7:/// Wasm binary unwrapped. If built with `SKIP_WASM_BINARY`, the function panics.

@bkchr
Copy link
Member Author

bkchr commented Nov 23, 2020

As far as I can see, code looks good.

there is still some occurences of SKIP_WASM_BINARY:

./primitives/runtime-interface/test-wasm-deprecated/src/lib.rs:29:/// Wasm binary unwrapped. If built with `SKIP_WASM_BINARY`, the function panics.
./frame/support/test/tests/reserved_keyword.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./frame/support/test/tests/derive_no_bound_ui.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./frame/support/test/tests/decl_module_ui.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./frame/support/test/tests/decl_storage_ui.rs:22:	std::env::set_var("SKIP_WASM_BINARY", "1");
./client/executor/runtime-test/src/lib.rs:7:/// Wasm binary unwrapped. If built with `SKIP_WASM_BINARY`, the function panics.

It seems I was drunk 🤦

@bkchr bkchr requested review from gui1117 and andresilva November 23, 2020 18:56
@bkchr bkchr merged commit 7e94f01 into master Nov 24, 2020
@bkchr bkchr deleted the bkchr-wasm-builder-improvements branch November 24, 2020 09:18
clearloop added a commit to patractlabs/substrate that referenced this pull request Nov 25, 2020
* make ClientConfig public (paritytech#7544)

* sc-basic-authorship: remove useless dependencies (paritytech#7550)

Signed-off-by: koushiro <[email protected]>

* Add slashing events to elections-phragmen. (paritytech#7543)

* Add slashing events to elections-phragmen.

* Fix build

* Apply suggestions from code review

* Update frame/elections-phragmen/src/lib.rs

* Update frame/elections-phragmen/src/lib.rs

Co-authored-by: Guillaume Thiolliere <[email protected]>

Co-authored-by: Guillaume Thiolliere <[email protected]>

* Remove necessity to pass ConsensusEngineId when registering notifications protocol (paritytech#7549)

* Remove necessity to pass ConsensusEngineId when registering notifications protocol

* Line width

* Fix tests protocol name

* Other renames

* Doc update

* Change issue in TODO

* sc-cli: replace bip39 with tiny-bip39 (paritytech#7551)

Signed-off-by: koushiro <[email protected]>

* Add extra docs to on_initialize (paritytech#7552)

* Add some extra on_initialize docs.

* Address review comments.

* More Extensible Multiaddress Format (paritytech#7380)

* More extensible multiaddress format

* update name

* Don't depend on indices to define multiaddress type

* Use MultiAddress in Node Template too!

* reduce traits, fix build

* support multiple `StaticLookup`

* bump tx version

* feedback

* Fix weight template to remove ugliness in rust doc (paritytech#7565)

fixed weight template

* Cargo.lock: Run cargo update (paritytech#7553)

* Cargo.lock: Run cargo update

* Cargo.lock: Downgrade cc to v1.0.62

* Cargo.lock: Revert wasm-* updates

* .github: Add dependabot config and thus enable dependabot (paritytech#7509)

* .github: Add dependabot config and thus enable dependabot

* Update .github/dependabot.yml

Co-authored-by: Pierre Krieger <[email protected]>

Co-authored-by: Pierre Krieger <[email protected]>

* Thread-local parameter_types for testing. (paritytech#7542)

* Thread-local parameter_types for testing.

* Better docs.

* Some minors

* Merge'em

* Update frame/support/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>

* Align more to basti's trick

* Update frame/support/src/lib.rs

* Update frame/support/src/lib.rs

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

* Bump wasm-bindgen-test from 0.3.12 to 0.3.17 (paritytech#7567)

* Bump wasm-bindgen-test from 0.3.12 to 0.3.17

Bumps [wasm-bindgen-test](https://github.com/rustwasm/wasm-bindgen) from 0.3.12 to 0.3.17.
- [Release notes](https://github.com/rustwasm/wasm-bindgen/releases)
- [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rustwasm/wasm-bindgen/commits)

Signed-off-by: dependabot[bot] <[email protected]>

* Update wasm-bindgen pin to 0.2.68

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pierre Krieger <[email protected]>

* pallet-evm: move to Frontier (Part IV) (paritytech#7573)

* Bump secrecy from 0.6.0 to 0.7.0 (paritytech#7568)

* Bump secrecy from 0.6.0 to 0.7.0

Bumps [secrecy](https://github.com/iqlusioninc/crates) from 0.6.0 to 0.7.0.
- [Release notes](https://github.com/iqlusioninc/crates/releases)
- [Commits](iqlusioninc/crates@secrecy/v0.6.0...secrecy/v0.7.0)

Signed-off-by: dependabot[bot] <[email protected]>

* Fix compilation errors

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Pierre Krieger <[email protected]>

* Bump wasm-bindgen-futures from 0.4.17 to 0.4.18 (paritytech#7572)

Bumps [wasm-bindgen-futures](https://github.com/rustwasm/wasm-bindgen) from 0.4.17 to 0.4.18.
- [Release notes](https://github.com/rustwasm/wasm-bindgen/releases)
- [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rustwasm/wasm-bindgen/commits)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump lru from 0.4.3 to 0.6.1 (paritytech#7577)

Bumps [lru](https://github.com/jeromefroe/lru-rs) from 0.4.3 to 0.6.1.
- [Release notes](https://github.com/jeromefroe/lru-rs/releases)
- [Changelog](https://github.com/jeromefroe/lru-rs/blob/master/CHANGELOG.md)
- [Commits](jeromefroe/lru-rs@0.4.3...0.6.1)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump wasm-bindgen-test from 0.3.17 to 0.3.18 (paritytech#7579)

Bumps [wasm-bindgen-test](https://github.com/rustwasm/wasm-bindgen) from 0.3.17 to 0.3.18.
- [Release notes](https://github.com/rustwasm/wasm-bindgen/releases)
- [Changelog](https://github.com/rustwasm/wasm-bindgen/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rustwasm/wasm-bindgen/commits)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump wasm-timer from 0.2.4 to 0.2.5 (paritytech#7578)

Bumps [wasm-timer](https://github.com/tomaka/wasm-timer) from 0.2.4 to 0.2.5.
- [Release notes](https://github.com/tomaka/wasm-timer/releases)
- [Commits](https://github.com/tomaka/wasm-timer/commits)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* grandpa: remove light-client specific block import pipeline (paritytech#7546)

* grandpa: remove light-client specific block import

* consensus, network: remove finality proofs

* client/authority-discovery: Publish and query on exponential interval (paritytech#7545)

* client/authority-discovery: Publish and query on exponential interval

When a node starts up publishing and querying might fail due to various
reasons, for example due to being not yet fully bootstrapped on the DHT.
Thus one should retry rather sooner than later. On the other hand, a
long running node is likely well connected and thus timely retries are
not needed. For this reasoning use an exponentially increasing interval
for `publish_interval`, `query_interval` and
`priority_group_set_interval` instead of a constant interval.

* client/authority-discovery/src/interval.rs: Add license header

* .maintain/gitlab: Ensure adder collator tests are run on CI

* sc-network: update some dependencies (paritytech#7582)

Signed-off-by: koushiro <[email protected]>

* Bump linregress and do some other cleanups (paritytech#7580)

* Wasm-builder 3.0 (paritytech#7532)

* Build every wasm crate in its own project with wasm-builder

Building all wasm crates in one workspace was a nice idea, however it
just introduced problems:

1. We needed to prune old members, but this didn't worked for old git
deps.
2. We locked the whole wasm workspace while building one crate. This
could lead to infinitely locking the workspace on a crash.

Now we just build every crate in its own project, this means we will
build the dependencies multiple times. While building the dependencies
multiple times, we still decrease the build time by around 30 seconds
for Polkadot and Substrate because of the new parallelism ;)

* Remove the requirement on wasm-builder-runner

This removes the requirement on wasm-builder-runner by using the new
`build_dep` feature of cargo. We use nightly anyway and that enables us
to use this feature. This solves the problem of not mixing
build/proc-macro deps with normal deps. By doing this we get rid off
this complicated project structure and can depend directly on
`wasm-builder`. This also removes all the code from wasm-builder-runner
and mentions that it is deprecated.

* Copy the `Cargo.lock` to the correct folder

* Remove wasm-builder-runner

* Update docs

* Fix deterministic check

Modified-by: Bastian Köcher <[email protected]>

* Try to make the ui test happy

* Switch to `SKIP_WASM_BUILD`

* Rename `SKIP_WASM_BINARY` to the correct name...

* Update utils/wasm-builder/src/builder.rs

Co-authored-by: André Silva <[email protected]>

* Update utils/wasm-builder/src/builder.rs

Co-authored-by: André Silva <[email protected]>

Co-authored-by: André Silva <[email protected]>

* Bump tracing from 0.1.21 to 0.1.22 (paritytech#7589)

Bumps [tracing](https://github.com/tokio-rs/tracing) from 0.1.21 to 0.1.22.
- [Release notes](https://github.com/tokio-rs/tracing/releases)
- [Commits](tokio-rs/tracing@tracing-0.1.21...tracing-0.1.22)

Signed-off-by: dependabot[bot] <[email protected]>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* contracts: Add `salt` argument to contract instantiation (paritytech#7482)

* pallet-contracts: Fix seal_restore_to to output proper module errors

Those errors where part of the decl_error for some time but where
never actually returned. This allows proper debugging of failed
restorations. Previously, any error did return the misleading
`ContractTrapped`.

* Bind UncheckedFrom<T::Hash> + AsRef<[u8]> everywhere

This allows us to make assumptions about the AccoutId
that are necessary for testing and in order to benchmark
the module properly.

This also groups free standing functions into inherent functions
in order to minimize the places where the new bounds need to
be specified.

* Rework contract address determination

* Do not allow override by runtime author
* Instantiate gained a new parameter "salt"

This change is done now in expecation of the upcoming code rent
which needs to change the instantiation dispatchable and
host function anyways.

The situation in where we have only something that is like CREATE2
makes it impossible for UIs to help the user to create an arbitrary
amount of instantiations from the same code.

With this change we have the same functionality as ethereum with
a CREATE and CREATE2 instantation semantic.

* Remove TrieIdGenerator

The new trait bounds allows us to remove this workaround
from the configuration trait.

* Remove default parameters for config trait

It should be solely the responsiblity to determine proper values for
these parameter. As a matter of fact most runtime weren't using these
values anyways.

* Fix tests for new account id type

Because of the new bounds on the trait tests can't get away by using
u64 as accound id. Replacing the 8 byte value by a 32 byte value
creates out quite a bit of code churn.

* Fix benchmarks

The benchmarks need adaption to the new instantiate semantics.

* Fix compile errors caused by adding new trait bounds
* Fix compile errors caused by renaming storage and rent functions
* Adapt host functions and dispatchables to the new salt
* Add tests for instantiate host functions (was not possible before)

* Add benchmark results

* Adapt to the new WeightInfo

The new benchmarks add a new parameter for salt "s" to the instantiate weights
that needs to be applied.

* Fix deploying_wasm_contract_should_work integration test

This test is adapted to use the new instantiate signature.

* Break overlong line

* Break more long lines

Co-authored-by: Parity Benchmarking Bot <[email protected]>

* */Cargo.toml: Remove unused dependencies (paritytech#7590)

* */Cargo.toml: Remove unused dependencies

Using cargo-udeps to detect unused dependencies.

* client/network/Cargo: Revert dependency removal

* Cargo.lock: Update

Co-authored-by: Andrew Plaza <[email protected]>
Co-authored-by: Qinxuan Chen <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Pierre Krieger <[email protected]>
Co-authored-by: Tomasz Drwięga <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Co-authored-by: Alexander Theißen <[email protected]>
Co-authored-by: Max Inden <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Wei Tang <[email protected]>
Co-authored-by: André Silva <[email protected]>
Co-authored-by: Parity Benchmarking Bot <[email protected]>
darkfriend77 pushed a commit to mogwaicoin/substrate that referenced this pull request Jan 11, 2021
* Build every wasm crate in its own project with wasm-builder

Building all wasm crates in one workspace was a nice idea, however it
just introduced problems:

1. We needed to prune old members, but this didn't worked for old git
deps.
2. We locked the whole wasm workspace while building one crate. This
could lead to infinitely locking the workspace on a crash.

Now we just build every crate in its own project, this means we will
build the dependencies multiple times. While building the dependencies
multiple times, we still decrease the build time by around 30 seconds
for Polkadot and Substrate because of the new parallelism ;)

* Remove the requirement on wasm-builder-runner

This removes the requirement on wasm-builder-runner by using the new
`build_dep` feature of cargo. We use nightly anyway and that enables us
to use this feature. This solves the problem of not mixing
build/proc-macro deps with normal deps. By doing this we get rid off
this complicated project structure and can depend directly on
`wasm-builder`. This also removes all the code from wasm-builder-runner
and mentions that it is deprecated.

* Copy the `Cargo.lock` to the correct folder

* Remove wasm-builder-runner

* Update docs

* Fix deterministic check

Modified-by: Bastian Köcher <[email protected]>

* Try to make the ui test happy

* Switch to `SKIP_WASM_BUILD`

* Rename `SKIP_WASM_BINARY` to the correct name...

* Update utils/wasm-builder/src/builder.rs

Co-authored-by: André Silva <[email protected]>

* Update utils/wasm-builder/src/builder.rs

Co-authored-by: André Silva <[email protected]>

Co-authored-by: André Silva <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasm-builder stalls build Collator build fails on the first run
5 participants