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

Small optimisation to --profile dev wasm builds #1851

Merged
merged 17 commits into from
Oct 25, 2023

Conversation

liamaharon
Copy link
Contributor

@liamaharon liamaharon commented Oct 11, 2023

wasm-builder was adjusted to default to building wasm blobs in release mode even when cargo is in debug because debug wasm is too slow.

A side effect of this was .compact and .compact.compressed getting built when the dev is running build in debug, adding ~5s to the build time of every wasm runtime.

I think it's reasonable to assume if the dev is running debug build they want to optimise speed and do not care about the size of the wasm binary. Compacting a blob has negligible impact on its actual performance.

In this PR, I adjusted the behavior of the wasm builder so it does not produce .compact or .compact.compressed wasm when the user is running in debug. The builder will continue to produce the bloaty wasm in release mode unless it is overriden with an env var.

As suggested by @koute in review, also refactored the maybe_compact_wasm_and_copy_blobs into multiple funuctions, and renamed things to better support RISC-V in the future.


There is no T-runtime label so @KiChjang told me to put T1-FRAME :)

@liamaharon liamaharon added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Oct 11, 2023
@liamaharon liamaharon requested a review from koute as a code owner October 11, 2023 12:52
@liamaharon liamaharon requested a review from koute October 24, 2023 01:29
@liamaharon liamaharon merged commit ff3a3bc into master Oct 25, 2023
8 checks passed
@liamaharon liamaharon deleted the liam-debug-wasm-optimisation branch October 25, 2023 00:02
ordian added a commit that referenced this pull request Oct 26, 2023
* master:
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
ordian added a commit that referenced this pull request Oct 26, 2023
* tsv-disabling:
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
ordian added a commit that referenced this pull request Oct 26, 2023
* tsv-disabling: (36 commits)
  Removed TODO from test-case for hard-coded delivery fee estimation (#2042)
  Expose collection attributes from `Inspect` trait (#1914)
  `polkadot-parachain-primitives` should not depend on `frame-support`. (#1897)
  [testnet] Align testnet system parachain runtimes using `RelayTreasuryLocation` and `SystemParachains` in the same way (#2023)
  Sort the benchmarks before listing them (#2026)
  publish pallet-root-testing (#2017)
  Contracts: Add benchmarks to include files (#2022)
  Small optimisation to `--profile dev` wasm builds (#1851)
  basic-authorship: Improve time recording and logging (#2010)
  Application Crypto and BEEFY Support for paired (ECDSA,BLS) crypto (#1815)
  [ci] Run check-rust-feature-propagation in pr and master (#2012)
  Improve features dev-ex (#1831)
  Remove obsolete comment. (#2008)
  Refactor candidates test in paras_inherent (#2004)
  PVF: Add worker check during tests and benches (#1771)
  Bump actions/setup-node from 3.8.1 to 4.0.0 (#1997)
  polkadot: enable tikv-jemallocator/unprefixed_malloc_on_supported_platforms (#2002)
  Make `IdentityInfo` generic in `pallet-identity` (#1661)
  Ensure correct variant count in `Runtime[Hold/Freeze]Reason` (#1900)
  `CheckWeight`: Add more logging (#1996)
  ...
s0me0ne-unkn0wn pushed a commit that referenced this pull request Oct 29, 2023
`wasm-builder` was adjusted to default to building wasm blobs in
`release` mode even when cargo is in `debug` because `debug` wasm is too
slow.

A side effect of this was `.compact` and `.compact.compressed` getting
built when the dev is running build in `debug`, adding ~5s to the build
time of every wasm runtime.

I think it's reasonable to assume if the dev is running `debug` build
they want to optimise speed and do not care about the size of the wasm
binary. Compacting a blob has negligible impact on its actual
performance.

In this PR, I adjusted the behavior of the wasm builder so it does not
produce `.compact` or `.compact.compressed` wasm when the user is
running in `debug`. The builder will continue to produce the bloaty wasm
in release mode unless it is overriden with an env var.

As suggested by @koute in review, also refactored the
`maybe_compact_wasm_and_copy_blobs` into multiple funuctions, and
renamed things to better support RISC-V in the future.

---

There is no `T-runtime` label so @KiChjang told me to put `T1-FRAME` :)

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
github-merge-queue bot pushed a commit that referenced this pull request Feb 22, 2024
This PR fixes a subtle bug in `wasm-builder` first introduced in
#1851 (sorry, my bad! I
should have caught this during review) where the status code of the
`cargo` subprocess is not properly checked, which results in builds
silently succeeding when they shouldn't (that is: if we successfully
build a runtime blob, and then modify the code so that it won't compile,
and recompile it again, then the build will succeed and silently use the
*old* blob).

cc @athei This is the bug you were seeing.

[edit]Also fixes a similar PolkaVM-specific bug where I accidentally
used the wrong comparison operator.[/edit]
girazoki pushed a commit to moondance-labs/polkadot-sdk that referenced this pull request Feb 22, 2024
This PR fixes a subtle bug in `wasm-builder` first introduced in
paritytech#1851 (sorry, my bad! I
should have caught this during review) where the status code of the
`cargo` subprocess is not properly checked, which results in builds
silently succeeding when they shouldn't (that is: if we successfully
build a runtime blob, and then modify the code so that it won't compile,
and recompile it again, then the build will succeed and silently use the
*old* blob).

cc @athei This is the bug you were seeing.

[edit]Also fixes a similar PolkaVM-specific bug where I accidentally
used the wrong comparison operator.[/edit]
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
`wasm-builder` was adjusted to default to building wasm blobs in
`release` mode even when cargo is in `debug` because `debug` wasm is too
slow.

A side effect of this was `.compact` and `.compact.compressed` getting
built when the dev is running build in `debug`, adding ~5s to the build
time of every wasm runtime.

I think it's reasonable to assume if the dev is running `debug` build
they want to optimise speed and do not care about the size of the wasm
binary. Compacting a blob has negligible impact on its actual
performance.

In this PR, I adjusted the behavior of the wasm builder so it does not
produce `.compact` or `.compact.compressed` wasm when the user is
running in `debug`. The builder will continue to produce the bloaty wasm
in release mode unless it is overriden with an env var.

As suggested by @koute in review, also refactored the
`maybe_compact_wasm_and_copy_blobs` into multiple funuctions, and
renamed things to better support RISC-V in the future.

---

There is no `T-runtime` label so @KiChjang told me to put `T1-FRAME` :)

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
This PR fixes a subtle bug in `wasm-builder` first introduced in
paritytech#1851 (sorry, my bad! I
should have caught this during review) where the status code of the
`cargo` subprocess is not properly checked, which results in builds
silently succeeding when they shouldn't (that is: if we successfully
build a runtime blob, and then modify the code so that it won't compile,
and recompile it again, then the build will succeed and silently use the
*old* blob).

cc @athei This is the bug you were seeing.

[edit]Also fixes a similar PolkaVM-specific bug where I accidentally
used the wrong comparison operator.[/edit]
bkchr pushed a commit that referenced this pull request Apr 10, 2024
…1851)

* fix `HeadersToKeep` and `MaxBridgedAuthorities` in Millau benchmarks

* typo

* impl review suggestion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants