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

Optimized weights #10692

Merged
merged 6 commits into from
Jan 24, 2022
Merged

Optimized weights #10692

merged 6 commits into from
Jan 24, 2022

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 18, 2022

Re-run all benchmarks on reference hardware with optimization flags enabled #10608.
Mostly all weights decreased as you can see in the diff. The few cases where not need some explanation.

👉 Good examples for improvement: pallet_timpestamp, utility.batch

Next Steps

Verification

Someone would have to do the same thing that I did and thereby verify that I did not just make the values up 🤣

✔️ Missing pallets

  • 0. I just noticed that some weight.rs files were not touched; will rerun for those pallets.
  • 1. babe, grandpa and mmr seem to not use automatic benchmarking. They have a default_weights.rs instead. How do I update that? -> follow up
  • 2. offences' benchmark is currently broken and weights are not updated broken benchmark for pallet_offences (report_offence_im_online) #10027

✔️ Weird cases

The weights of substrate itself are not used anyway. They just act as sanity-check.
All really weird changes have been addressed.

Explain the cases where the weight did not just decrease:

  • 1. Baseline.Hashing is now ignoring its i parameter. Probably the same as frame/benchmarking: baseline weights independent of complexity params polkadot-sdk#400
  • 2. Contracts.{on_initialize_per_trie_key, seal_clear_storage, seal_contains_storage, instr_i64clz}
  • 3. Democracy.on_initialize_base + _with_launch_period got more DB writes?!
  • 4. Democracy.unlock_remove slightly more linear weight but less base weight
  • 5. Proxy.remove_announcement now uses its formerly ignored p param
  • 6. Proxy.anonymous linear weight increase but less base
  • 7. Scheduler.{on_initialize_periodic_resolved, on_initialize_periodic_named_resolved, schedule, schedule_named} either base or linear increased but the other decreased
  • 8. Staking.rebond more linear but less base
  • 9. Tips.{close_tip, tip, tip_new} higher linear but lower base
  • 10. Tips.slash_tip MUCH higher linear but lower base
  • 11. Trasury.approve_proposal higher linear but lower base
  • 12. Vesting.vested_transfer higher s linear but lower base and l
  • 13. System.remark went to zero and ignores its param now

In the best case someone just forgot to re-run the benchmarks.

✔️ Cargo.toml

production profile was created.
The optimizations increase the compile time by a lot.
Should they be part of release or go in the separate optimized profile?
target-cpu=native needs to be configured via env variables in both cases.

✔️ Bench bot

... would need to be updated. paritytech/bench-bot#79

✔️ Fix problem in node runtime

Done
The following code in bin/node/runtime/src/lib.rs now panics with div by zero:

pub DeletionQueueDepth: u32 = ((DeletionWeightLimit::get() / (
	<Runtime as pallet_contracts::Config>::WeightInfo::on_initialize_per_queue_item(1) -
	<Runtime as pallet_contracts::Config>::WeightInfo::on_initialize_per_queue_item(0)
)) / 5) as u32;

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jan 18, 2022
@ggwpez ggwpez added B7-runtimenoteworthy C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 18, 2022
Cargo.toml Show resolved Hide resolved
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review January 19, 2022 17:16
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jan 19, 2022
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Contract weights look good to me. As for the code snippet you posted which is suppose to panic with a div by zero: When looking at the weights it does seem that:

on_initialize_per_queue_item(1) - on_initialize_per_queue_item(0) != 0

Not sure why it panics.

frame/contracts/src/weights.rs Show resolved Hide resolved
frame/contracts/src/weights.rs Show resolved Hide resolved
frame/contracts/src/weights.rs Show resolved Hide resolved
frame/contracts/src/weights.rs Show resolved Hide resolved
@ggwpez
Copy link
Member Author

ggwpez commented Jan 20, 2022

Contract weights look good to me. As for the code snippet you posted which is suppose to panic with a div by zero: When looking at the weights it does seem that:

on_initialize_per_queue_item(1) - on_initialize_per_queue_item(0) != 0

Not sure why it panics.

Okay thanks. I looked at the CI output and it seems to run now 👍

the weight of System.remark went to 0, the collective test uses a
remark call to trigger an out-of-gas condition so I replaced it
with a `remark_with_event` call.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

awesome 👍

Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I did not check the weights, but I would rename the profile.

Cargo.toml Show resolved Hide resolved
Copy link
Member

@athei athei left a comment

Choose a reason for hiding this comment

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

Nice! Would be awesome to see a followup where the same is done for the runtime (after the bot is also changed).

@ggwpez
Copy link
Member Author

ggwpez commented Jan 24, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 542a858 into master Jan 24, 2022
@paritytech-processbot paritytech-processbot bot deleted the oty-optimized-weights branch January 24, 2022 12:35
eskimor pushed a commit that referenced this pull request Jan 27, 2022
* Add optimization flags to 'release' profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Optimized weights

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add missing pallets

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add `production` profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* pallet-collective: fix tests

the weight of System.remark went to 0, the collective test uses a
remark call to trigger an out-of-gas condition so I replaced it
with a `remark_with_event` call.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Wizdave97 pushed a commit to ComposableFi/substrate that referenced this pull request Feb 4, 2022
* Add optimization flags to 'release' profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Optimized weights

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add missing pallets

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add `production` profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* pallet-collective: fix tests

the weight of System.remark went to 0, the collective test uses a
remark call to trigger an out-of-gas condition so I replaced it
with a `remark_with_event` call.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
* Add optimization flags to 'release' profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Optimized weights

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add missing pallets

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add `production` profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* pallet-collective: fix tests

the weight of System.remark went to 0, the collective test uses a
remark call to trigger an out-of-gas condition so I replaced it
with a `remark_with_event` call.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* Add optimization flags to 'release' profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Optimized weights

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add missing pallets

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* Add `production` profile

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>

* pallet-collective: fix tests

the weight of System.remark went to 0, the collective test uses a
remark call to trigger an out-of-gas condition so I replaced it
with a `remark_with_event` call.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
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. C3-medium PR touches the given topic and has a medium impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants