Skip to content

Commit 6eff113

Browse files
committed
Merge #778: Performance optimization: create a new torrent repository using a SkipMap instead of a BTreeMap
12f54e7 test: add tests for new torrent repository using SkipMap (Jose Celano) 0989285 refactor: separate torrent repository trait from implementations (Jose Celano) eec2024 chore: ignore crossbeam-skiplist crate in cargo-machete (Jose Celano) 642d6be feat: new torrent repository using crossbeam_skiplist::SkipMap (Jose Celano) 608585e chore: add new cargo dependency: crossbeam-skiplist (Jose Celano) Pull request description: This PR implements a new Torrent Repository replacing the outer BTreeMap for torrents with a [SkipMap](https://docs.rs/crossbeam-skiplist/latest/crossbeam_skiplist/). ### Why - It is straightforward to implement because the API is very similar. In fact, SkipMap is a replacement for BTreeMap that allows concurrency in adding new torrents. - One problem with BTreeMap is that you have to lock the entire structure to add a new torrent. SkipMap is a lock-free structure. - It is a lock-free structure that uses Atomics internally, which is more lightweight than locks. - Unlike the DashMap implementation, it does not use unsafe code. However: > NOTICE: Race conditions could be introduced if the implementation was incorrect. See https://docs.rs/crossbeam-skiplist/latest/crossbeam_skiplist/#concurrent-access ### Benchmarking Running the Aquatic UDP load test gives the same results: Current best implementation: ```output Requests out: 397287.37/second Responses in: 357549.15/second - Connect responses: 177073.94 - Announce responses: 176905.36 - Scrape responses: 3569.85 - Error responses: 0.00 Peers per announce response: 0.00 Announce responses per info hash: - p10: 1 - p25: 1 - p50: 1 - p75: 1 - p90: 2 - p95: 3 - p99: 104 - p99.9: 287 - p100: 371 ``` SkipMap: ```output Requests out: 396788.68/second Responses in: 357105.27/second - Connect responses: 176662.91 - Announce responses: 176863.44 - Scrape responses: 3578.91 - Error responses: 0.00 Peers per announce response: 0.00 Announce responses per info hash: - p10: 1 - p25: 1 - p50: 1 - p75: 1 - p90: 2 - p95: 3 - p99: 105 - p99.9: 287 - p100: 351 ``` However, benchmarking the repositories using Criterion shows better results in adding and updating multiple torrents in parallel. ![image](https://github.com/torrust/torrust-tracker/assets/58816/4f0736f1-3e70-4a55-8084-3265e9cd087d) ![image](https://github.com/torrust/torrust-tracker/assets/58816/a9c7a034-dbd3-40a0-85d5-c884f1286964) ![image](https://github.com/torrust/torrust-tracker/assets/58816/a3a8f37c-4230-4cfb-9adb-2c4d72ab9fd7) ![image](https://github.com/torrust/torrust-tracker/assets/58816/43a2d9cb-ccbc-496c-9d57-a98887f1575c) ### Conclusion I think we car merge it but I would also continue with the DashMap implementation to compare. ACKs for top commit: josecelano: ACK 12f54e7 Tree-SHA512: 0f300360abe5f70cef21bb5c583ecadedd5a1b233125951d90ffe510e551a9ee7b41ba6dacd23176656fb18095645fdde24a23c23d901e964300e3e1257b3b71
2 parents 2277099 + 12f54e7 commit 6eff113

File tree

13 files changed

+388
-131
lines changed

13 files changed

+388
-131
lines changed

Cargo.lock

+12
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

Cargo.toml

+4-3
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ chrono = { version = "0", default-features = false, features = ["clock"] }
4141
clap = { version = "4", features = ["derive", "env"] }
4242
colored = "2"
4343
config = "0"
44+
crossbeam-skiplist = "0.1"
4445
derive_more = "0"
4546
fern = "0"
4647
futures = "0"
@@ -63,8 +64,8 @@ serde_json = "1"
6364
serde_repr = "0"
6465
thiserror = "1"
6566
tokio = { version = "1", features = ["macros", "net", "rt-multi-thread", "signal", "sync"] }
66-
torrust-tracker-configuration = { version = "3.0.0-alpha.12-develop", path = "packages/configuration" }
6767
torrust-tracker-clock = { version = "3.0.0-alpha.12-develop", path = "packages/clock" }
68+
torrust-tracker-configuration = { version = "3.0.0-alpha.12-develop", path = "packages/configuration" }
6869
torrust-tracker-contrib-bencode = { version = "3.0.0-alpha.12-develop", path = "contrib/bencode" }
6970
torrust-tracker-located-error = { version = "3.0.0-alpha.12-develop", path = "packages/located-error" }
7071
torrust-tracker-primitives = { version = "3.0.0-alpha.12-develop", path = "packages/primitives" }
@@ -76,7 +77,7 @@ url = "2"
7677
uuid = { version = "1", features = ["v4"] }
7778

7879
[package.metadata.cargo-machete]
79-
ignored = ["serde_bytes"]
80+
ignored = ["serde_bytes", "crossbeam-skiplist"]
8081

8182
[dev-dependencies]
8283
local-ip-address = "0"
@@ -105,4 +106,4 @@ opt-level = 3
105106

106107
[profile.release-debug]
107108
inherits = "release"
108-
debug = true
109+
debug = true

cSpell.json

+1
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,7 @@
135135
"Shareaza",
136136
"sharktorrent",
137137
"SHLVL",
138+
"skiplist",
138139
"socketaddr",
139140
"sqllite",
140141
"subsec",

packages/torrent-repository/Cargo.toml

+3-2
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,12 @@ rust-version.workspace = true
1616
version.workspace = true
1717

1818
[dependencies]
19+
crossbeam-skiplist = "0.1"
1920
futures = "0.3.29"
2021
tokio = { version = "1", features = ["macros", "net", "rt-multi-thread", "signal", "sync"] }
21-
torrust-tracker-primitives = { version = "3.0.0-alpha.12-develop", path = "../primitives" }
22-
torrust-tracker-configuration = { version = "3.0.0-alpha.12-develop", path = "../configuration" }
2322
torrust-tracker-clock = { version = "3.0.0-alpha.12-develop", path = "../clock" }
23+
torrust-tracker-configuration = { version = "3.0.0-alpha.12-develop", path = "../configuration" }
24+
torrust-tracker-primitives = { version = "3.0.0-alpha.12-develop", path = "../primitives" }
2425

2526
[dev-dependencies]
2627
criterion = { version = "0", features = ["async_tokio"] }

packages/torrent-repository/benches/repository_benchmark.rs

+20-1
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ mod helpers;
55
use criterion::{criterion_group, criterion_main, Criterion};
66
use torrust_tracker_torrent_repository::{
77
TorrentsRwLockStd, TorrentsRwLockStdMutexStd, TorrentsRwLockStdMutexTokio, TorrentsRwLockTokio, TorrentsRwLockTokioMutexStd,
8-
TorrentsRwLockTokioMutexTokio,
8+
TorrentsRwLockTokioMutexTokio, TorrentsSkipMapMutexStd,
99
};
1010

1111
use crate::helpers::{asyn, sync};
@@ -45,6 +45,10 @@ fn add_one_torrent(c: &mut Criterion) {
4545
.iter_custom(asyn::add_one_torrent::<TorrentsRwLockTokioMutexTokio, _>);
4646
});
4747

48+
group.bench_function("SkipMapMutexStd", |b| {
49+
b.iter_custom(sync::add_one_torrent::<TorrentsSkipMapMutexStd, _>);
50+
});
51+
4852
group.finish();
4953
}
5054

@@ -89,6 +93,11 @@ fn add_multiple_torrents_in_parallel(c: &mut Criterion) {
8993
.iter_custom(|iters| asyn::add_multiple_torrents_in_parallel::<TorrentsRwLockTokioMutexTokio, _>(&rt, iters, None));
9094
});
9195

96+
group.bench_function("SkipMapMutexStd", |b| {
97+
b.to_async(&rt)
98+
.iter_custom(|iters| sync::add_multiple_torrents_in_parallel::<TorrentsSkipMapMutexStd, _>(&rt, iters, None));
99+
});
100+
92101
group.finish();
93102
}
94103

@@ -133,6 +142,11 @@ fn update_one_torrent_in_parallel(c: &mut Criterion) {
133142
.iter_custom(|iters| asyn::update_one_torrent_in_parallel::<TorrentsRwLockTokioMutexTokio, _>(&rt, iters, None));
134143
});
135144

145+
group.bench_function("SkipMapMutexStd", |b| {
146+
b.to_async(&rt)
147+
.iter_custom(|iters| sync::update_one_torrent_in_parallel::<TorrentsSkipMapMutexStd, _>(&rt, iters, None));
148+
});
149+
136150
group.finish();
137151
}
138152

@@ -178,6 +192,11 @@ fn update_multiple_torrents_in_parallel(c: &mut Criterion) {
178192
});
179193
});
180194

195+
group.bench_function("SkipMapMutexStd", |b| {
196+
b.to_async(&rt)
197+
.iter_custom(|iters| sync::update_multiple_torrents_in_parallel::<TorrentsSkipMapMutexStd, _>(&rt, iters, None));
198+
});
199+
181200
group.finish();
182201
}
183202

packages/torrent-repository/src/lib.rs

+11-6
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
use std::sync::Arc;
22

3+
use repository::rw_lock_std::RwLockStd;
4+
use repository::rw_lock_tokio::RwLockTokio;
5+
use repository::skip_map_mutex_std::CrossbeamSkipList;
36
use torrust_tracker_clock::clock;
47

58
pub mod entry;
@@ -9,12 +12,14 @@ pub type EntrySingle = entry::Torrent;
912
pub type EntryMutexStd = Arc<std::sync::Mutex<entry::Torrent>>;
1013
pub type EntryMutexTokio = Arc<tokio::sync::Mutex<entry::Torrent>>;
1114

12-
pub type TorrentsRwLockStd = repository::RwLockStd<EntrySingle>;
13-
pub type TorrentsRwLockStdMutexStd = repository::RwLockStd<EntryMutexStd>;
14-
pub type TorrentsRwLockStdMutexTokio = repository::RwLockStd<EntryMutexTokio>;
15-
pub type TorrentsRwLockTokio = repository::RwLockTokio<EntrySingle>;
16-
pub type TorrentsRwLockTokioMutexStd = repository::RwLockTokio<EntryMutexStd>;
17-
pub type TorrentsRwLockTokioMutexTokio = repository::RwLockTokio<EntryMutexTokio>;
15+
pub type TorrentsRwLockStd = RwLockStd<EntrySingle>;
16+
pub type TorrentsRwLockStdMutexStd = RwLockStd<EntryMutexStd>;
17+
pub type TorrentsRwLockStdMutexTokio = RwLockStd<EntryMutexTokio>;
18+
pub type TorrentsRwLockTokio = RwLockTokio<EntrySingle>;
19+
pub type TorrentsRwLockTokioMutexStd = RwLockTokio<EntryMutexStd>;
20+
pub type TorrentsRwLockTokioMutexTokio = RwLockTokio<EntryMutexTokio>;
21+
22+
pub type TorrentsSkipMapMutexStd = CrossbeamSkipList<EntryMutexStd>;
1823

1924
/// This code needs to be copied into each crate.
2025
/// Working version, for production.

packages/torrent-repository/src/repository/mod.rs

+1-34
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ pub mod rw_lock_std_mutex_tokio;
1111
pub mod rw_lock_tokio;
1212
pub mod rw_lock_tokio_mutex_std;
1313
pub mod rw_lock_tokio_mutex_tokio;
14+
pub mod skip_map_mutex_std;
1415

1516
use std::fmt::Debug;
1617

