Add a configurable memory limit to the torrent repository and implement dashmap [#567, #565] #645
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mickvandijke
had a problem deploying
to
coverage
GitHub Actions
Failure
|
Needs rebase and commit cleanup first before reviewing. |
This was
linked to
issues
Jan 25, 2024
mickvandijke
had a problem deploying
to
coverage
GitHub Actions
Failure
mickvandijke
had a problem deploying
to
coverage
GitHub Actions
Failure
… and set as used one
mickvandijke
changed the title
da2ce7
reviewed
Feb 1, 2024
| @@ -476,32 +393,47 @@ impl Repository for RepositoryDashmap { | |||
| fn upsert_torrent_with_peer_and_get_stats(&self, info_hash: &InfoHash, peer: &peer::Peer) -> (SwarmStats, bool) { | |||
| let hash = self.torrents.hash_usize(&info_hash); | |||
| let shard_idx = self.torrents.determine_shard(hash); | |||
| let mut shard = unsafe { self.torrents._yield_write_shard(shard_idx) }; | |||
There was a problem hiding this comment.
it is good to explain why unsafe code is needed, and why this use is safe
mickvandijke
force-pushed
the
sharded-priority-map
branch
from
2bda637 to
04e4f8f
Compare
mickvandijke
had a problem deploying
to
coverage
GitHub Actions
Failure
|
Hi @mickvandijke what is the status of this PR? Maybe: By the way, why did you use deepsize? I'm just curious, I've found this one https://github.com/str4d/memuse |
josecelano
added a commit
that referenced
this pull request
Apr 9, 2024
… using `DashMap` 4030fd1 fix: torrent repository tests. DashMap is not ordered (Jose Celano) 1e76c17 chore: add dashmap cargo dep to cargp machete (Jose Celano) 00ee9db feat: [#565] new torrent repository implementation usind DashMap (Jose Celano) 78b46c4 chore(deps): add cargo dependency: dashmap (Jose Celano) Pull request description: Relates to: - #778 - #567 (comment) This PR adds a new torrent repository implementation where the outer collection for torrents uses a [DashMap](https://docs.rs/dashmap/latest/dashmap/). This is something @mickvandijke was working on this [PR](#645). There have been many changes. @da2ce7 has extracted a [package for the repositories](https://github.com/torrust/torrust-tracker/tree/develop/packages/torrent-repository). This PR adds a new repo using DashMap to the new package. However, it does not implement some of the extra features @mickvandijke added to the other [PR](#645). For example, it does not limit memory consumption. None of the other repos have that feature, so I suggest merging this PR and implementing that feature in the future for all repos. ### Why The current repository used in production is the one using a [SkipMap](https://docs.rs/crossbeam-skiplist/latest/crossbeam_skiplist/struct.SkipMap.html) data structure. DashMap was the first type we considered to allow adding new torrents in parallels. The implementation with DashMap has not been merged because it does not guarantee the order when you iterate over the torrents. The tracker API returns torrents ordered by InfoHash. To avoid breaking the API that PR would need to add a new data structure (kind of Index) to keep the torrent list ordered. This implementation helps us run performance tests with this option without spending much time fixing its limitations. It looks like the performance is similar to the SkiMap, so we don't need to use it in production now. We only need to keep it for benchmarking in the future if other versions are better. ### Benchmarking Running the Aquatic UDP load test, the DashMap looks slightly better. 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 ``` DashMap with initial capacity 0 (best result. On average is lower, similar to SkipMap): ```output Requests out: 410658.38/second Responses in: 365892.86/second - Connect responses: 181258.91 - Announce responses: 181005.95 - Scrape responses: 3628.00 - 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: 295 - p100: 363 ``` With Criterion:     ### Conclusion From my point of view, other [performance optimisations](#774) have more potential than this, considering this implementation is not finished. The changes needed to finish this implementation will probably decrease the performance obtained in this benchmarking. In the future, we can review this option if we change the [tracker API behaviour to getting all torrents](#775). ACKs for top commit: josecelano: ACK 4030fd1 Tree-SHA512: 1a6c56f3aecb34fc40c401597efe1663c3cc66903f2d27bf8f2bbc6b058080d487a89b705c224657aaa6059f1c2a8597583e636e30727f9293bb43f460441415
|
Hi @mickvandijke I'm closing this PR: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #567
Closes #565