libtorrent session options #3235

Chocobo1 · 2015-06-16T09:25:23Z

>>First post outdated, see #3235 (comment) <<

I found some interesting options when reading libtorrent's document:

guided_read_cache: [default off]
Also see: tuning#disk-cache

The idea being that slow peers don't use up a disproportional amount of space in the cache

This looks promising.
prefer_tcp: [default off]
Also see: tuning#utp-tcp-mixed-mode
Since there are issues with peer_proportional in the past, then just use prefer_tcp in all cases.
I can't see why this should be depending on rate_limit_utp.
use_parole_mode: [default off]
Maybe give a hashcheck-failed peer another chance before banning.

Chocobo1 · 2016-01-21T02:34:08Z

I think this PR will have a better chance of getting merged by making these options available in "advanced options". Maybe later when I find time.

Chocobo1 · 2016-02-02T08:26:16Z

Commits & first post updated.

glassez · 2017-04-22T09:36:23Z

src/base/preferences.cpp

@@ -1403,6 +1403,16 @@ void Preferences::setOsCache(bool enable)
    setValue("Preferences/Advanced/osCache", enable);
 }

+bool Preferences::getGuidedReadCache() const


Preferences is deprecated now.

This whole PR is outdated, I'm not even sure if @sledgehammer999 would approve this...

Chocobo1 · 2017-05-26T06:52:41Z

@Chocobo1: ?

I plan to resurrect it after I'm done with other things, it's very low prio now.

sledgehammer999 · 2017-08-10T14:18:14Z

@Chocobo1 FYI, this is approved. You can resurrect and merge. And probably you don't need 3 separate commits.

sledgehammer999 · 2017-08-10T14:21:16Z

@Chocobo1 I think guided_read_cache and use_parole_mode can always be on. IMO, they don't need a user setting.

Chocobo1 · 2017-08-11T06:00:00Z

I think guided_read_cache and use_parole_mode can always be on. IMO, they don't need a user setting.

libtorrent made use_parole_mode = true as default, I'll drop this one.
As for guided_read_cache, it's disabled by default in libtorrent and might not get enough testing.
IMO make it tunable is better.

FYI, this is approved. You can resurrect and merge. And probably you don't need 3 separate commits.

Now I plan to add some other options, I'll put out a PR first, then we can bargain.

Chocobo1 · 2017-08-12T11:50:49Z

@sledgehammer999 @glassez
I've updated this PR and now it add options for the following settings:

guided_read_cache
mixed_mode_algorithm
allow_multiple_connections_per_ip
choking_algorithm
seed_choking_algorithm
suggest_mode
send_buffer_watermark
send_buffer_low_watermark
send_buffer_watermark_factor

sledgehammer999

And a final minor nitpick but 3 of your commit messages don't start with a capital letter like the others.

sledgehammer999 · 2017-08-12T12:55:03Z

src/base/bittorrent/session.cpp

@@ -270,6 +270,7 @@ Session::Session(QObject *parent)
    , m_maxUploadsPerTorrent(BITTORRENT_SESSION_KEY("MaxUploadsPerTorrent"), -1, lowerLimited(0, -1))
    , m_isUTPEnabled(BITTORRENT_SESSION_KEY("uTPEnabled"), true)
    , m_isUTPRateLimited(BITTORRENT_SESSION_KEY("uTPRateLimited"), true)
+    , m_utpMixedMode(BITTORRENT_SESSION_KEY("utpMixedMode"), m_isUTPEnabled)


I think most existing installations are working today with prefer_tcp. So I would say the default here should be false.

Also the name should be uTPMixedMode

Also the name should be uTPMixedMode

OK

So I would say the default here should be false.

Do you think we should also turn uTP off by default? otherwise, with uTP on + prefer_tcp, uTP would be useless in theory.

sledgehammer999 · 2017-08-12T13:02:42Z

src/base/bittorrent/session.cpp

