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

pread-disk-io #7013

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

pread-disk-io #7013

wants to merge 9 commits into from

Conversation

arvidn
Copy link
Owner

@arvidn arvidn commented Aug 14, 2022

add a multi-threaded, pread()-based, disk I/O backend (pread_disk_io)

@glassez
Copy link
Contributor

glassez commented Aug 15, 2022

add a multi-threaded, pread()-based, disk I/O backend (pread_disk_io)

Is it something similar to libtorrent 1.2 disk I/O?

@arvidn
Copy link
Owner Author

arvidn commented Aug 15, 2022

yes, it is similar. This doesn't cache blocks in memory though. It just has a store-buffer while waiting for blocks to be written to disk.

@xavier2k6
Copy link
Contributor

Failing to build with qBittorrent master:
https://github.com/xavier2k6/qBittorrent/actions/runs/2860408368

@glassez
Copy link
Contributor

glassez commented Aug 15, 2022

Failing to build with qBittorrent master: https://github.com/xavier2k6/qBittorrent/actions/runs/2860408368

We never previously care about supporting libtorrent master. I'll try to deal with it ASAP since I really interested with this new disk I/O type.

@xavier2k6
Copy link
Contributor

@glassez feel free to make any changes/pushes to my branch if needed for testing.

@arvidn
Copy link
Owner Author

arvidn commented Aug 15, 2022

I thought pausing a session was unusual. Maybe I should add that back.

@xavier2k6 is that a build with deprecated functions disabled?

@glassez
Copy link
Contributor

glassez commented Aug 15, 2022

I thought pausing a session was unusual.

Do you mean create session in paused state? Of course it is useful! It was even added at our request, if memory serves.

@xavier2k6
Copy link
Contributor

is that a build with deprecated functions disabled?

-Ddeprecated-functions=OFF

https://github.com/xavier2k6/qBittorrent/blob/6fdf4f1e2d741b7854999cefa75bf2fbbee7dae2/.github/workflows/ci_windows.yaml#L101

@arvidn
Copy link
Owner Author

arvidn commented Aug 15, 2022

Do you mean create session in paused state? Of course it is useful! It was even added at our request, if memory serves.

I think this feature got lost in a merge. I'll fix that in master

@userdocs
Copy link

I made some nox linux multiarch builds to test using the pr branch.

https://github.com/userdocs/qbt_static_test/releases/tag/release-4.4.3.1_pread-disk-io
https://github.com/userdocs/qbt_static_test/releases/tag/master_pread-disk-io

@glassez
Copy link
Contributor

glassez commented Aug 16, 2022

We never previously care about supporting libtorrent master. I'll try to deal with it ASAP since I really interested with this new disk I/O type.

It seems that it will not be possible to keep the qBittorrent code compatible with both master and RC_2_0 branch at the same time due to the fact that master declares the same version as RC_2_0. @arvidn, could you fix that?

@glassez
Copy link
Contributor

glassez commented Aug 16, 2022

@glassez feel free to make any changes/pushes to my branch if needed for testing.

I cannot push directly to this branch so I created PR.

I tested it a little with checking a large v2 torrent (30+ GB), but I didn't finish it, because I had a slow computer and little time. BUT nevertheless, I noticed significant hangups of UI during checking, so I believe that we still have some other problems besides the checking speed itself. I'll deal with related issues later. As for the checking speed itself, you should compare it on the same torrent, using both libtorrent-1.2 and libtorrent-2.0 with different disk I/O types. The RAM limit should be set to the highest possible value so as not to be a bottleneck in these tests.

@james58899
Copy link

I noticed this PR always used 16KB block size for reads, which is very inefficient, especially on HDD.

I think the block size too small is the root cause for the difference in performance between 1.2 and 2.0. In 1.2 with disk cache enable average size is >256KB, 2.0 mmap average size ~128KB and pread only 16KB. That caused the number of IOs to increase by more than ten times and kill disk performance.

