Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Benchmark for on_container_author_noted only #767

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

nanocryk
Copy link
Contributor

This new bench will allow us to get a better idea of how much 1 author noting progress costs, and thus better estimate how much we can support in one block.

Do you see any issue removing the condition over runtime-benchmarks features in the runtimes? set_latest_author_data will change weight but as it is Mandatory the weight hint doesn't matter IIRC.

@nanocryk nanocryk added a0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes labels Nov 28, 2024
Copy link
Contributor

github-actions bot commented Nov 28, 2024

WASM runtime size check:

Compared to target branch

dancebox runtime: 1424 KB (no changes) ✅

flashbox runtime: 820 KB (no changes) ✅

dancelight runtime: 2104 KB (+2104 KB) ⚠️

container chain template simple runtime: 1124 KB (-4296 KB) ✅

container chain template frontier runtime: 1400 KB (-5152 KB) ✅

Copy link
Contributor

github-actions bot commented Nov 28, 2024

Coverage Report

(master)

@@                            Coverage Diff                            @@
##           master   jeremy-bench-on-container-author-noted     +/-   ##
=========================================================================
  Coverage   64.56%                                   64.56%   0.00%     
  Files         320                                      320             
+ Lines       55422                                    55423      +1     
=========================================================================
+ Hits        35781                                    35782      +1     
  Misses      19641                                    19641             
Files Changed Coverage
/pallets/services-payment/src/lib.rs 90.30% (+0.03%)

Coverage generated Thu Dec 5 16:36:11 UTC 2024

@@ -137,6 +137,14 @@ benchmarks! {
assert_ok!(Pallet::<T>::set_author(RawOrigin::Root.into(), para_id, block_number, author, u64::from(block_number).into()));
}: _(RawOrigin::Root, para_id)

on_container_author_noted {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we should run it no (run it once at least in your own machine)? this should appear now when we benchmark author-noting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I'll run it on my machine for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Will you push the weights to the PR @nanocryk ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a0-pleasereview Pull request needs code review. B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. not-breaking Does not need to be mentioned in breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants