libtorrent icon indicating copy to clipboard operation
libtorrent copied to clipboard

Speed up delegating by returning multiple request assignments at once

Open kannibalox opened this issue 1 year ago • 7 comments

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:

  • Modifies Block::insert to return a NULL if the peer already exists, to avoid needing to scanning for the peer info twice.
  • BlockList uses a regular std::vector with the copy assignment/constructor disabled

kannibalox avatar Apr 13 '24 05:04 kannibalox

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.

stickz avatar Apr 13 '24 14:04 stickz

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.

kannibalox avatar Apr 13 '24 17:04 kannibalox

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 avatar Apr 14 '24 17:04 stickz

@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

stickz avatar Apr 15 '24 01:04 stickz

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

kannibalox avatar Apr 15 '24 02:04 kannibalox

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.

stickz avatar Apr 15 '24 13:04 stickz

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.

kannibalox avatar Apr 28 '24 13:04 kannibalox

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

anthonyryan1 avatar Aug 20 '24 14:08 anthonyryan1

@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.

stickz avatar Aug 20 '24 15:08 stickz