Skip to content

feat: added benchmarking binary for torrent repository struct#522

Merged
josecelano merged 3 commits intotorrust:developfrom
mickvandijke:496-benchmark-tracker
Dec 21, 2023
Merged

feat: added benchmarking binary for torrent repository struct#522
josecelano merged 3 commits intotorrust:developfrom
mickvandijke:496-benchmark-tracker

Conversation

@mickvandijke
Copy link
Copy Markdown
Member

@mickvandijke mickvandijke commented Nov 24, 2023 •
edited
Loading

Moved the Tracker.torrents field to be its own TorrentRepository struct with different implementations.

Added a new crate that benchmarks the different TorrentRepository implementations for speed.

Run benchmarks

cargo run --release -p torrust-torrent-repository-benchmarks -- --threads 4 --sleep 0 --compare true

Example result

tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, Entry>>
add_one_torrent: Avg/AdjAvg: (146ns, 0ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (5.965616ms, 5.816375ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (14.050554ms, 14.132902ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (7.201337ms, 7.120315ms)

std::sync::RwLock<std::collections::BTreeMap<InfoHash, Entry>>
add_one_torrent: Avg/AdjAvg: (82ns, 83ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (27.311075ms, 26.869114ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (28.967141ms, 28.967141ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (48.316966ms, 0ns)

std::sync::RwLock<std::collections::BTreeMap<InfoHash, Arc<std::sync::Mutex<Entry>>>>
add_one_torrent: Avg/AdjAvg: (137ns, 125ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (5.057729ms, 5.057729ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (48.833962ms, 48.833962ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (6.193212ms, 6.071166ms)

tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, Arc<std::sync::Mutex<Entry>>>>
add_one_torrent: Avg/AdjAvg: (174ns, 166ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (5.56332ms, 5.56332ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (16.360504ms, 15.872786ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (6.800225ms, 6.890521ms)

tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, Arc<tokio::sync::Mutex<Entry>>>>
add_one_torrent: Avg/AdjAvg: (192ns, 208ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (6.374304ms, 6.17711ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (17.313591ms, 17.089898ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (6.955371ms, 6.975057ms)

Relevant issues

#496
#495

@mickvandijke
Copy link
Copy Markdown
Member Author

mickvandijke commented Nov 29, 2023

Hi @josecelano,

I have a failing test for the connection cookie response. I saw that you wrote a comment that the response can change between versions, but it is not sure to me why this changes. Should I just update the test constant with the new cookie response?

@da2ce7
Copy link
Copy Markdown
Contributor

da2ce7 commented Nov 29, 2023

hello @WarmBeer , just add the new hashed-version to the list: https://github.com/torrust/torrust-tracker/blob/develop/src/servers/udp/connection_cookie.rs#L176

@da2ce7 da2ce7 added the Needs Rebase Base Branch has Incompatibilities label Nov 29, 2023
@da2ce7
Copy link
Copy Markdown
Contributor

da2ce7 commented Nov 29, 2023

Looks like you have been using a old version of the code as you base. If you rebase maybe the connection cookie hash error will disappear.

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 29, 2023 •
edited
Loading

Codecov Report

Attention: 437 lines in your changes are missing coverage. Please review.

Comparison is base (a3274dc) 84.20% compared to head (ebb7d4c) 79.57%.

Files Patch % Lines
src/core/torrent/repository.rs 16.45% 132 Missing ⚠️
.../torrent-repository-benchmarks/src/benches/asyn.rs 0.00% 109 Missing ⚠️
.../torrent-repository-benchmarks/src/benches/sync.rs 0.00% 97 Missing ⚠️
packages/torrent-repository-benchmarks/src/main.rs 1.78% 55 Missing ⚠️
...torrent-repository-benchmarks/src/benches/utils.rs 0.00% 29 Missing ⚠️
packages/torrent-repository-benchmarks/src/args.rs 0.00% 8 Missing ⚠️
src/core/mod.rs 84.44% 7 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #522      +/-   ##
===========================================
- Coverage    84.20%   79.57%   -4.63%     
===========================================
  Files          110      117       +7     
  Lines         7286     7747     +461     
===========================================
+ Hits          6135     6165      +30     
- Misses        1151     1582     +431

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mickvandijke
Copy link
Copy Markdown
Member Author

mickvandijke commented Nov 30, 2023

The contract pipeline job doesn't like my async functions in traits since it is running on stable instead of nightly. Is this something we can ignore or should I try to work around it?

error[E0554]: `#![feature]` may not be used on the stable release channel
 --> src/lib.rs:1:1
  |
1 | #![feature(return_position_impl_trait_in_trait)]
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0706]: functions in traits cannot be declared `async`
   --> src/tracker/torrent/repository.rs:135:5
    |
135 |     async fn update_torrent_with_peer_and_get_stats(&self, info_hash: &InfoHash, peer: &peer::Peer) -> (SwarmStats, bool) {
    |     -----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |     |
    |     `async` because of this
    |
    = note: `async` trait functions are not currently supported
    = note: consider using the `async-trait` crate: https://crates.io/crates/async-trait
    = note: see issue #91611 <https://github.com/rust-lang/rust/issues/91611> for more information

error[E0706]: functions in traits cannot be declared `async`
   --> src/tracker/torrent/repository.rs:193:5
    |
193 |     async fn update_torrent_with_peer_and_get_stats(&self, info_hash: &InfoHash, peer: &peer::Peer) -> (SwarmStats, bool) {
    |     -----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |     |
    |     `async` because of this
    |
    = note: `async` trait functions are not currently supported
    = note: consider using the `async-trait` crate: https://crates.io/crates/async-trait
    = note: see issue #91611 <https://github.com/rust-lang/rust/issues/91611> for more information

error[E0706]: functions in traits cannot be declared `async`
   --> src/tracker/torrent/repository.rs:250:5
    |
250 |     async fn update_torrent_with_peer_and_get_stats(&self, info_hash: &InfoHash, peer: &peer::Peer) -> (SwarmStats, bool) {
    |     -----^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |     |
    |     `async` because of this
    |
    = note: `async` trait functions are not currently supported
    = note: consider using the `async-trait` crate: https://crates.io/crates/async-trait
    = note: see issue #91611 <https://github.com/rust-lang/rust/issues/91611> for more information

@mickvandijke mickvandijke marked this pull request as ready for review November 30, 2023 11:03
@da2ce7
Copy link
Copy Markdown
Contributor

da2ce7 commented Nov 30, 2023

We do not have any !feature directives in our cow now... maybe the crate supplies a similar thing until it is stabilised?

@da2ce7 da2ce7 force-pushed the 496-benchmark-tracker branch from 68a9be1 to c1fbfea Compare December 2, 2023 09:35
@da2ce7 da2ce7 removed the Needs Rebase Base Branch has Incompatibilities label Dec 2, 2023
@da2ce7
Copy link
Copy Markdown
Contributor

da2ce7 commented Dec 2, 2023

@WarmBeer: Rebased and Fixed Clippy

@da2ce7 da2ce7 force-pushed the 496-benchmark-tracker branch from c1fbfea to 226ea1a Compare December 3, 2023 05:24
@da2ce7 da2ce7 force-pushed the 496-benchmark-tracker branch from a10f466 to 100eb00 Compare December 3, 2023 06:12
@da2ce7 da2ce7 force-pushed the 496-benchmark-tracker branch from 4be7ec8 to 2c79be5 Compare December 3, 2023 06:39
@da2ce7 da2ce7 force-pushed the 496-benchmark-tracker branch from 2c79be5 to 6b5b5ea Compare December 3, 2023 09:18
@da2ce7 da2ce7 force-pushed the 496-benchmark-tracker branch from 6b5b5ea to b0a1bb0 Compare December 3, 2023 09:22
@da2ce7
Copy link
Copy Markdown
Contributor

da2ce7 commented Dec 3, 2023

@WarmBeer I have got the CI to pass, by disabling the Stable Matrix Options, and Moving the ContainerFile to use the rust nightly.

@josecelano josecelano added the Needs Rebase Base Branch has Incompatibilities label Dec 4, 2023
@josecelano
Copy link
Copy Markdown
Member

josecelano commented Dec 4, 2023

Hi @WarmBeer this needs a rebase.

@josecelano
Copy link
Copy Markdown
Member

josecelano commented Dec 4, 2023

Hi @WarmBeer the command in the PR description is wrong:

cargo run --release -p torrust-torrent-repository-benchmarks -- --threads 4 --sleep 0 --compare true

instead of:

cargo run --release -p torrust-torrent-repository-benchmarks -- -threads 4 --sleep 0 -compare true

This is the result on my machine:

$ cargo run --release -p torrust-torrent-repository-benchmarks -- --threads 4 --sleep 0 --compare true
    Finished release [optimized + debuginfo] target(s) in 0.07s
     Running `target/release/torrust-torrent-repository-benchmarks --threads 4 --sleep 0 --compare true`
tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, Entry>>
add_one_torrent: Avg/AdjAvg: (57ns, 59ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (15.552808ms, 15.806128ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (20.473951ms, 20.818356ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (17.505768ms, 17.723086ms)

std::sync::RwLock<std::collections::BTreeMap<InfoHash, Entry>>
add_one_torrent: Avg/AdjAvg: (49ns, 49ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (5.951511ms, 5.951511ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (8.636744ms, 8.636744ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (6.938437ms, 6.938437ms)

std::sync::RwLock<std::collections::BTreeMap<InfoHash, Arc<std::sync::Mutex<Entry>>>>
add_one_torrent: Avg/AdjAvg: (59ns, 59ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (6.11163ms, 6.11163ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (12.533224ms, 12.533224ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (8.529448ms, 8.529448ms)

tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, Arc<std::sync::Mutex<Entry>>>>
add_one_torrent: Avg/AdjAvg: (89ns, 89ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (6.823796ms, 6.823796ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (27.933274ms, 27.933274ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (8.692325ms, 8.310186ms)

tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, Arc<tokio::sync::Mutex<Entry>>>>
add_one_torrent: Avg/AdjAvg: (111ns, 107ns)
update_one_torrent_in_parallel: Avg/AdjAvg: (14.997225ms, 15.177959ms)
add_multiple_torrents_in_parallel: Avg/AdjAvg: (29.507577ms, 29.961334ms)
update_multiple_torrents_in_parallel: Avg/AdjAvg: (9.156453ms, 9.367261ms)

@mickvandijke
Copy link
Copy Markdown
Member Author

mickvandijke commented Dec 21, 2023

Hi @josecelano @da2ce7 ,

The pipeline is failing on a Docker job. Is it possible to make the Docker container use the Rust nightly channel to fix this?

@mickvandijke
Copy link
Copy Markdown
Member Author

mickvandijke commented Dec 21, 2023

Pipeline is failing on an existing error in our code. Probably because the newer nightly builds have introduced this change:

error: item in documentation is missing backticks
  --> contrib/bencode/src/reference/decode_opt.rs:44:90
   |
44 |     /// rest of the payload starts that wasn't decoded, get the bencode buffer, and call len().
   |                                                                                          ^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#doc_markdown
   = note: `-D clippy::doc-markdown` implied by `-D clippy::pedantic`
   = help: to override `-D clippy::pedantic` add `#[allow(clippy::doc_markdown)]`
help: try
   |
44 |     /// rest of the payload starts that wasn't decoded, get the bencode buffer, and call `len()`.
   |                                                                                          ~~~~~~~

I will make a PR for this

@mickvandijke mickvandijke mentioned this pull request Dec 21, 2023
Closed
@josecelano josecelano removed the Needs Rebase Base Branch has Incompatibilities label Dec 21, 2023
@josecelano josecelano merged commit 20b052d into torrust:develop Dec 21, 2023
@mickvandijke mickvandijke mentioned this pull request Jan 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@da2ce7 da2ce7 Awaiting requested review from da2ce7

@josecelano josecelano Awaiting requested review from josecelano

Assignees

No one assigned

Labels

None yet

Projects

Torrust Solution
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mickvandijke @da2ce7 @josecelano