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

update weights #6025

Closed
wants to merge 9 commits into from
Closed

update weights #6025

wants to merge 9 commits into from

Conversation

coderobe
Copy link
Contributor

@coderobe coderobe commented Sep 19, 2022

new weights as requested by @shawntabrizi

@github-actions github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 19, 2022
@coderobe coderobe added 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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit. labels Sep 19, 2022
@coderobe
Copy link
Contributor Author

this was run off d17f062

Comment on lines +49 to +53
Weight::from_ref_time(105_000 as u64)
}
/// The range of component `i` is `[0, 1000000]`.
fn subtraction(_i: u32, ) -> Weight {
Weight::from_ref_time(125_000 as u64)
Weight::from_ref_time(112_000 as u64)
Copy link
Member

@shawntabrizi shawntabrizi Sep 20, 2022

Choose a reason for hiding this comment

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

@koute @ggwpez strange to me that these end in 000?

What are the chances if the precision is down to a single unit

Copy link
Member

Choose a reason for hiding this comment

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

I think the increased precision only applied to the linear component, or @koute ?

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, the increased precision is only for the component weights, as that's where it can make a difference.

Comment on lines +51 to +65
Weight::from_ref_time(5_800_910_000 as u64)
// Standard Error: 141_466
.saturating_add(Weight::from_ref_time(5_793_269 as u64).saturating_mul(v as u64))
// Standard Error: 14_463_035
.saturating_add(Weight::from_ref_time(1_592_527_344 as u64).saturating_mul(d as u64))
}
/// The range of component `v` is `[1000, 2000]`.
/// The range of component `t` is `[500, 1000]`.
/// The range of component `d` is `[5, 16]`.
fn phragmms(v: u32, _t: u32, d: u32, ) -> Weight {
Weight::from_ref_time(0 as u64)
// Standard Error: 79_000
.saturating_add(Weight::from_ref_time(14_480_000 as u64).saturating_mul(v as u64))
// Standard Error: 6_844_000
.saturating_add(Weight::from_ref_time(2_525_332_000 as u64).saturating_mul(d as u64))
Weight::from_ref_time(4_696_655_000 as u64)
// Standard Error: 150_968
.saturating_add(Weight::from_ref_time(5_769_927 as u64).saturating_mul(v as u64))
// Standard Error: 15_434_495
.saturating_add(Weight::from_ref_time(1_820_652_967 as u64).saturating_mul(d as u64))
Copy link
Member

Choose a reason for hiding this comment

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

big changes here

Copy link
Member

Choose a reason for hiding this comment

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

The worst-case improved for both runtimes, rococo is unchanged.:
image

Copy link
Contributor

Choose a reason for hiding this comment

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

It does look like this weight was affected by the zero base weight issue that my PR fixed; looks correct to me. As @ggwpez pointed out this actually lowered the weight final weight since the negative base weight isn't rounded up to zero anymore (so we were overestimating how costly this extrinsic was when the components are not zero)

@shawntabrizi
Copy link
Member

/cmd queue -v RUST_LOG=debug -c bench-bot $ runtime kusama-dev pallet_balances

@command-bot
Copy link

command-bot bot commented Sep 20, 2022

@shawntabrizi https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1864571 was started for your command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" runtime kusama-dev pallet_balances. Check out https://gitlab.parity.io/parity/mirrors/polkadot/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment /cmd cancel 41-68d77c72-0436-41d4-b2c1-c58b40841f82 to cancel this command or /cmd cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Sep 20, 2022

@shawntabrizi Command "$PIPELINE_SCRIPTS_DIR/bench-bot.sh" runtime kusama-dev pallet_balances has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1864571 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot/-/jobs/1864571/artifacts/download.

@ggwpez
Copy link
Member

ggwpez commented Sep 20, 2022

Interesting. There are some big increases, but also a lot of decreases.
Remark jumped to 1.46ms, but it seems consistent across the runtimes. Westend was not updated @coderobe?

SWC-compare link link and PDF for posterity.

@coderobe
Copy link
Contributor Author

westend wasn't ready when i opened the PR - it is now, i'll add the files.

Comment on lines +63 to +65
.saturating_add(T::DbWeight::get().reads(3 as u64))
.saturating_add(T::DbWeight::get().reads((1 as u64).saturating_mul(p as u64)))
.saturating_add(T::DbWeight::get().writes(2 as u64))
.saturating_add(T::DbWeight::get().writes(3 as u64))
Copy link
Member

Choose a reason for hiding this comment

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

changes

Copy link
Member

Choose a reason for hiding this comment

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

So the weights were outdated somehow

@coderobe coderobe marked this pull request as ready for review September 20, 2022 21:12
@coderobe coderobe 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 Sep 20, 2022
@shawntabrizi
Copy link
Member

shawntabrizi commented Sep 20, 2022

There are some changes here we should look into, but i feel pretty confident after looking over this, that the changes that @koute made are solid, and probably we need to hunt separately what is going on

Thanks @coderobe

@coderobe
Copy link
Contributor Author

doesn't compile, however

@shawntabrizi
Copy link
Member

Looks like an error in the template, should be easy to resolve, will look into it

@ggwpez
Copy link
Member

ggwpez commented Sep 20, 2022

Looks like an error in the template, should be easy to resolve, will look into it

Could also just be missing deps in runtime/kusama/constants/Cargo.toml.
PS I think we can and should get rid of that sub-crate and just put it into the runtime.

@ggwpez
Copy link
Member

ggwpez commented Sep 27, 2022

Regarding the merge conflicts: its since we renamed anonymous to pure. I will do a git merge -X theirs origin/master.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

The large changes are most likely from the recent weight calculation changes. We are investigating them in Substrate.
Should be more precise now 😄

@@ -1,28 +1,27 @@
// This file is part of Substrate.
// Copyright 2017-2022 Parity Technologies (UK) Ltd.
Copy link
Member

Choose a reason for hiding this comment

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

I added the correct file header manually. All future MRs should have them automatically #5984

@coderobe coderobe mentioned this pull request Sep 30, 2022
@coderobe coderobe closed this Jan 16, 2023
@coderobe coderobe deleted the coderobe/weights-st branch January 16, 2023 14:42
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. 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.

4 participants