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

Update treasury weights. #5723

Merged
merged 16 commits into from
Apr 28, 2020
Merged

Update treasury weights. #5723

merged 16 commits into from
Apr 28, 2020

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Apr 21, 2020

Also fix: #5706

polkadot PR: paritytech/polkadot#1044

@gui1117 gui1117 added the A0-please_review Pull request needs code review. label Apr 21, 2020
@gui1117 gui1117 requested review from shawntabrizi and apopiak April 21, 2020 13:47
@gui1117 gui1117 added this to the Polkadot milestone Apr 21, 2020
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
/// # </weight>
#[weight = SimpleDispatchInfo::FixedNormal(50_000_000)]
fn close_tip(origin, hash: T::Hash) {
#[weight = 211_000_000 + 1_094_000 * MAX_TIPPERS_COUNT + T::DbWeight::get().reads_writes(3, 3)]
Copy link
Member

Choose a reason for hiding this comment

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

I liked your suggestion to include the raw data with the weight paragraph. I also think it was suggested we choose "round" numbers for weights, but maybe we should double check with others.

I would write a weight of:

 #[weight = 200_000_000 + 1_000_000 * MAX_TIPPERS_COUNT + T::DbWeight::get().reads_writes(3, 3)] 

If I was doing this.

Copy link
Contributor Author

@gui1117 gui1117 Apr 22, 2020

Choose a reason for hiding this comment

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

it improves readibility but rounding 211 to 200 is 5% and 1_094 to 1_000 is 10%. I feel like its fine to just put the direct value with useless precision.

I rounded a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept 2 significative digits for each and rounded up to get them
https://en.wikipedia.org/wiki/Significant_figures

@gui1117 gui1117 added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Apr 21, 2020
@gui1117 gui1117 requested a review from kianenigma as a code owner April 22, 2020 08:41
@gui1117 gui1117 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 Apr 22, 2020
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
frame/support/src/traits.rs Show resolved Hide resolved
frame/support/src/traits.rs Outdated Show resolved Hide resolved
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
frame/treasury/src/lib.rs Outdated Show resolved Hide resolved
frame/elections-phragmen/src/lib.rs Outdated Show resolved Hide resolved
/// # </weight>
#[weight = 500_000_000]
#[weight = 114_700_000 + T::DbWeight::get().reads_writes(1, 2)]
Copy link
Contributor

Choose a reason for hiding this comment

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

so can you bring me up to speed, where are these numbers coming from? the 114_700_000 per se.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • to me it seems like we ignore sender account all the time here. Correct? Also best to call it origin in the doc as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this number directly comes from the benchmark server at https://www.shawntabrizi.com/substrate-graph-benchmarks/?p=treasury&e=tip_new

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 22, 2020

for all coefficient I kept 2 digits by rounding up.

@apopiak
Copy link
Contributor

apopiak commented Apr 22, 2020

for all coefficient I kept 2 digits by rounding up.

Just want to note that I saw changes of up to 30% in benchmark time component estimations between runs on Monday and Tuesday. It mostly seemed to shift between constant and parametrized components, but it still lead to me being more conservative (i.e. choosing higher) with my values.

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 23, 2020

for all coefficient I kept 2 digits by rounding up.

Just want to note that I saw changes of up to 30% in benchmark time component estimations between runs on Monday and Tuesday. It mostly seemed to shift between constant and parametrized components, but it still lead to me being more conservative (i.e. choosing higher) with my values.

oh ok, that is a bit surprising to me though, the machine wasn't use for anything else ?

EDIT: we could keep indeed only one or one with 0.5 digits but this sounds very unprecise like 1.1->1.5 is 36% change.

@shawntabrizi
Copy link
Member

for all coefficient I kept 2 digits by rounding up.

Just want to note that I saw changes of up to 30% in benchmark time component estimations between runs on Monday and Tuesday. It mostly seemed to shift between constant and parametrized components, but it still lead to me being more conservative (i.e. choosing higher) with my values.

oh ok, that is a bit surprising to me though, the machine wasn't use for anything else ?

EDIT: we could keep indeed only one or one with 0.5 digits but this sounds very unprecise like 1.1->1.5 is 36% change.

The error comes from the analysis I think more than the individual runs. When there is no attribution of the weight to other components, it seems the results are quite stable.

Although, this has not been tested thoroughly.

@gavofyork
Copy link
Member

polkadot companion still not building?

@gavofyork gavofyork added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 27, 2020
@gui1117
Copy link
Contributor Author

gui1117 commented Apr 28, 2020

yes done

@gui1117
Copy link
Contributor Author

gui1117 commented Apr 28, 2020

Flag has been changed to merge-once-green but actually this still lack an approve flag @gavofyork

@gavofyork gavofyork merged commit 9441e44 into master Apr 28, 2020
@gavofyork gavofyork deleted the gui-treasury-weight branch April 28, 2020 15:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contains trait seems missused and with outdated documentation.
6 participants