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

runtime: add support for transaction priority and weights #3965

Merged
merged 5 commits into from
May 28, 2021

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented May 24, 2021

Closes #3760

See also: oasisprotocol/oasis-sdk#146

Adds support for runtime transaction priority and weights that can be set by runtime on CheckTx. Transaction are scheduled by priority (higher first). Transaction weights are used by scheduler when creating batches to ensure no batch passes the per-round weight limits. Supported limits are:

  • size - number of transactions - per batch/round limit is defined in the registry
  • size_bytes - size of transaction in bytes - per batch/round limit is defined in the registry
  • consensus_messages - number of emitted consensus messages - per batch/round limit is defined in the registry
  • custom runtime specific weights - runtime is queried for the per/batch round limits (TODO: open issue as a follow up - all currently used weights are obtained from the registry anyway)

Per-account sequence numbers can be implemented in a follow-up PR (TODO: open issue), as unless all runtimes are required to implement accounts, the scheduler needs to handle the base case (no per-account seq. numbers) as well.

TODO:

  • tests
  • open follow up issues (querying per-round custom weight limits from the runtime, and support for per-account sequence numbers)

go/common/version/version.go Outdated Show resolved Hide resolved
runtime/src/common/quantity.rs Outdated Show resolved Hide resolved
@ptrus ptrus force-pushed the ptrus/feature/smarter-txscheduler branch 6 times, most recently from bfe5234 to ccfdc9c Compare May 25, 2021 11:47
@ptrus ptrus marked this pull request as ready for review May 25, 2021 13:02
@codecov
Copy link

codecov bot commented May 25, 2021

Codecov Report

Merging #3965 (ac2385c) into master (da3d951) will decrease coverage by 0.05%.
The diff coverage is 82.98%.

❗ Current head ac2385c differs from pull request most recent head f6222b0. Consider uploading reports for the commit f6222b0 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3965      +/-   ##
==========================================
- Coverage   67.22%   67.17%   -0.06%     
==========================================
  Files         410      410              
  Lines       41819    41815       -4     
==========================================
- Hits        28112    28088      -24     
- Misses       9751     9765      +14     
- Partials     3956     3962       +6     
Impacted Files Coverage Δ
go/worker/compute/executor/init.go 100.00% <ø> (ø)
go/runtime/host/helpers.go 60.97% <28.57%> (ø)
go/runtime/host/protocol/types.go 47.36% <60.00%> (+4.51%) ⬆️
go/runtime/scheduling/init.go 60.00% <66.66%> (ø)
go/worker/compute/executor/committee/node.go 70.11% <75.55%> (+0.11%) ⬆️
go/runtime/scheduling/simple/simple.go 80.00% <80.00%> (-1.40%) ⬇️
...ling/simple/txpool/priorityqueue/priority_queue.go 86.95% <86.95%> (ø)
go/runtime/client/client.go 66.11% <100.00%> (ø)
go/runtime/transaction/transaction.go 79.27% <100.00%> (+0.86%) ⬆️
go/worker/compute/executor/tests/tester.go 88.52% <100.00%> (ø)
... and 39 more

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 da3d951...f6222b0. Read the comment docs.

@ptrus ptrus force-pushed the ptrus/feature/smarter-txscheduler branch 3 times, most recently from 479737d to 23cd37e Compare May 26, 2021 08:27
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Did you do any benchmarking to compare with the previous implementation?

go/runtime/scheduling/api/api.go Outdated Show resolved Hide resolved
.changelog/3965.cfg.md Outdated Show resolved Hide resolved
go/runtime/scheduling/simple/txpool/tester.go Outdated Show resolved Hide resolved
go/runtime/transaction/transaction.go Show resolved Hide resolved
go/worker/compute/executor/committee/node.go Outdated Show resolved Hide resolved
Comment on lines +92 to +110
// XXX: potentially there could be smaller transactions that would
// fit, which this will miss. Could do some lookahead.
Copy link
Member

Choose a reason for hiding this comment

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

Should we open a follow up issue for this?

@ptrus
Copy link
Member Author

ptrus commented May 26, 2021

Did you do any benchmarking to compare with the previous implementation?

Yeah I did. I can dig up the results, but generally the adding/getting batches is a bit slower than previous orderedmap implementation. But nothing really drastic - and still better than the queue implementation we had initially - i compared with results here: #3434 (comment)).

The drastic performance degradation is in the RemoveBatch, (somewhat expected) as we need to individually delete each element and there's no batch optimization.

@ptrus ptrus force-pushed the ptrus/feature/smarter-txscheduler branch 4 times, most recently from 759a19b to dc8a844 Compare May 27, 2021 09:05
@ptrus ptrus force-pushed the ptrus/feature/smarter-txscheduler branch from dc8a844 to ac2385c Compare May 27, 2021 15:55
@ptrus ptrus force-pushed the ptrus/feature/smarter-txscheduler branch from ac2385c to f6222b0 Compare May 28, 2021 11:40
@ptrus ptrus enabled auto-merge May 28, 2021 11:40
@ptrus ptrus merged commit 640adb6 into master May 28, 2021
@ptrus ptrus deleted the ptrus/feature/smarter-txscheduler branch May 28, 2021 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Smarter transaction scheduler
2 participants