Performance optimization: create a new torrent repository using a SkipMap instead of a BTreeMap

[josecelano]

This PR implements a new Torrent Repository replacing the outer BTreeMap for torrents with a SkipMap.

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:

  
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:

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.

Conclusion

I think we car merge it but I would also continue with the DashMap implementation to compare.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.84%. Comparing base (2277099) to head (12f54e7).

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #778      +/-   ##
===========================================
+ Coverage    77.71%   77.84%   +0.12%     
===========================================
  Files          158      158              
  Lines         8711     8762      +51     
===========================================
+ Hits          6770     6821      +51     
  Misses        1941     1941              

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

[da2ce7]

Looks good, but it would be good to hook it up to the integration tests as well.

[josecelano]

Looks good, but it would be good to hook it up to the integration tests as well.

OK. I did not see that.

[josecelano]

ACK 12f54e7

[josecelano]

Hi @da2ce7, I've added the new repo implementation to the tests. I will merge as it is, but since the new repo does not use a BTReeMap for the outer collection and we don't need the outer lock, I think we should rename the repos in the enum and fixtures.

The current names are:

pub(crate) enum Repo {
    RwLockStd(TorrentsRwLockStd),
    RwLockStdMutexStd(TorrentsRwLockStdMutexStd),
    RwLockStdMutexTokio(TorrentsRwLockStdMutexTokio),
    RwLockTokio(TorrentsRwLockTokio),
    RwLockTokioMutexStd(TorrentsRwLockTokioMutexStd),
    RwLockTokioMutexTokio(TorrentsRwLockTokioMutexTokio),
    SkipMapMutexStd(TorrentsSkipMapMutexStd),
}

Including the new implementation, we always have a pattern like this:

[STD_RW_LOCK|TOKIO_RW_LOCK] -> OUTER_COLLECTION -> INNER_COLLECTION -> [STD_MUTEX|TOKIO_MUTEX -> [ARC] -> [STD_LOCK|TOKIO_LOCK] -> ENTRY

OUTER_COLLECTION: torrents
INNER_COLLECTION: peer list
[]: optional

We could rename them to:

pub(crate) enum Repo {
    //          Outer     Inner
    RwLockStd___BTreeMap__BTreeMap_None_________(TorrentsRwLockStd),
    RwLockStd___BTreeMap__BTreeMap_ArcMutexStd__(TorrentsRwLockStdMutexStd),
    RwLockStd___BTreeMap__BTreeMap_ArcMutexTokio(TorrentsRwLockStdMutexTokio),
    RwLockTokio_BTreeMap__BTreeMap_None_________(TorrentsRwLockTokio),
    RwLockTokio_BTreeMap__BTreeMap_ArcMutexStd__(TorrentsRwLockTokioMutexStd),
    RwLockTokio_BTreeMap__BTreeMap_ArcMutexTokio(TorrentsRwLockTokioMutexTokio),
    None________SkipMap___BTreeMap_ArcMutexStd__(TorrentsSkipMapMutexStd),
}

Underscores and "None" are to highlight the pattern.

Or just:

pub(crate) enum Repo {
    //          Outer     Inner
    Impl1(TorrentsRwLockStd),
    Impl2(TorrentsRwLockStdMutexStd),
    Impl3(TorrentsRwLockStdMutexTokio),
    Impl4(TorrentsRwLockTokio),
    Impl5(TorrentsRwLockTokioMutexStd),
    Impl6(TorrentsRwLockTokioMutexTokio),
    Impl7(TorrentsSkipMapMutexStd),
}

A generic name will be easier to maintain.

An option in the middle could be:

pub(crate) enum Repo {
    //  Outer and Inner Collections
    DoubleBTreeMapImpl1(TorrentsRwLockStd),
    DoubleBTreeMapImpl1(TorrentsRwLockStdMutexStd),
    DoubleBTreeMapImpl1(TorrentsRwLockStdMutexTokio),
    DoubleBTreeMapImpl1(TorrentsRwLockTokio),
    DoubleBTreeMapImpl1(TorrentsRwLockTokioMutexStd),
    DoubleBTreeMapImpl1(TorrentsRwLockTokioMutexTokio),
    SkipMapBTreeMap(TorrentsSkipMapMutexStd),
}

Just mention the collections types and remove the lock types.