Skip to content

New torrent repository: SkipMap for Peer List #794

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

New torrent repository: SkipMap for Peer List #794

wants to merge 2 commits into from

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Apr 15, 2024 •
edited
Loading

This is a new implementation of the torrent repository, where the peer list for a torrent entry is implemented using a SkipMap instead of a BTreeMap.

The performance is very bad:

Current implementation:

Requests out: 400847.83/second
Responses in: 361008.64/second
  - Connect responses:  178712.68
  - Announce responses: 178730.98
  - Scrape responses:   3564.99
  - 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: 293
  - p100: 363

This PR implementation:

Requests out: 39443.98/second
Responses in: 39443.38/second
  - Connect responses:  19505.14
  - Announce responses: 19556.91
  - Scrape responses:   381.33
  - Error responses:    0.00
Peers per announce response: 0.00
Announce responses per info hash:
  - p10: 1
  - p25: 1
  - p50: 1
  - p75: 1
  - p90: 7
  - p95: 15
  - p99: 31
  - p99.9: 41
  - p100: 51

I won't merge it because it makes the repository package much more complex.

This implementation uses a lock for the whole torrent entry. SkipMap would be a good choice if different entries in the peer list could be updated in parallel. However, that's not possible with this implementation, and I think it's not possible at all to use nested collections for the torrents and the torrent peer list.

@josecelano josecelano requested a review from da2ce7 April 15, 2024 14:31
@codecov Codecov
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

Attention: Patch coverage is 74.59459% with 47 lines in your changes are missing coverage. Please review.

Project coverage is 77.86%. Comparing base (52b7e3a) to head (4af6c0b).

Files Patch % Lines
packages/torrent-repository/src/entry/single.rs 65.45% 19 Missing ⚠️
...ckages/torrent-repository/src/entry/mutex_tokio.rs 0.00% 16 Missing ⚠️
packages/torrent-repository/src/entry/mutex_std.rs 60.00% 10 Missing ⚠️
...nt-repository/src/repository/skip_map_mutex_std.rs 96.49% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #794      +/-   ##
===========================================
- Coverage    78.15%   77.86%   -0.29%     
===========================================
  Files          159      159              
  Lines         8857     9001     +144     
===========================================
+ Hits          6922     7009      +87     
- Misses        1935     1992      +57

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

@josecelano josecelano closed this Apr 15, 2024
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

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@josecelano