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.

Sign up for GitHub

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

Jump to bottom

Skip idle_scheduler Calls When Not Producing blocks #1721

Merged
merged 31 commits into from
Apr 7, 2022
Merged

Skip idle_scheduler Calls When Not Producing blocks #1721

merged 31 commits into from
Apr 7, 2022
+328 −74

Conversation

ferrell-code
Copy link
Contributor

@ferrell-code ferrell-code commented Dec 22, 2021 •
edited
Loading

closes #1467

@ferrell-code ferrell-code marked this pull request as draft December 22, 2021 22:42
xlc
xlc reviewed Dec 24, 2021
View reviewed changes
Copy link
Member

@xlc xlc 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 fully get the formula. While we currently expecting one para block per two relay blocks, that will change in future and we shouldn't assume that.

Also BlockNumberDifference doesn't get updated on the do_dispatch_tasks path so it will get triggered every few blocks?

@ferrell-code
Copy link
Contributor Author

Yeah I could use aura slots instead but our consensus engine could change too.

Under this solution it would get triggered every 3 slots skipped... so under perfect conditions it would never update BlockNumberDifference. It felt silly to constantly write to storage

xlc
xlc reviewed Dec 28, 2021
View reviewed changes
@ferrell-code
Copy link
Contributor Author

ferrell-code commented Dec 28, 2021 •
edited
Loading

We might want to wait for #1734 to merge. IdleScheduler needs to execute on_initialize in this PR before ParachainSystem, therefore currently we would need to change the order they are listed in construct_runtime and then change back if we want to implement the more logical order of hooks executing in order rather than in reverse

@codecov
Copy link

codecov bot commented Jan 20, 2022 •
edited
Loading

Codecov Report

Merging #1721 (9c2b58a) into master (3833ebc) will decrease coverage by 0.00%.
The diff coverage is 87.17%.

@@            Coverage Diff             @@
##           master    #1721      +/-   ##
==========================================
- Coverage   86.38%   86.38%   -0.01%     
==========================================
  Files          98       98              
  Lines       20131    20163      +32     
==========================================
+ Hits        17390    17417      +27     
- Misses       2741     2746       +5
Impacted Files Coverage Δ
modules/idle-scheduler/src/lib.rs 85.71% <77.27%> (-6.40%) ⬇️
modules/idle-scheduler/src/tests.rs 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3833ebc...9c2b58a. Read the comment docs.

@ferrell-code ferrell-code marked this pull request as ready for review March 31, 2022 03:13
@ferrell-code
Copy link
Contributor Author

Need to fix mock on evm-tests before merge

@ferrell-code ferrell-code mentioned this pull request Mar 31, 2022
Merged
shaunxw
shaunxw reviewed Apr 1, 2022
View reviewed changes
@ferrell-code
Copy link
Contributor Author

/bench runtime acala module_idle_scheduler

@ghost
Copy link

ghost commented Apr 4, 2022

Starting benchmark for branch: fer-check-idle (vs master)

Comment will be updated.

@ferrell-code ferrell-code requested a review from shaunxw April 4, 2022 21:47
@ferrell-code
Copy link
Contributor Author

ferrell-code commented Apr 4, 2022 •
edited
Loading

Added on_initialize and more comprehensive benchmarks to on_idle too

shaunxw
shaunxw previously approved these changes Apr 4, 2022
View reviewed changes
xlc
xlc previously approved these changes Apr 7, 2022
View reviewed changes
@shaunxw shaunxw merged commit 05ae437 into master Apr 7, 2022
@shaunxw shaunxw deleted the fer-check-idle branch April 7, 2022 21:55
syan095 pushed a commit that referenced this pull request Apr 8, 2022
Merge remote-tracking branch 'origin/master' into impl-fungibles-trai…
1558334 
…ts-for-currencies

* origin/master: (362 commits)
  Skip idle_scheduler Calls When Not Producing blocks  (#1721)
  config more homa subaccounts for Karura (#2004)
  rm wt storage (#1984)
  Update construct_runtime's syntax (#2000)
  polkadot v0.9.18 (#1988)
  be srtool compatible (#2021)
  Update README.md (#2022)
  update precompile weights (#2020)
  Mandala supports PricingPegged for taiKSM (#2015)
  remove unneeded param (#2014)
  fix srtool (#2013)
  update precompile gas_limit (#2007)
  fix verify bootnodes script
  Bump minimist from 1.2.5 to 1.2.6 in /ts-tests (#2008)
  Bump (#2005)
  add NativeAssetId to asset registry (#1993)
  connect earning and incentives (#1995)
  Liquidation (#1955)
  Improvement/orml oracle (#1998)
  Mock xcm (#1977)
  ...

# Conflicts:
#	modules/currencies/src/lib.rs
#	predeploy-contracts
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shaunxw shaunxw

@xlc xlc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Skip idle scheduler if block production took too long
3 participants
@ferrell-code @xlc @shaunxw