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.

Sign up for GitHub

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

A few optimizations to lower CPU usage #7898

Open
wants to merge 5 commits into
base: RC_2_0
Choose a base branch
from
+42 −27
Open

A few optimizations to lower CPU usage #7898

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
16caed4
Adds caching to `is_i2p_url`
bolshoytoster Mar 1, 2025
a535e70
Adds a few optimizations
bolshoytoster Mar 1, 2025
9e59b89
Prevents `torrent::update_tracker_timer` from being called twice
bolshoytoster Mar 1, 2025
e6b3eae
Move more `session_settings::get_bool`s out of a loop
bolshoytoster Mar 2, 2025
b7890d8
Makes `announce_to_all_{tiers,trackers}` const
bolshoytoster Mar 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/string_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ POSSIBILITY OF SUCH DAMAGE.
#include <cstdlib> // for malloc
#include <cstring> // for strlen
#include <algorithm> // for search
#include <unordered_map>

namespace libtorrent {

Expand Down Expand Up @@ -400,12 +401,21 @@ namespace libtorrent {

bool is_i2p_url(std::string const& url)
{
static std::unordered_map<std::string, bool> cache;
Copy link
Contributor

@Chocobo1 Chocobo1 Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure my concerns are valid but anyway:

  1. What do you think about putting an upper limit on how many entries in the cache? IMO it isn't ideal for it to be unlimited. I suppose 1024 would suffice for most cases. Also this implies it needs some kind of cache replacement policy, for example LRU. Maybe libtorrent/boost provides such container?
  2. If I understand correctly, using static means it will be shared across all libtorrent sessions which isn't ideal either. It should be local to a specific torrent/session or whatever scope that make sense.
  3. Maybe this function requires to be thread-safe.

Copy link
Author

@bolshoytoster bolshoytoster Mar 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

  1. It looks like libtorrent implements an LRU cache in src/file_view_pool.cpp, but that's specific to the file pool. Boost also has an implementation, but it's under ::compute, so it can't be used here. I could implement one, but I'm not sure if it's needed - my qbittorrent uses 242 tracker urls in total, being ~8kb in text. Round it up to 10kb to account for std::unordered_map's overhead, and even 1024 urls should be only ~40kb.

2 & 3. I hadn't considered multiple threads, but I think I could just use thread_local instead of static.

I think we could get rid of this cache if announce_entry stored the result of is_i2p_url along with it's url, although I'm not sure if that could be added, since it looks like announce_entry is part of the ABI.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this cache is the only controversial part of this patch. It seems more natural to record whether a tracker is i2p or not once, and not call this function repeatedly. It's only 1 bit of information.
The ABI issue is a challenge though. in master the announce entries have been separated into internal and exported types, so it's easy to add internal fields without affecting the ABI

Copy link
Author

Choose a reason for hiding this comment

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

Since it's only one bit, could it be put in padding, like verified? Or could that cause some issues?

auto i2p = cache.find(url);
if (i2p != cache.end())
{
return i2p->second;
}

using std::ignore;
std::string hostname;
error_code ec;
std::tie(ignore, ignore, hostname, ignore, ignore)
= parse_url_components(url, ec);
return string_ends_with(hostname, ".i2p");
bool ends_with = string_ends_with(hostname, ".i2p");
cache.insert({url, ends_with});
return ends_with;
}

#endif
Expand Down
57 changes: 31 additions & 26 deletions src/torrent.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2922,7 +2922,7 @@ namespace {
for (auto& aep : aeps)
{
if (aep.socket != s) continue;
std::swap(aeps[valid_endpoints], aep);
if (&aeps[valid_endpoints] != &aep) std::swap(aeps[valid_endpoints], aep);
valid_endpoints++;
return;
}
Expand Down Expand Up @@ -3114,14 +3114,16 @@ namespace {
// so that each one should get at least one announce
std::vector<announce_state> listen_socket_states;

bool announce_to_all_tiers = settings().get_bool(settings_pack::announce_to_all_tiers);
bool announce_to_all_trackers = settings().get_bool(settings_pack::announce_to_all_trackers);
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this repo policy on const but I would suggest adding them anyway:

Suggested change
bool announce_to_all_tiers = settings().get_bool(settings_pack::announce_to_all_tiers);
bool announce_to_all_trackers = settings().get_bool(settings_pack::announce_to_all_trackers);
bool const announce_to_all_tiers = settings().get_bool(settings_pack::announce_to_all_tiers);
bool const announce_to_all_trackers = settings().get_bool(settings_pack::announce_to_all_trackers);

Copy link
Author

Choose a reason for hiding this comment

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

Whoops, I'm used to rust, where things are const by default.

#ifndef TORRENT_DISABLE_LOGGING
int idx = -1;
if (should_log())
{
debug_log("*** announce: "
"[ announce_to_all_tiers: %d announce_to_all_trackers: %d num_trackers: %d ]"
, settings().get_bool(settings_pack::announce_to_all_tiers)
, settings().get_bool(settings_pack::announce_to_all_trackers)
, announce_to_all_tiers
, announce_to_all_trackers
, int(m_trackers.size()));
}
#endif
Expand Down Expand Up @@ -3195,24 +3197,24 @@ namespace {
}
#endif

if (settings().get_bool(settings_pack::announce_to_all_tiers)
&& !settings().get_bool(settings_pack::announce_to_all_trackers)
if (announce_to_all_tiers
&& !announce_to_all_trackers
&& state.sent_announce
&& ae.tier <= state.tier
&& state.tier != INT_MAX)
continue;

if (ae.tier > state.tier && state.sent_announce
&& !settings().get_bool(settings_pack::announce_to_all_tiers)) continue;
&& !announce_to_all_tiers) continue;
if (a.is_working()) { state.tier = ae.tier; state.sent_announce = false; }
if (!a.can_announce(now, is_seed(), ae.fail_limit))
{
// this counts
if (a.is_working())
{
state.sent_announce = true;
if (!settings().get_bool(settings_pack::announce_to_all_trackers)
&& !settings().get_bool(settings_pack::announce_to_all_tiers))
if (!announce_to_all_trackers
&& !announce_to_all_tiers)
{
state.done = true;
}
Expand Down Expand Up @@ -3289,8 +3291,8 @@ namespace {

state.sent_announce = true;
if (a.is_working()
&& !settings().get_bool(settings_pack::announce_to_all_trackers)
&& !settings().get_bool(settings_pack::announce_to_all_tiers))
&& !announce_to_all_trackers
&& !announce_to_all_tiers)
{
state.done = true;
}
Expand Down Expand Up @@ -9901,16 +9903,18 @@ namespace {

time_point32 next_announce = time_point32::max();

std::vector<timer_state> listen_socket_states;
std::map<std::weak_ptr<aux::listen_socket_t>, timer_state, std::owner_less<std::weak_ptr<aux::listen_socket_t>>> listen_socket_states;

bool announce_to_all_tiers = settings().get_bool(settings_pack::announce_to_all_tiers);
bool announce_to_all_trackers = settings().get_bool(settings_pack::announce_to_all_trackers);
#ifndef TORRENT_DISABLE_LOGGING
int idx = -1;
if (should_log())
{
debug_log("*** update_tracker_timer: "
"[ announce_to_all_tiers: %d announce_to_all_trackers: %d num_trackers: %d ]"
, settings().get_bool(settings_pack::announce_to_all_tiers)
, settings().get_bool(settings_pack::announce_to_all_trackers)
, announce_to_all_tiers
, announce_to_all_trackers
, int(m_trackers.size()));
}
#endif
Expand All @@ -9921,14 +9925,12 @@ namespace {
#endif
for (auto const& aep : t.endpoints)
{
auto aep_state_iter = std::find_if(listen_socket_states.begin(), listen_socket_states.end()
, [&](timer_state const& s) { return s.socket == aep.socket; });
auto aep_state_iter = listen_socket_states.find(aep.socket.get_ptr());
if (aep_state_iter == listen_socket_states.end())
{
listen_socket_states.emplace_back(aep.socket);
aep_state_iter = listen_socket_states.end() - 1;
aep_state_iter = listen_socket_states.insert({aep.socket.get_ptr(), timer_state(aep.socket)}).first;
}
timer_state& ep_state = *aep_state_iter;
timer_state& ep_state = aep_state_iter->second;

if (!aep.enabled) continue;
for (protocol_version const ih : all_versions)
Expand All @@ -9952,16 +9954,16 @@ namespace {
}
#endif

if (settings().get_bool(settings_pack::announce_to_all_tiers)
&& !settings().get_bool(settings_pack::announce_to_all_trackers)
if (announce_to_all_tiers
&& !announce_to_all_trackers
&& state.found_working
&& t.tier <= state.tier
&& state.tier != INT_MAX)
continue;

if (t.tier > state.tier)
{
if (!settings().get_bool(settings_pack::announce_to_all_tiers)) break;
if (!announce_to_all_tiers) break;
state.found_working = false;
}
state.tier = t.tier;
Expand All @@ -9978,17 +9980,17 @@ namespace {
}
if (a.is_working()) state.found_working = true;
if (state.found_working
&& !settings().get_bool(settings_pack::announce_to_all_trackers)
&& !settings().get_bool(settings_pack::announce_to_all_tiers))
&& !announce_to_all_trackers
&& !announce_to_all_tiers)
state.done = true;
}
}

if (std::all_of(listen_socket_states.begin(), listen_socket_states.end()
, [supports_protocol](timer_state const& s) {
, [supports_protocol](std::pair<std::weak_ptr<aux::listen_socket_t>, timer_state> const& s) {
for (protocol_version const ih : all_versions)
{
if (supports_protocol[ih] && !s.state[ih].done)
if (supports_protocol[ih] && !s.second.state[ih].done)
return false;
}
return true;
Expand Down Expand Up @@ -12310,7 +12312,10 @@ namespace {
{
announce_with_tracker(r.event);
Copy link
Owner

Choose a reason for hiding this comment

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

if this call fails, update_tracker_timer() won't be called, with your patch. I'm not sure exactly what the consequence of that is, but it's possible the tracker will be tried over and over again in quick succession.
I would feel better about this change if you would check the return value here and call update_tracker_timer() if it fails. Or perhaps even better, make announce_with_tracker() update the timer unconditionally.

Copy link
Author

Choose a reason for hiding this comment

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

torrent::announce_with_tracker returns void, so there's no way to know if it fails.

In all cases where torrent::announce_with_tracker returns early (fails), there are either no trackers, or libtorrent isn't announcing. In both cases, torrent::update_tracker_timer would do nothing anyway, so I don't think it matters.

}
update_tracker_timer(aux::time_now32());
else
{
update_tracker_timer(aux::time_now32());
}
}

#ifndef TORRENT_DISABLE_LOGGING
Expand Down