Skip to content

Optimize initialization of networking protocol benchmarks#6636

Merged
AndreiEres merged 10 commits intomasterfrom
AndreiEres/network-bench-update
Dec 5, 2024
Merged

Optimize initialization of networking protocol benchmarks#6636
AndreiEres merged 10 commits intomasterfrom
AndreiEres/network-bench-update

Conversation

@AndreiEres
Copy link
Copy Markdown
Contributor

@AndreiEres AndreiEres commented Nov 25, 2024

Description

These changes should enhance the quality of benchmark results by excluding worker initialization time from the measurements and reducing the overall duration of the benchmarks.

Integration

It should not affect any downstream projects.

Review Notes

  • Workers initialize once per benchmark to avoid side effects.
  • The listen address is assigned when a worker starts.
  • Benchmarks are divided into two groups by size to create better charts for comparison.

@AndreiEres AndreiEres added R0-no-crate-publish-required The change does not require any crates to be re-published. T12-benchmarks This PR/Issue is related to benchmarking and weights. labels Nov 25, 2024
@AndreiEres AndreiEres marked this pull request as ready for review November 25, 2024 12:31
@AndreiEres
Copy link
Copy Markdown
Contributor Author

/cmd prdoc --audience node_dev

@paritytech-workflow-stopper
Copy link
Copy Markdown

All GitHub workflows were cancelled due to failure one of the required jobs.
Failed workflow url: https://github.com/paritytech/polkadot-sdk/actions/runs/12010630549
Failed job name: cargo-clippy

Copy link
Copy Markdown
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

Nice! 👍

Andrei could you give us a new picture of the report litep2p vs libp2p? 🙏

Copy link
Copy Markdown
Contributor

@dmitry-markin dmitry-markin left a comment

Choose a reason for hiding this comment

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

Exactly what we need!

(27, "128MB"),
const SMALL_PAYLOAD: &[(u32, usize, &'static str)] = &[
// (Exponent of size, number of notifications, label)
(6, 100, "64B"),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we increase the number of notifications, especially for smaller payload sizes? Even on localhost, the TCP window control can influence the results quite a lot and for low count of small notifications we can get inaccurate results.

Ideally, we would like to transmit tens of megabytes for every notification size. I.e., we can keep constant (size) * (number) = 100 MB. In case it takes too long for smaller payload sizes, we can limit it to 15-30 seconds. And we can also decrease the number of criterion iterations for every size. 10 should be fine to have an estimate of the mean deviation

We can do this number picking in a follow up PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can make the number of notifications a variable, but this will disrupt the charts that track the total time for a transfer.

let service1 = worker1.network_service().clone();
let (worker2, rx2, _notification_service2) =
create_network_worker::<B, H, N>(listen_address2.clone());
let _guard = rt.enter();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is this needed for?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Without it we can't initialize workers that needs to spawn a tokio task inside.

let (break_tx, break_rx) = async_channel::bounded(10);
async fn run_with_backpressure(setup: Arc<BenchSetup>, size: usize, limit: usize) {
let (break_tx, break_rx) = async_channel::bounded(1);
let requests = futures::future::join_all((0..limit).into_iter().map(|_| {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be interesting to compare the results when using a single-threaded runtime and a multi-threaded one and see if they differ.

@AndreiEres AndreiEres added this pull request to the merge queue Dec 5, 2024
Merged via the queue into master with commit f4a196a Dec 5, 2024
@AndreiEres AndreiEres deleted the AndreiEres/network-bench-update branch December 5, 2024 09:44
sylvaincormier pushed a commit to sylvaincormier/polkadot-sdk that referenced this pull request Dec 5, 2024
…#6636)

# Description
These changes should enhance the quality of benchmark results by
excluding worker initialization time from the measurements and reducing
the overall duration of the benchmarks.

### Integration
It should not affect any downstream projects.

### Review Notes
- Workers initialize once per benchmark to avoid side effects.  
- The listen address is assigned when a worker starts.  
- Benchmarks are divided into two groups by size to create better charts
for comparison.

---------

Co-authored-by: GitHub Action <[email protected]>
Krayt78 pushed a commit to Krayt78/polkadot-sdk that referenced this pull request Dec 18, 2024
…#6636)

# Description
These changes should enhance the quality of benchmark results by
excluding worker initialization time from the measurements and reducing
the overall duration of the benchmarks.

### Integration
It should not affect any downstream projects.

### Review Notes
- Workers initialize once per benchmark to avoid side effects.  
- The listen address is assigned when a worker starts.  
- Benchmarks are divided into two groups by size to create better charts
for comparison.

---------

Co-authored-by: GitHub Action <[email protected]>
dudo50 pushed a commit to paraspell-research/polkadot-sdk that referenced this pull request Jan 4, 2025
…#6636)

# Description
These changes should enhance the quality of benchmark results by
excluding worker initialization time from the measurements and reducing
the overall duration of the benchmarks.

### Integration
It should not affect any downstream projects.

### Review Notes
- Workers initialize once per benchmark to avoid side effects.  
- The listen address is assigned when a worker starts.  
- Benchmarks are divided into two groups by size to create better charts
for comparison.

---------

Co-authored-by: GitHub Action <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

R0-no-crate-publish-required The change does not require any crates to be re-published. T12-benchmarks This PR/Issue is related to benchmarking and weights.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants