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

Generate weights for runtime #2442

Merged
merged 20 commits into from
Aug 29, 2023
Merged

Generate weights for runtime #2442

merged 20 commits into from
Aug 29, 2023

Conversation

nbaztec
Copy link
Contributor

@nbaztec nbaztec commented Aug 18, 2023

What does it do?

Generates weights for the runtime. Uses the ./scripts/run-benches-for-runtime.sh adapated from polkadot's. Each pallet has its own weight file that contains a generated trait WeightInfo as per the included template. Upon running the run-benches-for-runtime.sh the default template is used which merely implements the WeightInfo from the respective pallet's own weights.rs module, with the values for the current runtime.

The current PR uses weights generated by the benchmarking server, so is production ready.


⚠️ Breaking Changes ⚠️

Reference hardware for collators

Benchmarks introduced in this PR were run over a c6i.4xlarge AWS instance. It is recommended that the hardware used to run a collator matches the following specs in order to ensure the proper block production in time, as well as to prevent other performance issues.

CPU

  • x86-64 compatible.
  • Intel Ice Lake, or newer (Xeon or Core series); AMD Zen3, or newer (EPYC or Ryzen).
  • 4 physical cores @ 3.4GHz.
  • Simultaneous multithreading disabled (Hyper-Threading on Intel, SMT on AMD)
  • Prefer single-threaded performance over higher cores count. A comparison of single-threaded performance can be found here.

Storage

  • An NVMe SSD of 1 TB (As it should be reasonably sized to deal with blockchain growth). In general, the latency is more important than the throughput.

Memory

  • 32 GB DDR4 ECC.

System

  • Linux Kernel 5.16 or newer.

Network

  • The minimum symmetric networking speed is set to 500 Mbit/s (= 62.5 MB/s).

What important points reviewers should know?

  • Currently all weights are generated into runtime-common, however that can be easily changed if needed.
  • moonbeam-xcm-benchmarks diverges on how polkadot uses the generated weights - namely this runtime/weights/mod.rs file implements what we're currently doing in the moonbeam-xcm-benchmarks pallet, this ends up causing a circular dependency on runtime-common so the xcm weights are currently left untouched. All other pallets are migrated.

Is there something left for follow-up PRs?

Figure out how to use XCM weights to be used from runtime-common isntead.

Are there relevant PRs or issues in other repositories (Substrate, Polkadot, Frontier, Cumulus)?

Fix benchmarks in frontier: polkadot-evm/frontier#1162
Currently we're using a patched substrate on our fork moonbeam-foundation/substrate#67 since the ED = 0 constraint caused these benchmarks to fail. We should probably copy these benchmarks in moonbeam and maintain them going forward.
Other repos that were fixed:
Moonsong-Labs/moonkit#4
moonbeam-foundation/crowdloan-rewards#69

@nbaztec nbaztec added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited breaking Needs to be mentioned in breaking changes labels Aug 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Aug 18, 2023

Coverage generated "Mon Aug 28 18:35:26 UTC 2023":
https://d3ifz9vhxc2wtb.cloudfront.net/pulls/2442/html/index.html

Master coverage: 87.39%
Pull coverage:

@librelois librelois mentioned this pull request Aug 28, 2023
28 tasks
Comment on lines +406 to +407
// Minimum execution time: 18_446_744_073_709_551_000 picoseconds.
Weight::from_parts(500_000_000_000, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm guessing 500_000_000_000 was hard coded, but why the huge Minimum execution time number?

Comment on lines -199 to +201
Weight::from_parts(70_805_524, 26879)
// Standard Error: 8_036
.saturating_add(Weight::from_parts(84_366, 0).saturating_mul(x.into()))
.saturating_add(T::DbWeight::get().reads(6_u64))
// Measured: `1421 + x * (38 ±0)`
// Estimated: `4752 + x * (41 ±0)`
// Minimum execution time: 75_412_000 picoseconds.
Weight::from_parts(85_543_270, 4752)
// Standard Error: 2_633
.saturating_add(Weight::from_parts(213_586, 0).saturating_mul(x.into()))
.saturating_add(T::DbWeight::get().reads(7_u64))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty big increase, is it expected?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, did these staking ones change because they're generated by a proper runtime instead of a mock runtime?

Copy link
Contributor

Choose a reason for hiding this comment

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

...actually, why is staking the only one where we touched the pallet-level weights? They went up for the most part, which is probably "safe." It's used by other projects which may not generate their own weights, so maybe that's a good thing 🤷

Copy link
Collaborator

Choose a reason for hiding this comment

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

The weights of some pallets were relying on substrate part that were not correctly configured.
I think the goal is to get default values for external team, but still generate weights for our runtimes once ready

Copy link
Collaborator

Choose a reason for hiding this comment

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

So yes we want to provide weight by default on pallets, and then a weight for our runtimes

Copy link
Contributor

@notlesh notlesh left a comment

Choose a reason for hiding this comment

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

LGTM. I spot-checked a few of the results and they look consistently lower, which is to be expected with improved hardware.

The exception is parachain-staking, I don't have an opinion on whether we want those changes or not, so maybe just ensure it's intentional.

@crystalin
Copy link
Collaborator

parachain-staking is the one of the reasons we changed those weights, like auto-compound (which also will have a fix separately).
We also changed things relying on the scheduler and other elements which are configured by default to be very high in substrate

@crystalin
Copy link
Collaborator

Do we have a tool that would display all the weight changes per function ?

@nbaztec nbaztec merged commit d0ae4a5 into master Aug 29, 2023
@nbaztec nbaztec deleted the nish-regen-all-weights branch August 29, 2023 08:44
@noandrea noandrea changed the title [MOON-2531] Generate weights for runtime Generate weights for runtime Aug 30, 2023
@crystalin crystalin mentioned this pull request Sep 1, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D9-needsaudit👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants