-
Notifications
You must be signed in to change notification settings - Fork 766
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
approval-voting: Make tests deterministic #3899
Conversation
With random connectivity and latency is hard to actually figure it out a delta in the benchmarking, so disable them in order to get full deterministic behaviour when measuring performance. Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
This sounds like a good idea to try. This would reduce the number of async tasks as we do this to implement latency:
However, I'd expect we generate deterministic latencies for the tests, so we get the same thing in all CI run. |
@@ -347,7 +347,7 @@ impl TestEnvironment { | |||
break | |||
} | |||
// Check value every 50ms. |
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.
// Check value every 50ms. | |
// Check value every 1000ms. |
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 why?
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.
Accidentally changed it :D , will revert it back.
I noticed that with sending messages in a spawned task even if the latency is zero, I receive more stable results. Without it with a zero latency, results can spike up to 40%. pub async fn send_message(&mut self, message: NetworkMessage) {
self.tx_limiter.reap(message.size()).await;
let to_node = self.to_node.clone();
let latency = std::time::Duration::from_millis(self.latency_ms as u64);
// Emulate RTT latency
self.spawn_handle
.spawn("peer-latency-emulator", "test-environment", async move {
if !latency.is_zero() {
tokio::time::sleep(latency).await;
}
to_node.unbounded_send(message).expect("Sending to the node never fails");
});
} |
I tried running availability benchmarks with a zero latency, but I can't say that I see a meaningful difference. It even has no sense to post the results here. However, I would try to apply this to our availability benchmarks and check the charts with the results over a time. |
Implements the idea from #3899 - Removed latencies - Number of runs reduced from 50 to 5, according to local runs it's quite enough - Network message is always sent in a spawned task, even if latency is zero. Without it, CPU time sometimes spikes. - Removed the `testnet` profile because we probably don't need that debug additions. After the local tests I can't say that it brings a significant improvement in the stability of the results. However, I belive it is worth trying and looking at the results over time.
With random connectivity and latency is hard to actually figure it out a delta in the benchmarking, so disable them in order to get full deterministic behaviour when measuring performance. At least on my machine with this configuration the results for approval-throughput are really similar between subsequent runs: ``` CPU usage, seconds total per block approval-distribution 36.9025 3.6902 approval-distribution 36.7579 3.6758 approval-distribution 37.0418 3.7042 approval-distribution 37.0339 3.7034 approval-distribution 36.9342 3.6934 approval-distribution 36.7177 3.6718 approval-voting 52.7756 5.2776 approval-voting 52.5999 5.2600 approval-voting 53.2158 5.3216 approval-voting 53.2493 5.3249 approval-voting 52.8524 5.2852 approval-voting 52.8611 5.2861 approval-voting 52.8210 5.2821 ``` --------- Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
With random connectivity and latency is hard to actually figure it out a delta in the benchmarking, so disable them in order to get full deterministic behaviour when measuring performance. At least on my machine with this configuration the results for approval-throughput are really similar between subsequent runs: ``` CPU usage, seconds total per block approval-distribution 36.9025 3.6902 approval-distribution 36.7579 3.6758 approval-distribution 37.0418 3.7042 approval-distribution 37.0339 3.7034 approval-distribution 36.9342 3.6934 approval-distribution 36.7177 3.6718 approval-voting 52.7756 5.2776 approval-voting 52.5999 5.2600 approval-voting 53.2158 5.3216 approval-voting 53.2493 5.3249 approval-voting 52.8524 5.2852 approval-voting 52.8611 5.2861 approval-voting 52.8210 5.2821 ``` --------- Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
With random connectivity and latency is hard to actually figure it out a delta in the benchmarking, so disable them in order to get full deterministic behaviour when measuring performance. At least on my machine with this configuration the results for approval-throughput are really similar between subsequent runs: ``` CPU usage, seconds total per block approval-distribution 36.9025 3.6902 approval-distribution 36.7579 3.6758 approval-distribution 37.0418 3.7042 approval-distribution 37.0339 3.7034 approval-distribution 36.9342 3.6934 approval-distribution 36.7177 3.6718 approval-voting 52.7756 5.2776 approval-voting 52.5999 5.2600 approval-voting 53.2158 5.3216 approval-voting 53.2493 5.3249 approval-voting 52.8524 5.2852 approval-voting 52.8611 5.2861 approval-voting 52.8210 5.2821 ``` --------- Signed-off-by: Alexandru Gheorghe <alexandru.gheorghe@parity.io>
With random connectivity and latency is hard to actually figure it out a delta in the benchmarking, so disable them in order to get full deterministic behaviour when measuring performance.
At least on my machine with this configuration the results for approval-throughput are really similar between subsequent runs: