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

Implement batch_all and update Utility pallet for weight refunds #7188

Merged
13 commits merged into from
Oct 27, 2020

Conversation

xlc
Copy link
Contributor

@xlc xlc commented Sep 23, 2020

Fixes #6519

Also updated #[transactional] to make it handle returns correctly.

polkadot companion: paritytech/polkadot#1775

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.

Looks good, but not sure if we need some different base weight for transactional dispatchables?

@shawntabrizi

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.

I don't see why there would be different weights based on transactional dispatch. Should there be?

@xlc can you confirm quickly that the base weight running batch and batch_all are the same on your local computer?

If you want to be really safe, it is probably best for each function to get its own weight function anyway, rather than sharing one. It allows more flexibility in the future for customization and for the functions to potentially diverge in logic.

@xlc
Copy link
Contributor Author

xlc commented Sep 24, 2020

Local benchmark result (with bunch browser tabs open and IDE running)

Pallet: "pallet_utility", Extrinsic: "batch", Lowest values: [], Highest values: [], Steps: [], Repeat: 200
Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    239.7
    + c    0.461
              µs

Reads = 0 + (0 * c)
Writes = 0 + (0 * c)
Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    c   mean µs  sigma µs       %
    0     237.6     2.219    0.9%
  100     313.2     20.86    6.6%
  200     340.2     11.81    3.4%
  300     378.1     3.134    0.8%
  400       423     3.532    0.8%
  500     477.3     16.24    3.4%
  600       591     6.071    1.0%
  700     612.9     38.86    6.3%
  800     627.1     28.18    4.4%
  900     655.1     4.802    0.7%
 1000     697.9     0.868    0.1%

Quality and confidence:
param     error
c         0.002

Model:
Time ~=    251.8
    + c     0.47
              µs

Reads = 0 + (0 * c)
Writes = 0 + (0 * c)
Pallet: "pallet_utility", Extrinsic: "batch_all", Lowest values: [], Highest values: [], Steps: [], Repeat: 200
Median Slopes Analysis
========
-- Extrinsic Time --

Model:
Time ~=    255.1
    + c    0.448
              µs

Reads = 0 + (0 * c)
Writes = 0 + (0 * c)
Min Squares Analysis
========
-- Extrinsic Time --

Data points distribution:
    c   mean µs  sigma µs       %
    0     253.5     2.751    1.0%
  100     299.2      0.42    0.1%
  200     348.8     3.611    1.0%
  300     390.2     4.021    1.0%
  400     471.6     15.69    3.3%
  500     479.1     23.33    4.8%
  600     521.5     6.381    1.2%
  700     568.6     16.76    2.9%
  800       599     0.832    0.1%
  900     669.9     22.94    3.4%
 1000     744.6     18.76    2.5%

Quality and confidence:
param     error
c         0.001

Model:
Time ~=    254.3
    + c    0.463
              µs
diff --git a/frame/utility/src/benchmarking.rs b/frame/utility/src/benchmarking.rs
index 8ca0e216f..8713fecae 100644
--- a/frame/utility/src/benchmarking.rs
+++ b/frame/utility/src/benchmarking.rs
@@ -49,6 +49,19 @@ benchmarks! {
                assert_last_event::<T>(Event::BatchCompleted.into())
        }

+       batch_all {
+               let c in 0 .. 1000;
+               let mut calls: Vec<<T as Trait>::Call> = Vec::new();
+               for i in 0 .. c {
+                       let call = frame_system::Call::remark(vec![]).into();
+                       calls.push(call);
+               }
+               let caller = whitelisted_caller();
+       }: _(RawOrigin::Signed(caller), calls)
+       verify {
+               assert_last_event::<T>(Event::BatchCompleted.into())
+       }
+
        as_derivative {
                let u in 0 .. 1000;
                let caller = account("caller", u, SEED);

@shawntabrizi shawntabrizi changed the title implement batch_all Implement batch_all and update Utility pallet for weight refunds Oct 2, 2020
@shawntabrizi
Copy link
Member

shawntabrizi commented Oct 2, 2020

@xlc and @bkchr please take a look at and review my changes.

Note that your PR actually had a mistake, where you assume weight should start at zero, but it should actually start at base_weight using the WeightInfo.

@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. labels Oct 2, 2020
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.

LGTM

Does this needs an audit?

frame/utility/src/lib.rs Outdated Show resolved Hide resolved
frame/utility/src/lib.rs Outdated Show resolved Hide resolved
Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>
@shawntabrizi shawntabrizi added the D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. label Oct 2, 2020
@shawntabrizi
Copy link
Member

Does this needs an audit?

I have added the nice to have audit tag, but it seems the only logic it is touching is weights, with no so much concern from me?

@gavofyork what do you think?

@xlc
Copy link
Contributor Author

xlc commented Oct 6, 2020

ping @gavofyork

@bkchr bkchr added D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited and removed D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 12, 2020
@viniul
Copy link
Contributor

viniul commented Oct 26, 2020

We are done with the review for this PR - looks good to us!

@bkchr bkchr added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Oct 26, 2020
@bkchr
Copy link
Member

bkchr commented Oct 26, 2020

@xlc please merge master and we can merge this! :)

@shawntabrizi
Copy link
Member

bot merge

@ghost
Copy link

ghost commented Oct 27, 2020

Trying merge.

This pull request was closed.
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Atomic version of utility.batch
5 participants