+
+void Session::setChokingAlgorithm(bool mode)
+{
+    if (mode == m_chokingAlgorithm)return;


space before return

fixed here, will push update later

sledgehammer999 · 2017-08-12T13:03:45Z

src/gui/advancedsettings.cpp

@@ -187,6 +189,13 @@ void AdvancedSettings::saveAdvancedSettings()
    // Tracker
    session->setTrackerEnabled(cb_tracker_status.isChecked());
    pref->setTrackerPort(spin_tracker_port.value());
+
+    // Choking algorithm


do you really need the blank lines before the comments?

will remove it.

sledgehammer999 · 2017-08-12T13:06:30Z

src/base/bittorrent/session.cpp

+
+    settingsPack.set_int(libt::settings_pack::choking_algorithm,
+        chokingAlgorithm() ? libt::choking_algorithm_t::rate_based_choker : libt::choking_algorithm_t::fixed_slots_choker);
+    settingsPack.set_int(libt::settings_pack::seed_choking_algorithm, seedChokingAlgorithm());


seed_choking_algorithm is being set in the session constructor so you need to remove it from there too.

OK, thanks!

sledgehammer999 · 2017-08-12T13:12:29Z

src/gui/advancedsettings.cpp

+    // Send buffer watermark
+    spinSendBufferWatermark.setMinimum(1);
+    spinSendBufferWatermark.setMaximum(INT_MAX);
+    spinSendBufferWatermark.setSuffix(tr(" KB"));


KiB.
Also shouldn't it be non-translatable?

Also shouldn't it be non-translatable?

fixed, will also fix other locations.

Chocobo1 · 2017-08-14T14:54:55Z

@sledgehammer999
I think it's ready, the only thing left is #3235 (comment)

sledgehammer999 · 2017-08-14T16:07:01Z

I think it's ready, the only thing left is #3235 (comment)

Shit. I thought I had replied to this. Yes. No problem. Approved by me.
Now @glassez remains to revoke his block.

glassez · 2017-08-14T16:44:25Z

src/gui/advancedsettings.cpp

@@ -197,7 +223,7 @@ void AdvancedSettings::updateCacheSpinSuffix(int value)
    if (value <= 0)
        spin_cache.setSuffix(tr(" (auto)"));
    else
-        spin_cache.setSuffix(tr(" MiB"));
+        spin_cache.setSuffix(" MiB");
 }


Why this string isn't translatable?

@glassez: This was requested by @sledgehammer999

@sledgehammer999, why?

It was a question.
Is it reasonable to have translatable units?

At least some of the Cyrillic languages use transliterated abbreviations (e.g. "MіБ" for "MiB" in Ukrainian).

OK, thanks, will fix it.

glassez · 2017-08-14T18:52:06Z

src/base/bittorrent/session.cpp

+void Session::setGuidedReadCache(bool enabled)
+{
+    if (enabled == m_guidedReadCache) return;
+    m_guidedReadCache = enabled;


I would add blank line before this one like in another setters.

glassez · 2017-08-14T18:54:59Z

src/base/bittorrent/session.h

@@ -314,6 +314,10 @@ namespace BitTorrent
        void setForceProxyEnabled(bool enabled);
        bool isProxyPeerConnectionsEnabled() const;
        void setProxyPeerConnectionsEnabled(bool enabled);
+        bool chokingAlgorithm() const;


Why do you provide originally enumerable setting as bool?
The same question is about mixed mode algorithm.

both changed to int type.

glassez · 2017-08-14T18:57:04Z

src/base/bittorrent/session.h

@@ -330,6 +334,16 @@ namespace BitTorrent
        void setDiskCacheTTL(uint ttl);
        bool useOSCache() const;
        void setUseOSCache(bool use);
+        bool guidedReadCache() const;


Please use naming convention for bool properties: isGuidedReadCacheUsed or isGuidedReadCacheEnabled.
The same comment is for other bool properties.

Naming Boolean Getters, Setters, and Properties
Finding good names for the getter and setter of a bool property is always a special pain. Should the getter be called checked() or isChecked()? scrollBarsEnabled() or areScrollBarEnabled()?

In Qt 4, we used the following guidelines for naming the getter function:

Adjectives are prefixed with is-. Examples:
isChecked()
isDown()
isEmpty()
isMovingEnabled()
However, adjectives applying to a plural noun have no prefix:
scrollBarsEnabled(), not areScrollBarsEnabled()
Verbs have no prefix and don't use the third person (-s):
acceptDrops(), not acceptsDrops()
allColumnsShowFocus()
Nouns generally have no prefix:
autoCompletion(), not isAutoCompletion()
boundaryChecking()
Sometimes, having no prefix is misleading, in which case we prefix with is-:
isOpenGLAvailable(), not openGL()
isDialog(), not dialog()
(From a function called dialog(), we would normally expect that it returns a QDialog.)
The name of the setter is derived from that of the getter by removing any is prefix and putting a set at the front of the name; for example, setDown() and setScrollBarsEnabled(). The name of the property is the same as the getter, but without the is prefix.

So useOSCache kind of makes sense. Even guidedReadCache can be used as bool property name.

Apparently I jumped the gun here... the point is that if bool property has a name without is-/has- prefix it's not so clear it is correct or not. We need to check all new bool properties.

Chocobo1 · 2017-08-15T06:39:01Z

PR updated.

sledgehammer999 · 2017-08-18T16:44:11Z

src/base/bittorrent/session.cpp

@@ -299,6 +299,8 @@ Session::Session(QObject *parent)
    , m_encryption(BITTORRENT_SESSION_KEY("Encryption"), 0)
    , m_isForceProxyEnabled(BITTORRENT_SESSION_KEY("ForceProxy"), true)
    , m_isProxyPeerConnectionsEnabled(BITTORRENT_SESSION_KEY("ProxyPeerConnections"), false)
+    , m_chokingAlgorithm(BITTORRENT_SESSION_KEY("ChokingAlgorithm"), 0)
+    , m_seedChokingAlgorithm(BITTORRENT_SESSION_KEY("SeedChokingAlgorithm"), 0)


The default value here should be 1. It was a request in the past to use fastest_upload by default.

will fix

However, it feels weird because peer which has better download speed will always have higher priority to be served.

sledgehammer999 · 2017-08-18T16:56:11Z

src/base/bittorrent/session.cpp

-    , m_chokingAlgorithm(BITTORRENT_SESSION_KEY("ChokingAlgorithm"), 0)
-    , m_seedChokingAlgorithm(BITTORRENT_SESSION_KEY("SeedChokingAlgorithm"), 0)
+    , m_chokingAlgorithm(BITTORRENT_SESSION_KEY("ChokingAlgorithm"), 0, clampValue(0, 2))
+    , m_seedChokingAlgorithm(BITTORRENT_SESSION_KEY("SeedChokingAlgorithm"), 0, clampValue(0, 2))


The default value here should be 1. It was a request in the past to use fastest_upload by default.

glassez · 2017-08-18T16:57:00Z

src/base/bittorrent/session.cpp

@@ -296,6 +317,8 @@ Session::Session(QObject *parent)
    , m_encryption(BITTORRENT_SESSION_KEY("Encryption"), 0)
    , m_isForceProxyEnabled(BITTORRENT_SESSION_KEY("ForceProxy"), true)
    , m_isProxyPeerConnectionsEnabled(BITTORRENT_SESSION_KEY("ProxyPeerConnections"), false)
+    , m_chokingAlgorithm(BITTORRENT_SESSION_KEY("ChokingAlgorithm"), 0, clampValue(0, 2))


ChockingAlgorithm should be limited by 0 and 1, isn't it?

see enum choking_algorithm_t in libtorrent doc.

But in earlier commit you chose to not expose bittyrant_choker.
Was there a reason?

You can't mix interface of components of an application with interface of underlying library. If you allow users only two choices then Session::chockingAlgorithm property has only two possible values independently of the way it really implemented.

Was there a reason?

libtorrent states its incomplete & untested.
Also read Q: Won’t BitTyrant hurt overall BitTorrent performance if everyone uses it?
in http://bittyrant.cs.washington.edu/

libtorrent states its incomplete & untested.
Also read Q: Won’t BitTyrant hurt overall BitTorrent performance if everyone uses it?
in http://bittyrant.cs.washington.edu/

+1.
Don't forget to rebase.

You can't mix interface of components of an application with interface of underlying library

That won't be a problem after enums are deployed.

cleanup header include order

send_buffer_watermark_factor knobs

Chocobo1 · 2017-08-21T11:54:47Z

Updated.

glassez

Last small comments. The rest LGTM.

glassez · 2017-08-21T12:18:00Z

src/base/bittorrent/session.cpp

@@ -282,6 +301,9 @@ Session::Session(QObject *parent)
    , m_maxUploadsPerTorrent(BITTORRENT_SESSION_KEY("MaxUploadsPerTorrent"), -1, lowerLimited(0, -1))
    , m_isUTPEnabled(BITTORRENT_SESSION_KEY("uTPEnabled"), true)
    , m_isUTPRateLimited(BITTORRENT_SESSION_KEY("uTPRateLimited"), true)
+    , m_utpMixedMode(BITTORRENT_SESSION_KEY("uTPMixedMode"), static_cast<BitTorrent::MixedModeAlgorithm>(m_isUTPEnabled.value())


Do we have dependent default value here?

IMO, using ternary operator (instead of cast) would be more obvious.

Do we have dependent default value here?

Ah, damn it, I didn't realize until now... it doesn't need to depend on others.
Changed.

Chocobo1 · 2017-08-23T14:31:03Z

@evsh @sledgehammer999
Any comments?

zeule · 2017-08-23T15:11:49Z

Any comments?

No.

Chocobo1 · 2017-08-24T03:36:21Z

thanks to everyone

Seeker2 · 2017-10-30T18:35:23Z

Was announce_to_all_tiers dropped because qBitTorrent lacks the GUI support for it?

Send upload piece suggestions cripples upload speed for some reason. I'm guessing it's an super/initial seeding type of feature to be used with low upload max?

prefer_tcp set to false also causes local peers to slow to almost nothing when internet uTP peers are connected even when LAN speeds are set to unlimited.

sledgehammer999 · 2017-10-30T21:11:56Z

@Seeker2 look at #7660

Chocobo1 force-pushed the session_option branch 4 times, most recently from 518107c to 3e025e8 Compare February 2, 2016 08:20

Chocobo1 mentioned this pull request Feb 25, 2017

I plan to create a PR for missing and hardcoded session settings #6421

Closed

LordNyriox approved these changes Apr 21, 2017

View reviewed changes

glassez requested changes Apr 22, 2017

View reviewed changes

LordNyriox mentioned this pull request May 10, 2017

Steps for version 3.4 release #6767

Closed

thalieht mentioned this pull request Jun 12, 2017

Support libtorrent option allow_multiple_connections_per_ip #5884

Closed

Chocobo1 force-pushed the session_option branch from 3e025e8 to 77dea27 Compare August 12, 2017 11:43

sledgehammer999 reviewed Aug 12, 2017

View reviewed changes

Chocobo1 force-pushed the session_option branch 2 times, most recently from 1b82f06 to 012b22f Compare August 12, 2017 13:59

sledgehammer999 previously approved these changes Aug 14, 2017

View reviewed changes

glassez requested changes Aug 14, 2017

View reviewed changes

Chocobo1 dismissed sledgehammer999’s stale review via 1daf5a3 August 15, 2017 06:07

Chocobo1 force-pushed the session_option branch 2 times, most recently from 1daf5a3 to 70df60b Compare August 15, 2017 06:31

Chocobo1 force-pushed the session_option branch 2 times, most recently from 5ba34f9 to ed7bdb2 Compare August 15, 2017 06:52

Chocobo1 force-pushed the session_option branch from 885f338 to e2eff8b Compare August 15, 2017 14:54

sledgehammer999 reviewed Aug 18, 2017

View reviewed changes

glassez requested changes Aug 18, 2017

View reviewed changes

glassez added the Core label Aug 18, 2017

Chocobo1 added 7 commits August 21, 2017 16:48

Add guided_read_cache knob

6150e0c

cleanup header include order

Add mixed_mode_algorithm knob

4185209

Add allow_multiple_connections_per_ip knob. Closes qbittorrent#5884.

21ec8fe

Add choking_algorithm & seed_choking_algorithm knob

6c32f6b

Add suggest_mode knob

55a1628

Add send_buffer_watermark send_buffer_low_watermark

f4be165

send_buffer_watermark_factor knobs

Add sanitize helper clampValue()

3f5fa00

Chocobo1 force-pushed the session_option branch from e2eff8b to c1a3389 Compare August 21, 2017 10:38

glassez reviewed Aug 21, 2017

View reviewed changes

Use enums for settings

2da2054

Chocobo1 force-pushed the session_option branch from c1a3389 to 2da2054 Compare August 21, 2017 12:34

glassez approved these changes Aug 21, 2017

View reviewed changes

LordNyriox approved these changes Aug 21, 2017

View reviewed changes

zeule approved these changes Aug 23, 2017

View reviewed changes

Chocobo1 merged commit 1afd5f7 into qbittorrent:master Aug 24, 2017

Chocobo1 deleted the session_option branch August 24, 2017 03:35

Seeker2 mentioned this pull request Sep 28, 2017

Feature Request: all libtorrent settings #7506

Closed

Seeker2 mentioned this pull request Nov 1, 2017

Expose arbitrary libtorrent settings like Deluge does, to tune for high-performance connections. #7585

Open

LordNyriox mentioned this pull request Sep 15, 2018

Going to release v4.1.3 #9468

Closed

libtorrent session options #3235

libtorrent session options #3235

Conversation

Chocobo1 commented Jun 16, 2015 • edited

>>First post outdated, see #3235 (comment) <<

Chocobo1 commented Jan 21, 2016

Chocobo1 commented Feb 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented May 26, 2017

sledgehammer999 commented Aug 10, 2017

sledgehammer999 commented Aug 10, 2017

Chocobo1 commented Aug 11, 2017

Chocobo1 commented Aug 12, 2017

sledgehammer999 left a 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

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 Aug 12, 2017 • edited

Choose a reason for hiding this comment

sledgehammer999 Aug 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Aug 14, 2017

sledgehammer999 commented Aug 14, 2017

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

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

glassez Aug 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Aug 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 Aug 18, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chocobo1 commented Aug 21, 2017

glassez left a 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 23, 2017

zeule commented Aug 23, 2017

Chocobo1 commented Aug 24, 2017

Seeker2 commented Oct 30, 2017

sledgehammer999 commented Oct 30, 2017

Chocobo1 commented Jun 16, 2015 •

edited

Chocobo1 Aug 12, 2017 •

edited

sledgehammer999 Aug 12, 2017 •

edited

glassez Aug 14, 2017 •

edited

Chocobo1 Aug 18, 2017 •

edited