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.

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

Default cache size documentation problem #4502

Closed
FranciscoPombal opened this issue Apr 3, 2020 · 4 comments
Closed

Default cache size documentation problem #4502

FranciscoPombal opened this issue Apr 3, 2020 · 4 comments

Comments

@FranciscoPombal
Copy link
Contributor

Please provide the following information

libtorrent version (or branch): RC_1_2

platform/architecture: N/A

compiler and compiler version: N/A

please describe what symptom you see, what you would expect to see instead and
how to reproduce it.

Related to qbittorrent/qBittorrent#12336.

The table in the documentation says (number of 16 KiB blocks):

name type default
cache_size int 2048

but in the code:

  • m_max_use is initialized like this to 64
  • if the amount of physical RAM cannot be determined, it is set to 1024 (the documentation text also states this further down):

    If the amount of physical RAM cannot be determined, it's set to 1024 (= 16 MiB).

In neither case does it default to 2048.

@arvidn
Copy link
Owner

arvidn commented Apr 4, 2020

The logic to set the cache size based on the amount of physical RAM is only triggered if settings_pack::cache_size is -1, right? and that's not default.

I don't see a code path where m_max_use is left as its initialized value of 64. In the disk_io_thread constructor, settings_updated() is called, specifically to trigger the initial value of m_max_use. It will in turn call block_cache::set_settings() which will call disk_buffer_pool::set_settings() which does this initialization.

As far as I can tell, m_max_use is indeed set to 2048 by default.

@arvidn
Copy link
Owner

arvidn commented Apr 4, 2020

I see, the qbt ticket is specifically about the auto case. I agree that the 1024 there should at least be the same as the regular default.

@arvidn
Copy link
Owner

arvidn commented Apr 30, 2020

@FranciscoPombal is there any action I should take on this ticket still?

@FranciscoPombal
Copy link
Contributor Author

@arvidn
I think everything has been addressed, thanks. Relevant commit for future reference: 6c88015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants