Add checking_mem_usage option to AdvancedSettings #9373

FranciscoPombal · 2018-08-19T14:25:59Z

Adds a spinbox in advanced options to change the libtorrent setting
checking_mem_usage.

Potential usefulness: refer to discussion at #9061

Chocobo1 · 2018-08-19T14:48:53Z

src/gui/advancedsettings.h

@@ -59,7 +59,7 @@ private slots:
    template <typename T> void addRow(int row, const QString &rowText, T *widget);

    QLabel labelQbtLink, labelLibtorrentLink;
-    QSpinBox spinBoxAsyncIOThreads, spinBoxCache, spinBoxSaveResumeDataInterval, spinBoxOutgoingPortsMin, spinBoxOutgoingPortsMax, spinBoxListRefresh, spinBoxMaxHalfOpen,
+    QSpinBox spinBoxCheckingMemUsage, spinBoxAsyncIOThreads, spinBoxCache, spinBoxSaveResumeDataInterval, spinBoxOutgoingPortsMin, spinBoxOutgoingPortsMax, spinBoxListRefresh, spinBoxMaxHalfOpen,


If you're placing the new option after "async IO threads", please do it here as well.

Ok, will do.

Chocobo1 · 2018-08-19T14:52:57Z

src/gui/advancedsettings.cpp

+    spinBoxCheckingMemUsage.setMinimum(1);
+    spinBoxCheckingMemUsage.setMaximum(256000);
+    spinBoxCheckingMemUsage.setValue(session->checkingMemUsage());
+    addRow(CHECKING_MEM_USAGE, tr("Checking memory usage"), &spinBoxCheckingMemUsage);


I think the name a bit too vague, maybe "Outstanding memory blocks when checking torrents".

Agreed, but wouldn't it also be useful to also mention that each block is 16 KiB? Example:
"Outstanding 16 KiB memory blocks when checking torrents"
or
"Outstanding memory when checking torrents (16 KiB blocks)"
or
"Outstanding memory (16 KiB blocks) when checking torrents"

Or is it too much information/confusing?

Agreed, but wouldn't it also be useful to also mention that each block is 16 KiB?

Sure, or even better: let users specify in MiB instead of number of blocks, sort of like the "Disk cache" option.

Ok, I will make that change and set the text to "Outstanding memory when checking torrents"

Chocobo1 · 2018-08-19T14:57:44Z

Recently I saw libtorrent has increased its default value to 1024, do you still see improvements when you tune this number even higher? Just curious.

FranciscoPombal · 2018-08-19T15:05:07Z

Recently I saw libtorrent has increased its default value to 1024, do you still see improvements when you tune this number even higher? Just curious.

Currently I do not have a machine that I can test the effect of this setting on. However, according to #9061, rechecking speeds have increased as a result of recent patches to libtorrent, but not to optimal levels (as I understand it).

This PR will make it easier for people to test rechecking speed while fiddling with async_io_threads and checking_mem_usage without having to recompile each time they change a value.

Even if it turns out that 1024 is not the ideal default value for qBittorrent, we will now be able to easily change it.

Chocobo1 · 2018-08-19T15:14:09Z

Even if it turns out that 1024 is not the ideal default value for qBittorrent, we will now be able to easily change it.

Let's just follow libtorrent default for now.

FranciscoPombal · 2018-08-19T15:36:54Z

Let's just follow libtorrent default for now.

Sounds good.

FranciscoPombal · 2018-08-19T16:37:04Z

@Chocobo1 Addressed review comments.

I need advice on the maximum allowed value for checking memory for 32-bit architectures. Currently, I set it to 128 MiB, because as I understand it, this will be competing with disk cache + program data for 2 GiB of RAM.

The max for 64-bit architectures is currently 4 GiB as previously (~256000 16 KiB blocks). Is this value sane or should I set it higher?

LordNyriox · 2018-08-19T18:30:33Z

@FranciscoPombal: Some systems have more memory than others.

Setting the max to 32 or 64 GB (the highest RAM consumer systems can generally use)—accompanied by a warning tooltip that this value needs to be significantly less than system memory—seems reasonable to me.

FranciscoPombal · 2018-08-19T20:39:57Z

@LordNyriox
Thanks for the feedback.
I am interested in the idea of tooltips.
However, I would take care of that in another PR, in order to both deliver this one swiftly but also to broaden its scope: I think all options, especially the advanced ones need a tooltip with snippets of documentation to make it clearer what each option actually does.

Anyway, I agree on setting the max higher, to something like 64 GiB (there are, indeed, enthusiast consumer machines sporting 128 GiB of RAM nowadays), even though that is well into diminishing returns territory (for now...).
I would appreciate more feedback on the max value for 32-bit architectures, which I currently have set to 128 MiB.

FranciscoPombal · 2018-08-19T20:44:36Z

@Chocobo1
Somewhat related:
What is the status of #9120?
Does qBittorrent still recheck all torrents concurrently instead of sequentially?

I remember that in previous versions recheck was sequential. I don't remember exactly the version in which this behaviour was changed.

Sequential rechecking is (always?) better than concurrent, because even though it might not make much or any difference at all for systems with SSDs, the former is much faster in systems with HDDs.

LordNyriox · 2018-08-20T03:39:18Z

@FranciscoPombal:

I would appreciate more feedback on the max value for 32-bit architectures, which I currently have set to 128 MiB.

32-bit qBittorrent should use the same max-memory strategy as 64-bit.

In Windows systems (both x86 and x64), the maximum possible memory available to any 32-bit program, is 4 GB.

Chocobo1 · 2018-08-20T04:35:07Z

I need advice on the maximum allowed value for checking memory for 32-bit architectures. Currently, I set it to 128 MiB, because as I understand it, this will be competing with disk cache + program data for 2 GiB of RAM.

Instead of separating values for different architectures, I would just go for 1GB for both, even high performance seeding mode in libtorrent is merely using 32 MB: arvidn/libtorrent@70d6432
If someone benchmark it later and suspect it isn't sufficient, we can bump it later.

Chocobo1 · 2018-08-20T04:41:20Z

Does qBittorrent still recheck all torrents concurrently instead of sequentially?
I remember that in previous versions recheck was sequential. I don't remember exactly the version in which this behaviour was changed.

I am not sure, IIRC related code wasn't changed for some time, someone might need to check it again.

sledgehammer999 · 2018-08-20T09:15:24Z

Does qBittorrent still recheck all torrents concurrently instead of sequentially?

IIRC there isn't an option that we can set.
I think libtorrent decides this depending on the storage point. If 2 torrents reside on different disks/partitions (or is it physical disks?) it starts checking them concurrently.

FranciscoPombal · 2018-08-20T10:28:23Z

@LordNyriox

32-bit qBittorrent should use the same max-memory strategy as 64-bit.

In Windows systems (both x86 and x64), the maximum possible memory available to any 32-bit program, is 4 GB.

4 GiB vs 2 GiB maximum memory available depends on whether or not the process is "large address aware". I wasn't sure that was the case for qBittorrent, so I went with the conservative number.
Anyhow, not all of those 2 GiB/4 GiB should be available for this particular functionality, as there must be some left for the disk cache and program memory.

@Chocobo1

Instead of separating values for different architectures, I would just go for 1GB for both, even high performance seeding mode in libtorrent is merely using 32 MB: arvidn/libtorrent@70d6432
If someone benchmark it later and suspect it isn't sufficient, we can bump it later.

Ok. I will change the limit to 1 GiB for both. If someone then benchmarks and finds that there is still scaling near the 1 GiB mark, that would warrant a bump.

@Chocobo1 @sledgehammer999

I am not sure, IIRC related code wasn't changed for some time, someone might need to check it again.

IIRC there isn't an option that we can set.
I think libtorrent decides this depending on the storage point. If 2 torrents reside on different disks/partitions (or is it physical disks?) it starts checking them concurrently.

I see no problem in checking concurrently if two torrents are on different physical disks, but can libtorrent even distinguish different partitions from different physical disks? (pinging @arvidn)

Plus, users reporting these issues are saying that it happens for torrents on the same disk anyway.
It just seems safer to always check sequentially.
On the related issues, some users have found workarounds, but obviously not everyone knows about them and a permanent fix would be better.
Related issues: #9120, #8917, (maybe more, these are the most recent and active ones)

sledgehammer999 · 2018-08-20T15:54:02Z

About the sequential/concurrent hash rechecks. I just checked libtorrent docs. From settings_pack:

active_checking is the limit of number of simultaneous checking torrents.

The default value is set to 1. I did a search on qbt code and we don't change this field. I wonder how some users see concurrent hash rechecks then!?!?

FranciscoPombal · 2018-08-20T17:28:38Z

@sledgehammer999
The plot thickens...
I have not experienced the issue myself, but then again my main system is still running an older version (3.3.16). The problem seems to only affect 4.x versions anyhow.

Maybe in the following weeks I will have the means to attempt to reproduce the behaviour on the latest version.

LordNyriox · 2018-08-20T20:06:12Z

@sledgehammer999, @FranciscoPombal:

The problem seems to only affect 4.x versions anyhow.

More specifically, the problem seems to affect any build of qBittorrent that uses Libtorrent 1.1.x (instead of 1.0.x).

Just try @Gelmir's old 64-bit qBittorrent builds, if you do not believe me.

