-
Notifications
You must be signed in to change notification settings - Fork 935
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
Parallelize process_inboxes_and_force_validator_updates #3245
Parallelize process_inboxes_and_force_validator_updates #3245
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
linera-client/src/client_context.rs
Outdated
let (listener, _listen_handle, notification_stream) = chain_client.listen().await?; | ||
self.chain_listeners.spawn_task(listener); | ||
Ok(notification_stream) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these need to be pub
? And shouldn't they be in the #[cfg(feature = "benchmark")]
impl below?
linera-client/src/client_context.rs
Outdated
result? | ||
}; | ||
certificates.extend(new_certificates); | ||
if maybe_timeout.is_none() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this ever be Some
in the benchmark? Aren't these all single-user chains, where we expect the first proposal to succeed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! They're all single super owner chains. This should simplify the code quite a bit :)
linera-client/src/client_context.rs
Outdated
}) | ||
.collect::<Vec<_>>(); | ||
|
||
let mut chain_client_and_notification_stream = Vec::new(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut chain_client_and_notification_stream = Vec::new(); | |
let mut chain_clients_and_notification_streams = Vec::new(); |
This one is significantly faster, 850ms in parallel vs 3500ms serially |
d19808a
to
34e94bf
Compare
2e762fc
to
466a717
Compare
466a717
to
8af1570
Compare
34e94bf
to
96d6a58
Compare
8af1570
to
f7cca92
Compare
96d6a58
to
179c093
Compare
6797419
to
6e1bb0d
Compare
linera-client/src/client_context.rs
Outdated
.await | ||
.expect("Processing inbox should not fail!"); | ||
chain_client | ||
.update_validators(None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if that's even necessary. process_inbox_without_prepare
already updates the validators about the new certificates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably an unlikely case, but from what I looked, process_inbox_without_prepare
will only execute a new block (and then update the validators) if we have pending message bundles. If that's not the case, then we won't update the validators.
update_validators
in this case will only update anything if our committee has changed. So I guess in the case where we're running this against the testnet or something, and some validator gets added/removed, we should keep this around to make sure things will be properly updated in that case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if the committee changed, we do receive a message about it, so there will be a pending message bundle. (At least until we have event streams—but even if, I'd say it isn't the benchmark's task to update the validators about the admin chain.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see! Then maybe we don't need it. I'll remove it.
179c093
to
4073be0
Compare
6e1bb0d
to
09aeee8
Compare
09aeee8
to
fdd3ba9
Compare
Motivation
Will start paralellizing some parts of this existing flow
Proposal
Paralellize initial validator sync step
Test Plan
CI + ran locally
Release Plan