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

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kannibalox
Copy link

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

@stickz
Copy link
Contributor

stickz commented Apr 13, 2024

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
Copy link
Author

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
Copy link
Contributor

stickz commented Apr 14, 2024

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 added a commit to stickz/rtorrent that referenced this pull request Apr 14, 2024
From Kannibalox:
rakshasa/libtorrent#250

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
@stickz
Copy link
Contributor

stickz commented Apr 15, 2024

@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
Copy link
Author

kannibalox commented Apr 15, 2024

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

@stickz
Copy link
Contributor

stickz commented Apr 15, 2024

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.

void
TrackerList::receive_success(Tracker* tb, AddressList* l) {
iterator itr = find(tb);
if (itr == end() || tb->is_busy())
throw internal_error("TrackerList::receive_success(...) called but the iterator is invalid.");
// Promote the tracker to the front of the group since it was
// successfull.
itr = promote(itr);
l->sort();
l->erase(std::unique(l->begin(), l->end()), l->end());
LT_LOG_TRACKER(INFO, "received %u peers (url:%s)", l->size(), tb->url().c_str());
tb->m_success_time_last = cachedTime.seconds();
tb->m_success_counter++;
tb->m_failed_counter = 0;
tb->m_latest_sum_peers = l->size();
tb->m_latest_new_peers = m_slot_success(tb, l);
}

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

@kannibalox
Copy link
Author

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.

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

2 participants