@brvphoenix
Copy link

@glassez feel free to make any changes/pushes to my branch if needed for testing.

I cannot push directly to this branch so I created PR.

I tested it a little with checking a large v2 torrent (30+ GB), but I didn't finish it, because I had a slow computer and little time. BUT nevertheless, I noticed significant hangups of UI during checking, so I believe that we still have some other problems besides the checking speed itself. I'll deal with related issues later. As for the checking speed itself, you should compare it on the same torrent, using both libtorrent-1.2 and libtorrent-2.0 with different disk I/O types. The RAM limit should be set to the highest possible value so as not to be a bottleneck in these tests.

glassez/qBittorrent@97e4855#diff-0efd426259b58f47b1f848560cf9c51a92268084060cebf91c7ef22f092da1f7R1410

Doesn't this line should be sessionParams.disk_io_constructor = customPReadDiskIOConstructor;?

@glassez
Copy link
Contributor

glassez commented Aug 17, 2022

glassez/qBittorrent@97e4855#diff-0efd426259b58f47b1f848560cf9c51a92268084060cebf91c7ef22f092da1f7R1410

Doesn't this line should be sessionParams.disk_io_constructor = customPReadDiskIOConstructor;?

Damn it! You're right. I forgot to change this in a hurry after copy-pasting.
Fixed.

@glassez
Copy link
Contributor

glassez commented Aug 17, 2022

I believe that increasing the checking speed is not the only expected advantage of this disk I/O type. So we should test other cases where MMap based I/O messes up, for example, using external and especially network drives to download torrents, problems related to I/O on macOS.

@arvidn
Copy link
Owner Author

arvidn commented Aug 17, 2022

in parallel to this I'm working on modifying the mmap_disk_io in 2.0 to use pwrite() for writes and mmap for reads. I think that could give use the best of both worlds (as long as the OS is sophisticated enough I suppose).

@glassez
Copy link
Contributor

glassez commented Aug 17, 2022

I'm working on modifying the mmap_disk_io in 2.0 to use pwrite() for writes and mmap for reads.

IIRC, another problem of current MMap disk I/O implementation is that you map the entire file, which can still be a stumbling block, even if memory mapping will be used only for reading files.

@xavier2k6
Copy link
Contributor

@glassez I merged your PR in my branch, build is available below:
https://github.com/xavier2k6/qBittorrent/suites/7850730272/artifacts/332517996

Is there any Large V2 torrent publicly available that we can all use for testing purposes for consistency?

BTW, does OpenSSL have anything to do with the hashing? (official qBittorrent use 1.1.1 where as qBittorrent CI (Windows) builds use 3.0.5)

@glassez
Copy link
Contributor

glassez commented Aug 17, 2022

Is there any Large V2 torrent publicly available that we can all use for testing purposes for consistency?

I used one specially created to test another problem with large torrents. But they must exist in reality, otherwise where would that problem come from?

Another thing is since we are interested in comparing libtorrent 1.2 vs 2.0 in the checking speed problem, we will have to use a pure v1 torrent in testing.

@glassez
Copy link
Contributor

glassez commented Aug 18, 2022

It seems that it will not be possible to keep the qBittorrent code compatible with both master and RC_2_0 branch at the same time due to the fact that master declares the same version as RC_2_0. @arvidn, could you fix that?

#7018 will do (if there are no pitfalls).

@ghost
Copy link

ghost commented Aug 25, 2022

I think the hangups during checking are due to high page faults due to working set limit. It should be disabled if one isn’t using mmap I/O.

@tristanleboss
Copy link

tristanleboss commented Aug 25, 2022

@xavier2k6 There is a torrent https://the-eye.eu/public/Random/torrents/oldversion.com_Sept2019.torrent with the dump of all stuffs from oldversion.com. The torrent file itself is 5.5mb and there is 30k files inside along with as many folders (each version is in its own folder) for a gross total of 472.18gb. Those stuffs are just demo, freeware or shareware... no cracked softwares. The file sizes range from 1mb to hundreds of megabytes. It works flawlessly with qBt 4.4.4 RC_1_2.

@ghost

This comment was marked as off-topic.

@tristanleboss
Copy link

tristanleboss commented Aug 25, 2022

@summerqb I have it in my qBt 4.4.4 RC_1_2 and it works just fine. But RC_2_0 was freezing and never starting. That's why it's probably a really good test torrent!

@ghost
Copy link

ghost commented Aug 25, 2022

Tested all 3 branches and RC_1_2 still beats the others.
RC_1_2 is 33% faster than this one and 50% faster than RC_2_0.

@ghost

This comment was marked as off-topic.

@arvidn
Copy link
Owner Author

arvidn commented Jan 15, 2024

BTW, could it not be really avoided?

There is hack in 2.0 to OR a flag into the status_t type. patching the pread disk I/O code to use the hack wouldn't be a big deal.

@luzpaz
Copy link
Contributor

luzpaz commented Feb 11, 2024

Any more traction here ?

@arvidn arvidn force-pushed the pread-disk-io branch 4 times, most recently from a71a969 to 25dd5e2 Compare February 19, 2024 01:21
@SemiAccurate
Copy link

@arvidn we have replied to you and have been patiently waiting for your input for many months now. When do you plan to continue the conversation regarding your use of memory mapped I/O and its consequences for libtorrent?

@Rootax
Copy link

Rootax commented Feb 23, 2024

@arvidn we have replied to you and have been patiently waiting for your input for many months now. When do you plan to continue the conversation regarding your use of memory mapped I/O and its consequences for libtorrent?

It seems he is pushing a lot of commits, so, I would say he's working on it :)

@SemiAccurate
Copy link

@arvidn we have replied to you and have been patiently waiting for your input for many months now. When do you plan to continue the conversation regarding your use of memory mapped I/O and its consequences for libtorrent?

It seems he is pushing a lot of commits, so, I would say he's working on it :)

Well even this PR - going at a snail's pace as it is - is inadequate based on the performance measurements of #7657

There the author had gone over this PR and implemented preads and pwrites along the lines of this PR and it still gave half the performance of libtorrent 1.2. That is because Arvid's changes in this PR don't include a read buffer along the lines of 1.2, and so the performance is still inadequate.

@arvidn why this half measure? Why don't you implement the full solution from 1.2 with the disk buffer effectively acting as an ARC cache so that 2.x gets maximum performance?

@nagyimre1980
Copy link

libtorrent 2.0 is still unusable, poor performance. it caused a lot of damage to the torrent community.

@Rootax
Copy link

Rootax commented May 3, 2024

libtorrent 2.0 is still unusable, poor performance. it caused a lot of damage to the torrent community.

Wait what ? I agree that 2.0 didn't work as expected, but client like QBT just switched back to 1.2, which is still being patched, and voilà. I get that it's frustrating, but it's not the end of the world...

@stalkerok
Copy link

A lot of thumbs down... That's a mouthful, of course, but there's truth to it anyway.
In my tests, “pread-disk-io” is inferior to disk I/O in 1.2.

@Rootax
Copy link

Rootax commented May 3, 2024

A lot of thumbs down... That's a mouthful, of course, but there's truth to it anyway. In my tests, “pread-disk-io” is inferior to disk I/O in 1.2.

True, but there is a proper way to express it imo. Arvidn is working a lot on 2.1, maybe we can wait to see the results ...

@stalkerok
Copy link

QBT just switched back to 1.2, which is still being patched, and voilà

1.2 doesn't contain a huge amount of features, fixes and changes compared to 2.0, if the branches were developed in parallel there would be a lot less negativity.

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

Successfully merging this pull request may close these issues.

None yet