Calculate and refund weight for identity pallet by apopiak · Pull Request #5680 · paritytech/substrate

apopiak · 2020-04-17T10:44:35Z

This PR implements weight calculation and refund for the dispatchables in pallet-identity.

Changes:

add more sophisticated weight calculation to all the dispatchables and refunds to some of them
fix set_subs benchmark (to contain 2 parameters).
limit registrars in add_registrar to new MaxRegistrars (new error TooManyRegistrars)

API Notes:

No notable changes to the dispatchables API (only the return value changed to DispatchResultWithPostInfo)

Drive-by Changes:

some small docs adjustments (for benchmarking)
convert an addition to saturating_add because it was failing a test

Related to #5596

parity-cla-bot · 2020-04-17T10:44:39Z

It looks like @apopiak signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

frame/identity/src/lib.rs

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

…est_judgement

apopiak · 2020-04-23T14:26:02Z

Does the PR need a Polkadot companion if it doesn't change the parameters and only the return type of the Dispatchables?

…-weight-params

apopiak · 2020-04-23T15:13:34Z

Never mind, I need to add MaxRegistrars of course 🤦

shawntabrizi · 2020-04-23T19:23:30Z

frame/benchmarking/src/lib.rs

 /// (or not) by each arm. Syntax is available to allow for only the range to be drawn upon if
 /// desired, allowing an alternative instancing expression to be given.
 ///
+/// Note that the ranges are *inclusive* on both sides. This is in contrast to ranges in Rust which


As a follow up, we can just change the macro syntax to MIN ..= MAX, which I also think solve this syntax mismatch right?

shawntabrizi · 2020-04-23T19:40:00Z

frame/identity/src/lib.rs

