rakshasa / libtorrent

libTorrent BitTorrent library
http://rtorrent.net/downloads/
GNU General Public License v2.0
892 stars 209 forks source link

Speed up delegating by returning multiple request assignments at once #250

Closed kannibalox closed 1 month ago

kannibalox commented 5 months ago

This should follow the same logic as before, which I double-checked by graphing the piece progress from the LT_LOG_PIECE_EVENTS log and confirming the same patterns showed up.

In addition to the main change, this also makes some other performance improvements:

stickz commented 5 months ago

Thanks so much for this new code @kannibalox! Would you mind submitting this to the develop branch on my new rTorrent project? I would like to distribute this change to multiple platforms.

You'll know about in about 2 minutes if the change builds successfully with LTO. There are GitHub action runners waiting for you! I will test the change for you to ensure it works properly.

kannibalox commented 5 months ago

Unfortunately I already develop against vanilla while porting changes into jesec/rtorrent for personal use, and for the sake of my own sanity I can't add in support for a fork I'm not using.

stickz commented 5 months ago

This is totally understandable. I will backport these changes to my fork, like I did for your simplified queue entries.

If you change your mind and would like to use stickz/rtorrent in the future, I'll have most of the jesec/rtorrent changes back ported as well. I was able to greatly improve the UDNS implementation with your simplified queue entries!

stickz commented 5 months ago

@kannibalox This change crashes the torrent client near the end of a download. I have a limited stack trace, pointing to the newly revised function torrent::RequestList::delegate.

#0  0x00007ffff7e15a9e in torrent::RequestList::delegate(unsigned int) () from /lib/libtorrent.so.21
#1  0x00007ffff7e0e4e3 in torrent::PeerConnectionBase::try_request_pieces() () from /lib/libtorrent.so.21
#2  0x00007ffff7e0f0f0 in torrent::PeerConnection<(torrent::Download::ConnectionType)0>::fill_write_buffer() () from /lib/libtorrent.so.21
#3  0x00007ffff7e10dc2 in torrent::PeerConnection<(torrent::Download::ConnectionType)0>::event_write() () from /lib/libtorrent.so.21
#4  0x00007ffff7db1528 in torrent::PollEPoll::perform() () from /lib/libtorrent.so.21
#5  0x00007ffff7dd66c2 in torrent::thread_base::event_loop(torrent::thread_base*) () from /lib/libtorrent.so.21
#6  0x000055555559a513 in main (argc=1, argv=<optimized out>) at main.cc:508
kannibalox commented 5 months ago

Should be fixed now @stickz, let this be a lesson to me that no final little change is too small to thoroughly test.

stickz commented 5 months ago

Thanks @kannibalox this is confirmed fixed. The impact of this change is about 15-20% in terms of performance which is great.

I just have a quick question, since you seem to know how libtorrent works. When I startup my torrent client, it takes a few minutes to announce to thousands of UDP trackers. It also slows down my torrents because CPU usage is pegged at 100%.

Would it be possible to announce/scrape UDP trackers on a new thread, then receive success or failure, once the task has been completed? It seems to me like this task can easily be separated by calling the success or failure event on completion.

https://github.com/rakshasa/libtorrent/blob/91f8cf4b0358d9b4480079ca7798fa7d9aec76b5/src/torrent/tracker_list.cc#L320-L342

I'd be happy to provide you with any necessary profiles and test your changes, should this task be feasible.

kannibalox commented 5 months ago

I took a quick poke around the code and tried out a synthetic session with 10k UDP torrents, but didn't see anything immediately obvious. If you have profiles I wouldn't mind taking a look, but if it's truly CPU bound then threading probably won't be the answer.

anthonyryan1 commented 1 month ago

@rakshasa are you able to test and review this at your convenience?

stickz commented 1 month ago

@rakshasa are you able to test and review this at your convenience?

I've done extensive testing with this pull request, it should be ready to go.