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

Add piece_extent_affinity to AdvancedSettings #11781

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

FranciscoPombal
Copy link
Member

@FranciscoPombal FranciscoPombal commented Dec 29, 2019

And expose option in WebUI settings.

More info:
https://libtorrent.org/single-page-ref.html#piece_extent_affinity

name type default
piece_extent_affinity bool false

when this is true, create an affinity for downloading 4 MiB extents of adjacent pieces. This is an attempt to achieve better disk I/O throughput by downloading larger extents of bytes, for torrents with small piece sizes

Copy link
Member

@Chocobo1 Chocobo1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also you should make sure this PR will build nicely on our CI which is cumbersome work by adding #if conditionals here and there...
As a lazy coder, I would simply put off this PR till the time that specific libtorrent version is available widely.

src/gui/advancedsettings.cpp Show resolved Hide resolved
@Chocobo1 Chocobo1 added this to the 4.2.2 milestone Dec 29, 2019
@Chocobo1 Chocobo1 added the Core label Dec 29, 2019
@FranciscoPombal
Copy link
Member Author

FranciscoPombal commented Dec 29, 2019

@Chocobo1

Also you should make sure this PR will build nicely on our CI which is cumbersome work by adding #if conditionals here and there...
As a lazy coder, I would simply put off this PR till the time that specific libtorrent version is available widely.

Ugh, I totally forgot this setting is only available in libtorrent >= 1.2.x. So basically I have to wrap everything I wrote in

#if (LIBTORRENT_VERSION_NUM >= 10200)
//code
#endif

right?

@Chocobo1
Copy link
Member

right?

You got the idea right but not the version number. Libtorrent seems got that feature in 1.2.2 and you will need to use the correct version format.
Also that leads to another problem: How should WebUI handle this compile time detection?
Or you may just show the option but without any real effects...

@FranciscoPombal
Copy link
Member Author

FranciscoPombal commented Dec 29, 2019

@Chocobo1

You got the idea right but not the version number. Libtorrent seems got that feature in 1.2.2 and you will need to use the correct version format.

Yeah, right, should be 10202.

Also that leads to another problem: How should WebUI handle this compile time detection?
Or you may just show the option but without any real effects...

I was thinking of changing the description to Use piece extent affinity (requires libtorrent >=1.2.2), so that at least users will get a clue if it does nothing. Otherwise, we could make a request to api/v2/app/buildInfo and conditionally disable stuff based on the libtorrent version, but I am not sure if that is a good solution.

@sledgehammer999
Copy link
Member

Being helpful to readers you should have included in the opening comment a link to the docs for piece_extent_affinity and if the docs were small also copy them in a quote.

@FranciscoPombal
Copy link
Member Author

FranciscoPombal commented Jan 4, 2020

@Chocobo1 Ok now there are #if guards everywhere except:

  • in src/gui/advancedsettings.h, where it was not needed and would either require me to put it inline or change the indentation of the file in a weird way
  • in the html file, so that checking/unchecking the box in the WebUI does nothing if using libtorrent <1.2.2

@FranciscoPombal
Copy link
Member Author

By the way, I also tested with libtorrent 1.2.1 and verified that it was working as expected.

Copy link
Member

@Chocobo1 Chocobo1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only 2 minor comments left

src/gui/advancedsettings.cpp Outdated Show resolved Hide resolved
src/webui/www/private/views/preferences.html Outdated Show resolved Hide resolved
@FranciscoPombal
Copy link
Member Author

@Chocobo1 ready.

Chocobo1
Chocobo1 previously approved these changes Jan 5, 2020
@FranciscoPombal
Copy link
Member Author

@glassez Review please?

src/webui/api/appcontroller.cpp Outdated Show resolved Hide resolved
@FranciscoPombal
Copy link
Member Author

@Chocobo1 I found a better way of addressing your most recent comments.

I moved the libtorrent version checks to the function bodies instead of the whole definitions and declarations, which allowed me to remove the checks in other places.

I did not remove the versions checks from the GUI code because I think it is better that the option does not appear at all if it does nothing. On the WebUI code I had no choice but to always include the checkbox, and as per @glassez's suggestion, I also had no choice on the API side, in order to keep the API interface stable.

Expose option in WebUI settings and WebAPI.

Requires WebAPI version bump.

Closes qbittorrent#11436.
@Chocobo1 Chocobo1 merged commit 146e821 into qbittorrent:master Jan 14, 2020
@Chocobo1
Copy link
Member

@FranciscoPombal
Thank you!

@FranciscoPombal
Copy link
Member Author

@glassez @Piccirello As a result of this PR, the preferences JSON returned by the WebAPI will potentially have one more key value pair, and it is now also be possible to change said field with the api/v2/app/setPreferences method.

I believe this warrants a patch version bump of the WebAPI, since it is not a breaking change for existing clients. It is part of the API contract that the content returned by api/v2/app/preferences is variable.

If it is ok with you, I'd rather this feature also be part of version 2.4.1 of the API, piggy-backing off of #11825

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

Successfully merging this pull request may close these issues.

4 participants