@@ -40,37 +41,3 @@ pub trait RepositoryAsync<T>: Debug + Default + Sized + 'static {
4041
peer: &peer::Peer,
4142
) -> impl std::future::Future<Output = (bool, SwarmMetadata)> + Send;
4243
}
43-
44-
#[derive(Default, Debug)]
45-
pub struct RwLockStd<T> {
46-
torrents: std::sync::RwLock<std::collections::BTreeMap<InfoHash, T>>,
47-
}
48-
49-
#[derive(Default, Debug)]
50-
pub struct RwLockTokio<T> {
51-
torrents: tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, T>>,
52-
}
53-
54-
impl<T> RwLockStd<T> {
55-
/// # Panics
56-
///
57-
/// Panics if unable to get a lock.
58-
pub fn write(
59-
&self,
60-
) -> std::sync::RwLockWriteGuard<'_, std::collections::BTreeMap<torrust_tracker_primitives::info_hash::InfoHash, T>> {
61-
self.torrents.write().expect("it should get lock")
62-
}
63-
}
64-
65-
impl<T> RwLockTokio<T> {
66-
pub fn write(
67-
&self,
68-
) -> impl std::future::Future<
69-
Output = tokio::sync::RwLockWriteGuard<
70-
'_,
71-
std::collections::BTreeMap<torrust_tracker_primitives::info_hash::InfoHash, T>,
72-
>,
73-
> {
74-
self.torrents.write()
75-
}
76-
}

packages/torrent-repository/src/repository/rw_lock_std.rs

+16
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,22 @@ use super::Repository;
1111
use crate::entry::Entry;
1212
use crate::{EntrySingle, TorrentsRwLockStd};
1313

14+
#[derive(Default, Debug)]
15+
pub struct RwLockStd<T> {
16+
pub(crate) torrents: std::sync::RwLock<std::collections::BTreeMap<InfoHash, T>>,
17+
}
18+
19+
impl<T> RwLockStd<T> {
20+
/// # Panics
21+
///
22+
/// Panics if unable to get a lock.
23+
pub fn write(
24+
&self,
25+
) -> std::sync::RwLockWriteGuard<'_, std::collections::BTreeMap<torrust_tracker_primitives::info_hash::InfoHash, T>> {
26+
self.torrents.write().expect("it should get lock")
27+
}
28+
}
29+
1430
impl TorrentsRwLockStd {
1531
fn get_torrents<'a>(&'a self) -> std::sync::RwLockReadGuard<'a, std::collections::BTreeMap<InfoHash, EntrySingle>>
1632
where

packages/torrent-repository/src/repository/rw_lock_tokio.rs

+18
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,24 @@ use super::RepositoryAsync;
1111
use crate::entry::Entry;
1212
use crate::{EntrySingle, TorrentsRwLockTokio};
1313