Most of those builds provided both Libtorrent1.0 and Libtorrent1.1 variants (which were otherwise identical). I ran into simultaneous Recheck issues every time I used a 1.1.x build.

sledgehammer999 · 2018-08-20T20:17:48Z

Out of curiosity I tried it myself. 2 big torrents on the same physical disk recheck sequentially. Adding a 3rd one for recheck which resides on another physical disk makes it recheck concurrently with the one checking from the other disk.
So active_checking limit probably has extra undocumented criteria.

FranciscoPombal · 2018-08-20T20:31:56Z

@LordNyriox

More specifically, the problem seems to affect any build of qBittorrent that uses Libtorrent 1.1.x (instead of 1.0.x).

Good point.

@sledgehammer999

Out of curiosity I tried it myself. 2 big torrents on the same physical disk recheck sequentially. Adding a 3rd one for recheck which resides on another physical disk makes it recheck concurrently with the one checking from the other disk.
So active_checking limit probably has extra undocumented criteria.

Have you tried with more than 2 torrents on the same physical disk? it would not be unreasonable to think that the problem maybe only manifests itself with >2 torrents.

LordNyriox · 2018-08-21T05:35:58Z

@FranciscoPombal, @sledgehammer999:

Have you tried with more than 2 torrents on the same physical disk? it would not be unreasonable to think that the problem maybe only manifests itself with >2 torrents.

I have (at various times, using the builds linked previously).

And the behavior described by @sledgehammer999, also affected 3+ torrents on the same disk.

@arvidn: Any input on this subject?

src/base/bittorrent/session.cpp

arvidn · 2018-08-21T15:27:59Z

src/gui/advancedsettings.cpp

@@ -151,6 +152,8 @@ void AdvancedSettings::saveAdvancedSettings()
    // Async IO threads
    session->setAsyncIOThreads(spinBoxAsyncIOThreads.value());
 #endif
+    // Checking Memor Usage


"Memor" -> "Memory"?

Will fix this typo

arvidn · 2018-08-21T15:31:00Z

if a torrent is not auto-managed, it is not subject to the active_checking limit. If both checking torrents are auto managed, and active_checking is 1, then it sounds like a bug

LordNyriox · 2018-08-21T16:10:25Z

@arvidn: What about when 3+ torrents are simultaneously marked for rechecking?

That seems to be when the bug triggers. When only 2 torrents are marked for rechecking, they recheck sequentially (as intended).

sledgehammer999 · 2018-08-21T22:18:06Z

if a torrent is not auto-managed, it is not subject to the active_checking limit.

This is it.

@arvidn: What about when 3+ torrents are simultaneously marked for rechecking?

That seems to be when the bug triggers. When only 2 torrents are marked for rechecking, they recheck sequentially (as intended).

In my above post, the 3rd torrent that I tried for rechecking was paused. In qbt everything explicitly paused is not auto-managed. That's why that torrent started checking concurrently with the other one.
And probably that's what the users observe.
PS: Torrents with the status "Completed" are seeding torrents that are paused.
PS1: Also "force resume" makes the torrent not auto-managed.

LordNyriox · 2018-08-22T04:48:38Z

@sledgehammer999, @arvidn: Just an FYI, some cautious users (such as myself) may prefer to only trigger rechecking on "paused" torrents.

Is there a reasonable way to disable "concurrent" rechecking on those?

arvidn · 2018-08-22T06:30:37Z

yes, set the stop-when-ready flag on the torrent before starting the check.

arvidn · 2018-08-22T06:59:44Z

what occurs to me is that to check a paused torrent, and have it still be paused after the check, still has a race condition. There is no way to start the check and set the auto-managed flag atomically.

if you set the auto-managed flag first, there's a risk the torrent may start downloading for a split second, until the recheck call is made.

If you start the rechecking first, it will start checking for a split second, until the auto managed flag is set and it's queued (assuming there's another torrent being checked already).

The latter is probably less severe.

sledgehammer999 · 2018-08-22T07:52:56Z

what occurs to me is that to check a paused torrent, and have it still be paused after the check, still has a race condition. There is no way to start the check and set the auto-managed flag atomically.

Internally we track if torrent is paused. If yes, we set upload_mode and resume it without automanage. Immediately we issue force recheck. On recheck completion, we pause it and take it out of upload mode.

sledgehammer999 · 2018-08-22T08:09:38Z

I forgot to add that theoritically we could ourselves(qbt) keep count of the rechecking torrents and preserve the limit before issuing the recheck command.

FranciscoPombal · 2018-08-27T22:29:18Z

Addressed review comments, rebased and squashed commits.
@Chocobo1 @sledgehammer999 @arvidn @LordNyriox
Feel free to merge if there are no further comments.

Chocobo1 · 2018-08-28T05:26:04Z

src/base/bittorrent/session.h

@@ -377,6 +377,8 @@ namespace BitTorrent
        void setAnnounceToAllTiers(bool val);
        int asyncIOThreads() const;
        void setAsyncIOThreads(int num);
+        int checkingMemUsage() const;
+        void setCheckingMemUsage(int num);


here the parameter is num while at definition is size, you should use the same name.

FranciscoPombal · 2018-08-28T10:19:05Z

Addressed review comments and squashed commits.
@Chocobo1 @sledgehammer999 @arvidn @LordNyriox
Feel free to merge if there are no further comments.

src/gui/advancedsettings.cpp

to unblock merging

Chocobo1 · 2018-12-05T03:40:55Z

@glassez
want to review/give an approval?

src/base/bittorrent/session.cpp

Chocobo1 · 2018-12-06T08:20:10Z

Thank you!

Chocobo1 reviewed Aug 19, 2018

View reviewed changes

FranciscoPombal force-pushed the checking_mem_usage_adv_settings branch from e184293 to 6e72a2e Compare August 19, 2018 16:31

glassez added GUI GUI-related issues/changes Core labels Aug 20, 2018

arvidn reviewed Aug 21, 2018

View reviewed changes

src/base/bittorrent/session.cpp Show resolved Hide resolved

arvidn reviewed Aug 21, 2018

View reviewed changes

FranciscoPombal force-pushed the checking_mem_usage_adv_settings branch 2 times, most recently from aa862c6 to 61e03e7 Compare August 27, 2018 22:27

thalieht mentioned this pull request Aug 28, 2018

QB moves multiple complete torrents at a time, leading to super high IO contention. #9407

Closed

Chocobo1 reviewed Aug 28, 2018

View reviewed changes

FranciscoPombal force-pushed the checking_mem_usage_adv_settings branch from 61e03e7 to f4806ec Compare August 28, 2018 10:18

Chocobo1 previously approved these changes Aug 28, 2018

View reviewed changes

zeule previously requested changes Aug 29, 2018

View reviewed changes

src/gui/advancedsettings.cpp Show resolved Hide resolved

glassez reviewed Dec 5, 2018

View reviewed changes

src/base/bittorrent/session.cpp Outdated Show resolved Hide resolved

Add checking_mem_usage option to AdvancedSettings

74be5d9

Chocobo1 dismissed their stale review via 74be5d9 December 5, 2018 19:11

Chocobo1 force-pushed the checking_mem_usage_adv_settings branch from f4806ec to 74be5d9 Compare December 5, 2018 19:11

glassez approved these changes Dec 6, 2018

View reviewed changes

Chocobo1 approved these changes Dec 6, 2018

View reviewed changes

Chocobo1 merged commit 1eef5b6 into qbittorrent:master Dec 6, 2018

FranciscoPombal deleted the checking_mem_usage_adv_settings branch December 6, 2018 13:29

Add checking_mem_usage option to AdvancedSettings #9373

Add checking_mem_usage option to AdvancedSettings #9373

Conversation

FranciscoPombal commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Aug 19, 2018

FranciscoPombal commented Aug 19, 2018 • edited

Chocobo1 commented Aug 19, 2018

FranciscoPombal commented Aug 19, 2018

FranciscoPombal commented Aug 19, 2018

LordNyriox commented Aug 19, 2018

FranciscoPombal commented Aug 19, 2018

FranciscoPombal commented Aug 19, 2018

LordNyriox commented Aug 20, 2018 • edited

Chocobo1 commented Aug 20, 2018

Chocobo1 commented Aug 20, 2018

sledgehammer999 commented Aug 20, 2018

FranciscoPombal commented Aug 20, 2018

sledgehammer999 commented Aug 20, 2018

FranciscoPombal commented Aug 20, 2018

LordNyriox commented Aug 20, 2018

sledgehammer999 commented Aug 20, 2018

FranciscoPombal commented Aug 20, 2018 • edited

LordNyriox commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn commented Aug 21, 2018

LordNyriox commented Aug 21, 2018 • edited

sledgehammer999 commented Aug 21, 2018

LordNyriox commented Aug 22, 2018

arvidn commented Aug 22, 2018

arvidn commented Aug 22, 2018

sledgehammer999 commented Aug 22, 2018

sledgehammer999 commented Aug 22, 2018

FranciscoPombal commented Aug 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranciscoPombal commented Aug 28, 2018

Chocobo1 commented Dec 5, 2018

Chocobo1 commented Dec 6, 2018

FranciscoPombal commented Aug 19, 2018 •

edited

LordNyriox commented Aug 20, 2018 •

edited

FranciscoPombal commented Aug 20, 2018 •

edited

LordNyriox commented Aug 21, 2018 •

edited