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

Add support for runtime schedule control #4438

Merged
merged 5 commits into from
Jan 31, 2022

Conversation

kostko
Copy link
Member

@kostko kostko commented Jan 20, 2022

Based on #4415.
Also adds functionality needed for #4436.

TODO

  • Cleanup.
  • More testing, including long-term tests.
  • Support for fetching more transactions from the host (so that we don't need to always dump the full txpool into the runtime to then just schedule a few of them).
  • Interoperability testing with old runtimes and old nodes in the same committee.
  • Forward-port some fixes from [BACKPORT/21.3.x] Add support for runtime schedule control #4446.

@kostko kostko added c:runtime/compute Category: runtime compute worker c:runtime/scheduler Category: runtime txn scheduler labels Jan 20, 2022
@kostko kostko force-pushed the kostko/feature/rt-schedule-control branch 2 times, most recently from cc3d529 to bcb893d Compare January 20, 2022 17:53
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch from 0f54266 to 37b80a7 Compare January 21, 2022 08:09
@kostko kostko force-pushed the kostko/feature/rt-schedule-control branch from bcb893d to 029b27d Compare January 21, 2022 08:10
go/runtime/host/protocol/types.go Outdated Show resolved Hide resolved
go/runtime/host/protocol/types.go Outdated Show resolved Hide resolved
defer q.Unlock()

var (
batch []*transaction.CheckedTransaction
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense pre-allocating batch here (with length 0)

@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch from 37b80a7 to 3a18290 Compare January 26, 2022 08:55
@kostko kostko force-pushed the kostko/feature/rt-schedule-control branch 4 times, most recently from 6dfd433 to 540aaab Compare January 27, 2022 09:09
@kostko kostko force-pushed the kostko/feature/incoming-rt-messages branch 4 times, most recently from 6a038f4 to c95e5a1 Compare January 28, 2022 09:54
Base automatically changed from kostko/feature/incoming-rt-messages to master January 28, 2022 10:28
@kostko kostko force-pushed the kostko/feature/rt-schedule-control branch from 540aaab to ca07ed3 Compare January 28, 2022 10:38
@kostko kostko marked this pull request as ready for review January 28, 2022 10:38
@kostko kostko force-pushed the kostko/feature/rt-schedule-control branch 2 times, most recently from 9d265b4 to a0975f2 Compare January 29, 2022 19:10
@kostko kostko force-pushed the kostko/feature/rt-schedule-control branch from a0975f2 to a6e547e Compare January 29, 2022 19:21
@codecov
Copy link

codecov bot commented Jan 31, 2022

Codecov Report

Merging #4438 (a6e547e) into master (795eb65) will increase coverage by 0.21%.
The diff coverage is 74.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4438      +/-   ##
==========================================
+ Coverage   68.66%   68.87%   +0.21%     
==========================================
  Files         415      415              
  Lines       46571    46759     +188     
==========================================
+ Hits        31976    32204     +228     
+ Misses      10635    10609      -26     
+ Partials     3960     3946      -14     
Impacted Files Coverage Δ
go/runtime/registry/host.go 71.88% <0.00%> (-4.81%) ⬇️
go/worker/common/committee/runtime_host.go 50.00% <0.00%> (-7.15%) ⬇️
go/worker/compute/executor/committee/state.go 87.09% <ø> (ø)
go/worker/compute/executor/committee/batch.go 75.75% <33.33%> (-16.25%) ⬇️
go/worker/compute/executor/committee/node.go 71.66% <75.11%> (+2.37%) ⬆️
go/runtime/host/sandbox/sandbox.go 71.27% <83.33%> (+0.21%) ⬆️
...ling/simple/txpool/priorityqueue/priority_queue.go 88.82% <90.00%> (+0.22%) ⬆️
go/runtime/host/mock/mock.go 92.38% <100.00%> (+0.46%) ⬆️
go/runtime/host/protocol/connection.go 67.66% <100.00%> (-6.08%) ⬇️
go/runtime/host/protocol/types.go 57.14% <100.00%> (+3.29%) ⬆️
... 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 740bca8...a6e547e. Read the comment docs.

@kostko kostko enabled auto-merge January 31, 2022 14:15
hashes := make([]hash.Hash, len(batch))
for i, tx := range batch {
hashes[i] = tx.Hash()
hashes := make([]hash.Hash, 0, len(batch))
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Since you're inserting duplicates, should this be

Suggested change
hashes := make([]hash.Hash, 0, len(batch))
hashes := make([]hash.Hash, 0, 2*len(batch))

Or just remove the capacity entirely.

Copy link
Contributor

@Yawning Yawning left a comment

Choose a reason for hiding this comment

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

Looks fine, we're basically running a more involved version of this already.

@@ -254,6 +259,18 @@ func (c *connection) Close() {
c.quitWg.Wait()
}

// Implements Connection.
func (c *connection) GetInfo(ctx context.Context) (*RuntimeInfoResponse, error) {
c.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't this just use defer?

@kostko kostko merged commit b8c6ecd into master Jan 31, 2022
@kostko kostko deleted the kostko/feature/rt-schedule-control branch January 31, 2022 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:runtime/compute Category: runtime compute worker c:runtime/scheduler Category: runtime txn scheduler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants