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

WeightInfo for Multisig Pallet #7154

Merged
15 commits merged into from
Sep 21, 2020
Merged

WeightInfo for Multisig Pallet #7154

15 commits merged into from
Sep 21, 2020

Conversation

shawntabrizi
Copy link
Member

@shawntabrizi shawntabrizi commented Sep 21, 2020 •
edited
Loading

This introduces the automated benchmarking freamwork to the Multisig Pallet.

All extrinsic weights are now represented by a configurable WeightInfo.

as_multi_threshold_1

Before

34_000_000 + (2_000 * call_len) + call_weight

After

14_749_000 + (1_000 * len) + call_weight

as_multi_create

Before

55_000_000 + (250_000 * sig_len) + (3_000 * call_len) + 2 Read + 1 Write

After

74_556_000 + (135_000  * s ) + (1_000  * z) + 2 Read + 1 Write

as_multi_create_store

Before

55_000_000 + (250_000 * sig_len) + (3_000 * call_len) + 2 Read + 1 Write + 1 Write (store)

Missing contains_key read, so the new weight is correct

After

90_218_000 + (129_000  * s ) + (3_000  * z) + 3 Read + 2 Write

as_multi_approve

Before

55_000_000 + (250_000 * sig_len) + (3_000 * call_len) + 2 Read + 1 Write

Overweight by 1 read?

confirmed by https://www.shawntabrizi.com/substrate-graph-benchmarks/db-stats.html?p=multisig&e=as_multi_approve

After

48_402_000 + (132_000 * sig_len) + (1_000 * call_len) + 1 Read + 1 Write

cancel_as_multi

Old weight is overweight by 1 read and write due to not whitelisting

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Sep 21, 2020
@shawntabrizi shawntabrizi mentioned this pull request Sep 21, 2020
Closed
23 tasks
@shawntabrizi shawntabrizi 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. labels Sep 21, 2020
apopiak
apopiak approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

LGTM, though the weight reductions are somewhat surprising to me (might be that I missed some changes to benchmarks?).

frame/multisig/src/benchmarking.rs Outdated Show resolved Hide resolved
@apopiak
Copy link
Contributor

apopiak commented Sep 21, 2020

needs a companion, no?

@shawntabrizi
Copy link
Member Author

shawntabrizi commented Sep 21, 2020 •
edited
Loading

@apopiak i will make a larger companion with multiple weight files. Since this doesnt break the API, we should be good to go.

As for reduction in weights, its hard to say exactly what caused it, but I can say that I am getting consistent results with the current substrate, and i see no reason to think the new weights are not accurate.

gui1117
gui1117 approved these changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

PR is good to me

frame/multisig/src/lib.rs
.max(T::WeightInfo::approve_as_multi_approve(s))
.max(T::WeightInfo::approve_as_multi_complete(s))
.saturating_add(*max_weight)
}]
Copy link
Contributor

Choose a reason for hiding this comment

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

here the call is still decoded in some situation (like if it gets complete), thus decoding the code is still O(call_len) but anyway it should be small (like we don't hash it here). And it hasn't been introduced by this PR.

@kianenigma
Copy link
Contributor

Ain't the weight of as_multi_threshold_1 dropped by half for no reason?

@shawntabrizi
Copy link
Member Author

@kianenigma but results are consistent on the benchmark machine. I have nothing else to really go off of.

@gavofyork
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Sep 21, 2020

Trying merge.

@ghost ghost merged commit f778556 into master Sep 21, 2020
@ghost ghost deleted the shawntabrizi-multisig-weightinfo branch September 21, 2020 14:39
rakanalh pushed a commit to rakanalh/substrate that referenced this pull request Sep 22, 2020
@shawntabrizi @apopiak
WeightInfo for Multisig Pallet (paritytech#7154)
cd2f367 
* as multi threshold 1

* add `as_multi_approve_store` benchmark

* finish update

* final weights

* integrate into runtime

* whitelist accounts

* whitelisted caller weights

* clean up comments

* Get up to date `call_len`

* better implementation

* fix spacing

* spacing

* Update frame/multisig/src/benchmarking.rs

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>

Co-authored-by: Alexander Popiak <alexander.popiak@parity.io>
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@gui1117 gui1117 gui1117 approved these changes

@apopiak apopiak apopiak approved these changes

@kianenigma kianenigma kianenigma approved these changes

Assignees
No one assigned
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@shawntabrizi @apopiak @kianenigma @gavofyork @gui1117 @paritytech-ci