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

approval-voting: Make tests deterministic #3899

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

alexggh
Copy link
Contributor

@alexggh alexggh commented Mar 29, 2024

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

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>
@sandreim
Copy link
Contributor

sandreim commented Apr 8, 2024

This sounds like a good idea to try. This would reduce the number of async tasks as we do this to implement latency:

// Emulate RTT latency
			self.spawn_handle
				.spawn("peer-latency-emulator", "test-environment", async move {
					tokio::time::sleep(latency_ms).await;
					to_node.unbounded_send(message).expect("Sending to the node never fails");
				});

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Check value every 50ms.
// Check value every 1000ms.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why?

Copy link
Contributor Author

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.

@AndreiEres
Copy link
Contributor

AndreiEres commented Apr 9, 2024

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");
		});
}

@AndreiEres
Copy link
Contributor

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.
WDYT @sandreim @alexggh

github-merge-queue bot pushed a commit that referenced this pull request Apr 11, 2024
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.
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this pull request May 27, 2024
Implements the idea from
paritytech#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.
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.

None yet

3 participants