Skip to content
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

Add a configurable memory limit to the torrent repository #567

Closed
mickvandijke opened this issue Jan 2, 2024 · 7 comments
Closed

Add a configurable memory limit to the torrent repository #567

mickvandijke opened this issue Jan 2, 2024 · 7 comments
Assignees
@mickvandijke
Labels
Enhancement / Feature Request Something New Hard Non-Trivial
Milestone
v3.0.0

Comments

@mickvandijke
Copy link
Member

mickvandijke commented Jan 2, 2024 •
edited
Loading

Currently the torrent repository is not bound by an upper limit of torrents or memory usage. When the memory size of the torrent repository exceeds the amount of memory available on the host system, the tracker will restart.

To solve this problem, I want to add a configurable memory limit for the torrent repository. When the torrent repository is full, it should let new torrents replace old low priority torrents. For the first implementation I will base the priority on the time it was last accessed in an announce request.

Relevant discussions/issues

#499
#566

Needs issues

#565

cc @josecelano @da2ce7
@mickvandijke mickvandijke self-assigned this Jan 2, 2024
@josecelano
Copy link
Member

Hi, @WarmBeer I think an explicit limit is a nice feature but it would also be good to set a no-limit option so the tracker can consume as much memory as possible without panicking.

@josecelano josecelano mentioned this issue Jan 2, 2024
Closed
@mickvandijke
Copy link
Member Author

Hi, @WarmBeer I think an explicit limit is a nice feature but it would also be good to set a no-limit option so the tracker can consume as much memory as possible without panicking.

Will add a limit of 0 that will allow the torrent repository to grow until there is no more space available.

@josecelano
Copy link
Member

josecelano commented Jan 3, 2024 •
edited
Loading

Hi, @WarmBeer I think an explicit limit is a nice feature but it would also be good to set a no-limit option so the tracker can consume as much memory as possible without panicking.

Will add a limit of 0 that will allow the torrent repository to grow until there is no more space available.

@WarmBeer maybe -1 it's a better option.

ChatGPT:

When choosing a value to represent 'no limit' for a configuration option like memory_usage_limit in an application configuration file, it's essential to consider clarity, standard practices, and the potential for misinterpretation. Let's evaluate the options you've provided:

0: Typically, '0' implies no allocation or disabling the feature. In the context of memory usage, it might be misunderstood as not allowing the application to use any memory, which could confuse users.

-1: This is a common convention in many programming and configuration contexts to represent 'unlimited' or 'no limit'. It's widely recognized by developers and system administrators, making it a good choice for clarity and standard practice.

null: Using 'null' can be a good option, as it explicitly represents the absence of a value. However, it might require additional handling in the code to interpret 'null' as 'no limit' and not as an uninitialized or error state.

Empty (no value): An empty value might lead to confusion or be interpreted as a misconfiguration. It's less explicit than other options and might require additional documentation for users to understand its meaning.

"" (empty string): Similar to an empty value, an empty string can be ambiguous and might be interpreted as a configuration error. It's not a standard way to represent 'no limit' in a numerical context.

Considering these points, -1 seems to be the best choice. It's a widely recognized convention for 'no limit' in various programming and configuration contexts. It is explicit and less likely to be misinterpreted compared to the other options. However, it's important to clearly document this in the configuration file or accompanying documentation so that users understand that setting memory_usage_limit to -1 means there is no limit on memory usage.

@mickvandijke mickvandijke mentioned this issue Jan 11, 2024
Closed
@josecelano josecelano mentioned this issue Jan 11, 2024
Closed
@cgbosse cgbosse added this to the v3.0.0 milestone Jan 12, 2024
@mickvandijke mickvandijke added the Hard Non-Trivial label Jan 18, 2024
@mickvandijke mickvandijke mentioned this issue Jan 25, 2024
Closed
@mickvandijke mickvandijke linked a pull request Jan 25, 2024 that will close this issue
Add a configurable memory limit to the torrent repository and implement dashmap [#567, #565] #645
Closed
@josecelano josecelano mentioned this issue Feb 6, 2024
Closed
@josecelano
Copy link
Member

Relates to: #646

@da2ce7
Copy link
Contributor

da2ce7 commented Mar 25, 2024

Now that #690 has been merged, updating the torrent repository api to include memory tracking should be more simple.

@josecelano josecelano mentioned this issue Apr 9, 2024
Merged
josecelano added a commit that referenced this issue Apr 9, 2024
@josecelano
Merge #784: Performance optimization: create a new torrent repository…
b5fb03b 
… 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:

  ![image](https://github.com/torrust/torrust-tracker/assets/58816/1e1fca2d-821d-4e2c-adf8-49e055758bd0)

  ![image](https://github.com/torrust/torrust-tracker/assets/58816/fcb9a32f-c4f1-4ffd-9797-4a74fc000336)

  ![image](https://github.com/torrust/torrust-tracker/assets/58816/efe2d788-31ac-4997-82f6-d022aa8f79a0)

  ![image](https://github.com/torrust/torrust-tracker/assets/58816/f64a4e5a-a363-48cd-87d9-78162cda11d4)

  ### 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
@josecelano
Copy link
Member

I suggest closing this issue and opening a new proposal in discussions to implement this memory limit for all torrent repositories. When the issue was open, the intention was to add this memory limit only to the new DashMap implementation @mickvandijke was working on. However, I think we all agree we should implement it for all the repository implementations.

This is also related to @mickvandijke's proposal:

#499

I guess that proposal is what you wanted to implement in this issue. And I think you implemented it in this PR. Rigth @mickvandijke?

@josecelano
Copy link
Member

I suggest closing this issue and opening a new proposal in discussions to implement this memory limit for all torrent repositories. When the issue was open, the intention was to add this memory limit only to the new DashMap implementation @mickvandijke was working on. However, I think we all agree we should implement it for all the repository implementations.

This is also related to @mickvandijke's proposal:

#499

I guess that proposal is what you wanted to implement in this issue. And I think you implemented it in this PR. Rigth @mickvandijke?

I've opened the new discussion: #789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@mickvandijke mickvandijke

Labels
Enhancement / Feature Request Something New Hard Non-Trivial
Projects
Torrust Solution
Status: Done
Milestone
v3.0.0
4 participants
@josecelano @da2ce7 @mickvandijke @cgbosse