14+
#[derive(Default, Debug)]
15+
pub struct RwLockTokio<T> {
16+
pub(crate) torrents: tokio::sync::RwLock<std::collections::BTreeMap<InfoHash, T>>,
17+
}
18+
19+
impl<T> RwLockTokio<T> {
20+
pub fn write(
21+
&self,
22+
) -> impl std::future::Future<
23+
Output = tokio::sync::RwLockWriteGuard<
24+
'_,
25+
std::collections::BTreeMap<torrust_tracker_primitives::info_hash::InfoHash, T>,
26+
>,
27+
> {
28+
self.torrents.write()
29+
}
30+
}
31+
1432
impl TorrentsRwLockTokio {
1533
async fn get_torrents<'a>(&'a self) -> tokio::sync::RwLockReadGuard<'a, std::collections::BTreeMap<InfoHash, EntrySingle>>
1634
where
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,106 @@
1+
use std::collections::BTreeMap;
2+
use std::sync::Arc;
3+
4+
use crossbeam_skiplist::SkipMap;
5+
use torrust_tracker_configuration::TrackerPolicy;
6+
use torrust_tracker_primitives::info_hash::InfoHash;
7+
use torrust_tracker_primitives::pagination::Pagination;
8+
use torrust_tracker_primitives::swarm_metadata::SwarmMetadata;
9+
use torrust_tracker_primitives::torrent_metrics::TorrentsMetrics;
10+
use torrust_tracker_primitives::{peer, DurationSinceUnixEpoch, PersistentTorrents};
11+
12+
use super::Repository;
13+
use crate::entry::{Entry, EntrySync};
14+
use crate::{EntryMutexStd, EntrySingle};
15+
16+
#[derive(Default, Debug)]
17+
pub struct CrossbeamSkipList<T> {
18+
pub torrents: SkipMap<InfoHash, T>,
19+
}
20+
21+
impl Repository<EntryMutexStd> for CrossbeamSkipList<EntryMutexStd>
22+
where
23+
EntryMutexStd: EntrySync,
24+
EntrySingle: Entry,
25+
{
26+
fn update_torrent_with_peer_and_get_stats(&self, info_hash: &InfoHash, peer: &peer::Peer) -> (bool, SwarmMetadata) {
27+
let entry = self.torrents.get_or_insert(*info_hash, Arc::default());
28+
entry.value().insert_or_update_peer_and_get_stats(peer)
29+
}
30+
31+
fn get(&self, key: &InfoHash) -> Option<EntryMutexStd> {
32+
let maybe_entry = self.torrents.get(key);
33+
maybe_entry.map(|entry| entry.value().clone())
34+
}
35+
36+
fn get_metrics(&self) -> TorrentsMetrics {
37+
let mut metrics = TorrentsMetrics::default();
38+
39+
for entry in &self.torrents {
40+
let stats = entry.value().lock().expect("it should get a lock").get_stats();
41+
metrics.complete += u64::from(stats.complete);
42+
metrics.downloaded += u64::from(stats.downloaded);
43+
metrics.incomplete += u64::from(stats.incomplete);
44+
metrics.torrents += 1;
45+
}
46+
47+
metrics
48+
}
49+
50+
fn get_paginated(&self, pagination: Option<&Pagination>) -> Vec<(InfoHash, EntryMutexStd)> {
51+
match pagination {
52+
Some(pagination) => self
53+
.torrents
54+
.iter()
55+
.skip(pagination.offset as usize)
56+
.take(pagination.limit as usize)
57+
.map(|entry| (*entry.key(), entry.value().clone()))
58+
.collect(),
59+
None => self
60+
.torrents
61+
.iter()
62+
.map(|entry| (*entry.key(), entry.value().clone()))
63+
.collect(),
64+
}
65+
}
66+
67+
fn import_persistent(&self, persistent_torrents: &PersistentTorrents) {
68+
for (info_hash, completed) in persistent_torrents {
69+
if self.torrents.contains_key(info_hash) {
70+
continue;
71+
}
72+
73+
let entry = EntryMutexStd::new(
74+
EntrySingle {
75+
peers: BTreeMap::default(),
76+
downloaded: *completed,
77+
}
78+
.into(),
79+
);
80+
81+
// Since SkipMap is lock-free the torrent could have been inserted
82+
// after checking if it exists.
83+
self.torrents.get_or_insert(*info_hash, entry);
84+
}
85+
}
86+
87+
fn remove(&self, key: &InfoHash) -> Option<EntryMutexStd> {
88+
self.torrents.remove(key).map(|entry| entry.value().clone())
89+
}
90+
91+
fn remove_inactive_peers(&self, current_cutoff: DurationSinceUnixEpoch) {
92+
for entry in &self.torrents {
93+
entry.value().remove_inactive_peers(current_cutoff);
94+
}
95+
}
96+
97+
fn remove_peerless_torrents(&self, policy: &TrackerPolicy) {
98+
for entry in &self.torrents {
99+
if entry.value().is_good(policy) {
100+
continue;
101+
}
102+
103+
entry.remove();
104+
}
105+
}
106+
}

0 commit comments

Comments
 (0)