+		extra_fields: impl Into<Weight>
+	) -> Weight {
+		db.reads_writes(1, 1)
+			+ db.reads_writes(1, 1) // balance ops


This should be excluded because balance read/write targets the System.Account storage item, which will already be updated this block because a user submitted an extrinsic, and their nonce/fee will be charged.

Same for the other balance operations in this PR. It would only apply if the user whose balance is changing is NOT the caller.

frame/identity/src/lib.rs

shawntabrizi · 2020-04-23T20:15:46Z

frame/identity/src/lib.rs

+		extra_fields: impl Into<Weight>
+	) -> Weight {
+		db.reads_writes(3, subs.into() + 3) // 2 `take`s + S deletions
+			+ db.reads_writes(1, 1) // balance ops


This case balance op is correct

But you seem to be missing a write to the treasury account (which I think you would skip anyway since treasury is written to every block, but for further analysis, would be good to double check why you might have missed treasury)

I should have noted that the I could not account for 1 read and 1 write in the first line.
You think that might be the treasury? Where does it come in? Through Slashed::on_unbalanced?

Yes, you can see the configuration in the Node runtime, but also you can see it in our DB logs

So the benchmark says there are 4 reads and subs + 4 writes.
I'm happy to subtract 1 for the balance op, but I don't know how to trace whether the treasury one is a duplicate. With "DB logs" do you mean the logs under the DB benchmark? How would I tell that it's the treasury? Is it a special address?

I have added treasury to our "whitelist" so it should show up now explicitly in the name and remove itself automatically from the read count.

The way I figured it out is that i used the DB logs and walked through the code to identify each read/write operation on the DB log with some code that was written.

I suggest you do the same

shawntabrizi · 2020-04-23T20:20:33Z

frame/identity/src/lib.rs

+		subs: impl Into<Weight> + Copy,
+		extra_fields: impl Into<Weight>
+	) -> Weight {
+		db.reads_writes(3, subs.into() + 3) // 2 `take`s + S deletions


This should be 2 instead of 3?

This might be the treasury ops you noted above?

Not sure, but this should be 2 for both read and write

yeah, you're right

frame/identity/src/lib.rs

shawntabrizi

Left some feedback, but everything else looks good here!

shawntabrizi

This looks right.

I guess my last worry would be any chances of overflow in the weight math.

Some of these values are directly controllable by the end user right?

shawntabrizi

Looks good!

* Companion PR for paritytech#5501 * Bump runtime versions * add MaxRegistrars config * Pull substrate master in PR-5501 * Attempt to fix pin commit again * update to substrate master Co-authored-by: Alexander Popiak <alexander.popiak@parity.io> Co-authored-by: André Silva <andre.beat@gmail.com>

add old_registrar_count as param to estimate weight

bd03eb7

paritytech-ci added the B2-breaksapi label Apr 17, 2020

shawntabrizi reviewed Apr 17, 2020

View reviewed changes

frame/identity/src/lib.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed Apr 17, 2020

View reviewed changes

frame/identity/src/lib.rs Outdated Show resolved Hide resolved

apopiak and others added 25 commits April 17, 2020 15:43

cast count to Weight

eeaf09f

Co-Authored-By: Shawn Tabrizi <shawntabrizi@gmail.com>

add weight calculation for set_identity

abf2bf2

remove superfluous weight comment

b4cb9de

add detailed weight estimation for set_subs

77e4568

adjust benchmarking code to the new API

3c6bb77

add second parameter to set_subs benchmark

6a64c7e

rename o to p

31ba72f

calculate weight based on benchmarks

d4feebe

Merge remote-tracking branch 'origin' into apopiak-weight-params

f34aad1

Merge remote-tracking branch 'origin/master' into apopiak-weight-params

dd023c1

use try_mutate for registrars

6d08bc1

fix weight number typo

fa04aab

update weights for set_subs + add weights for clear_identity and requ…

3fb9789

…est_judgement

improve naming and docs

9d027ba

add weight calculation for cancel_request

6c0dfa9

fix benchmark

d662f5a

fix tests

1316345

fix arithmetic overflow in balances triggered by tests

c699699

add weight calcluations for more dispatchables

269f50b

add weight calculation for provide_judgement

8dec341

mark param as unused

155f193

add MaxRegistrars associated type used for weight estimation

d6f68ff

check that MaxRegistrars is not exceeded

ba839d6

add remaining weight calculations

4719cc5

Merge branch 'master' into apopiak-weight-params

05343cb

fix inconsistencies before review

1072ff8

apopiak requested a review from gui1117 April 23, 2020 13:19

apopiak marked this pull request as ready for review April 23, 2020 14:25

apopiak requested a review from kianenigma as a code owner April 23, 2020 14:25

Merge branch 'master' of github.com:paritytech/substrate into apopiak…

e4724da

…-weight-params

apopiak changed the title ~~Improve weight estimation for identity pallet~~ Calculate and refund weight for identity pallet Apr 23, 2020

apopiak mentioned this pull request Apr 23, 2020

Document and Calculate Weights for Pallets #5596

Closed

apopiak mentioned this pull request Apr 23, 2020

Companion PR to Substrate #5680 paritytech/polkadot#1028

Closed

fix actual weight calculation for add_registrar

98718d9

apopiak added the A0-please_review Pull request needs code review. label Apr 23, 2020

apopiak requested a review from shawntabrizi April 23, 2020 16:31

shawntabrizi reviewed Apr 23, 2020

View reviewed changes

frame/identity/src/lib.rs Outdated Show resolved Hide resolved

shawntabrizi reviewed Apr 23, 2020

View reviewed changes

frame/identity/src/lib.rs Show resolved Hide resolved

shawntabrizi suggested changes Apr 23, 2020

View reviewed changes

remove duplicate balance ops weights + refund on all dispatchables

02f8cab

shawntabrizi reviewed Apr 24, 2020

View reviewed changes

shawntabrizi approved these changes Apr 24, 2020

View reviewed changes

shawntabrizi added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 24, 2020

gnunicorn merged commit 66960eb into master Apr 24, 2020

gnunicorn deleted the apopiak-weight-params branch April 24, 2020 14:47

apopiak mentioned this pull request Apr 24, 2020

Timestamp weights #5775

Merged

sorpaas mentioned this pull request Apr 24, 2020

Companion PR for #5501 and #5680 paritytech/polkadot#1012

Merged

Conversation

apopiak commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

API Notes:

Drive-by Changes:

Uh oh!

parity-cla-bot commented Apr 17, 2020

Uh oh!

Uh oh!

Uh oh!

apopiak commented Apr 23, 2020

Uh oh!

apopiak commented Apr 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apopiak Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shawntabrizi left a comment

Choose a reason for hiding this comment

Uh oh!

shawntabrizi left a comment

Choose a reason for hiding this comment

Uh oh!

shawntabrizi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

apopiak commented Apr 17, 2020 •

edited

Loading

apopiak Apr 24, 2020 •

edited

Loading