From fc0ba38bc99b423e4a0aaaf821c6ca7d75b6e81e Mon Sep 17 00:00:00 2001 From: Charles Kerr Date: Wed, 15 Sep 2021 09:32:07 -0500 Subject: [PATCH] refactor: tr_quickfindFirstK --> std::partial_sort (#1794) * refactor: tr_quickfindFirstK --> std::partial_sort Remove `tr_quickfindFirstK()` and use `std::partial_sort()` instead. Co-authored-by: Mike Gelfand --- Transmission.xcodeproj/project.pbxproj | 20 +++- libtransmission/announcer.cc | 44 ++++----- libtransmission/peer-mgr.cc | 42 ++------ libtransmission/session.cc | 85 +++++----------- libtransmission/utils.cc | 130 ------------------------- libtransmission/utils.h | 3 - tests/libtransmission/utils-test.cc | 25 ----- 7 files changed, 70 insertions(+), 279 deletions(-) diff --git a/Transmission.xcodeproj/project.pbxproj b/Transmission.xcodeproj/project.pbxproj index 0526d458b..8a16ed77e 100644 --- a/Transmission.xcodeproj/project.pbxproj +++ b/Transmission.xcodeproj/project.pbxproj @@ -1515,7 +1515,7 @@ A25BFD68167BED3B0039D1AA /* variant.h */, A2EA522F1686AC0D00180493 /* quark.cc */, A2EA52301686AC0D00180493 /* quark.h */, - A2AF23C616B44FA0003BC59E /* log.c */, + A2AF23C616B44FA0003BC59E /* log.cc */, A2AF23C716B44FA0003BC59E /* log.h */, A2A4EA0B0DE106E8000CE197 /* ConvertUTF.h */, A2A4EA0A0DE106E8000CE197 /* ConvertUTF.c */, @@ -3076,6 +3076,7 @@ ., ); LD_RUNPATH_SEARCH_PATHS = "@executable_path/../Frameworks"; + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = Transmission; WRAPPER_EXTENSION = app; }; @@ -3090,6 +3091,7 @@ ., "third-party/libevent/include", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = transmissioncli; }; name = Debug; @@ -3107,6 +3109,7 @@ "$(inherited)", "-DHAVE_DAEMON", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "transmission-daemon"; }; name = Debug; @@ -3120,6 +3123,7 @@ ., "third-party/libevent/include", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "transmission-remote"; }; name = Debug; @@ -3263,6 +3267,7 @@ ., "third-party/libevent/include", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = transmissioncli; }; name = Release; @@ -3280,6 +3285,7 @@ ., ); LD_RUNPATH_SEARCH_PATHS = "@executable_path/../Frameworks"; + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = Transmission; WRAPPER_EXTENSION = app; }; @@ -3458,6 +3464,7 @@ ., ); LD_RUNPATH_SEARCH_PATHS = "@executable_path/../Frameworks"; + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = Transmission; WRAPPER_EXTENSION = app; }; @@ -3472,6 +3479,7 @@ ., "third-party/libevent/include", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = transmissioncli; }; name = "Release - Debug"; @@ -3523,6 +3531,7 @@ "$(inherited)", "-DHAVE_DAEMON", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "transmission-daemon"; }; name = "Release - Debug"; @@ -3536,6 +3545,7 @@ ., "third-party/libevent/include", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "transmission-remote"; }; name = "Release - Debug"; @@ -3594,7 +3604,7 @@ ); INFOPLIST_FILE = "macosx/QuickLookPlugin/QuickLookPlugin-Info.plist"; INSTALL_PATH = /Library/QuickLook; - OTHER_LDFLAGS = ""; + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "$(TARGET_NAME)"; WRAPPER_EXTENSION = qlgenerator; }; @@ -3611,7 +3621,7 @@ ); INFOPLIST_FILE = "macosx/QuickLookPlugin/QuickLookPlugin-Info.plist"; INSTALL_PATH = /Library/QuickLook; - OTHER_LDFLAGS = ""; + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "$(TARGET_NAME)"; WRAPPER_EXTENSION = qlgenerator; }; @@ -3628,7 +3638,7 @@ ); INFOPLIST_FILE = "macosx/QuickLookPlugin/QuickLookPlugin-Info.plist"; INSTALL_PATH = /Library/QuickLook; - OTHER_LDFLAGS = ""; + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "$(TARGET_NAME)"; WRAPPER_EXTENSION = qlgenerator; }; @@ -3684,6 +3694,7 @@ "$(inherited)", "-DHAVE_DAEMON", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "transmission-daemon"; }; name = Release; @@ -3697,6 +3708,7 @@ ., "third-party/libevent/include", ); + OTHER_LDFLAGS = "-lc++"; PRODUCT_NAME = "transmission-remote"; }; name = Release; diff --git a/libtransmission/announcer.cc b/libtransmission/announcer.cc index 6a82ef253..510f427fa 100644 --- a/libtransmission/announcer.cc +++ b/libtransmission/announcer.cc @@ -6,10 +6,12 @@ * */ +#include // std::partial_sort #include /* INT_MAX */ #include #include /* qsort() */ #include /* strcmp(), memcpy(), strncmp() */ +#include #include #include /* evtimer */ @@ -1567,17 +1569,15 @@ static void scrape_request_delegate( } } -static void multiscrape(tr_announcer* announcer, tr_ptrArray* tiers) +static void multiscrape(tr_announcer* announcer, std::vector const& tiers) { size_t request_count = 0; time_t const now = tr_time(); - size_t const tier_count = tr_ptrArraySize(tiers); tr_scrape_request requests[MAX_SCRAPES_PER_UPKEEP] = {}; /* batch as many info_hashes into a request as we can */ - for (size_t i = 0; i < tier_count; ++i) + for (auto* tier : tiers) { - auto* tier = static_cast(tr_ptrArrayNth(tiers, i)); struct tr_scrape_info* const scrape_info = tier->currentTracker->scrape_info; uint8_t const* hash = tier->tor->info.hash; bool found = false; @@ -1658,11 +1658,8 @@ static inline int countDownloaders(tr_tier const* tier) return tracker == nullptr ? 0 : tracker->downloaderCount + tracker->leecherCount; } -static int compareAnnounceTiers(void const* va, void const* vb) +static int compareAnnounceTiers(tr_tier const* a, tr_tier const* b) { - tr_tier const* a = (tr_tier const*)va; - tr_tier const* b = (tr_tier const*)vb; - /* prefer higher-priority events */ int const priority_a = a->announce_event_priority; int const priority_b = b->announce_event_priority; @@ -1714,8 +1711,8 @@ static void scrapeAndAnnounceMore(tr_announcer* announcer) time_t const now = tr_time(); /* build a list of tiers that need to be announced */ - auto announceMe = tr_ptrArray{}; - auto scrapeMe = tr_ptrArray{}; + auto announce_me = std::vector{}; + auto scrape_me = std::vector{}; tr_torrent* tor = nullptr; while ((tor = tr_torrentNext(announcer->session, tor)) != nullptr) { @@ -1727,12 +1724,12 @@ static void scrapeAndAnnounceMore(tr_announcer* announcer) if (tierNeedsToAnnounce(tier, now)) { - tr_ptrArrayInsertSorted(&announceMe, tier, compareAnnounceTiers); + announce_me.push_back(tier); } if (tierNeedsToScrape(tier, now)) { - tr_ptrArrayAppend(&scrapeMe, tier); + scrape_me.push_back(tier); } } } @@ -1741,22 +1738,25 @@ static void scrapeAndAnnounceMore(tr_announcer* announcer) * we can work through that queue much faster than announces * (thanks to multiscrape) _and_ the scrape responses will tell * us which swarms are interesting and should be announced next. */ - multiscrape(announcer, &scrapeMe); + multiscrape(announcer, scrape_me); /* Second, announce what we can. If there aren't enough slots * available, use compareAnnounceTiers to prioritize. */ - int n = MIN(tr_ptrArraySize(&announceMe), MAX_ANNOUNCES_PER_UPKEEP); - for (int i = 0; i < n; ++i) + if (announce_me.size() > MAX_ANNOUNCES_PER_UPKEEP) { - auto* tier = static_cast(tr_ptrArrayNth(&announceMe, i)); - tr_logAddTorDbg(tier->tor, "%s", "Announcing to tracker"); - dbgmsg(tier, "announcing tier %d of %d", i, n); - tierAnnounce(announcer, tier); + std::partial_sort( + std::begin(announce_me), + std::begin(announce_me) + MAX_ANNOUNCES_PER_UPKEEP, + std::end(announce_me), + [](auto const* a, auto const* b) { return compareAnnounceTiers(a, b) < 0; }); + announce_me.resize(MAX_ANNOUNCES_PER_UPKEEP); } - /* cleanup */ - tr_ptrArrayDestruct(&scrapeMe, nullptr); - tr_ptrArrayDestruct(&announceMe, nullptr); + for (auto*& tier : announce_me) + { + tr_logAddTorDbg(tier->tor, "%s", "Announcing to tracker"); + tierAnnounce(announcer, tier); + } } static void onUpkeepTimer(evutil_socket_t fd, short what, void* vannouncer) diff --git a/libtransmission/peer-mgr.cc b/libtransmission/peer-mgr.cc index f644cae5e..60ec7f3e0 100644 --- a/libtransmission/peer-mgr.cc +++ b/libtransmission/peer-mgr.cc @@ -6,6 +6,7 @@ * */ +#include // std::partial_sort #include /* error codes ERANGE, ... */ #include /* INT_MAX */ #include /* memcpy, memcmp, strstr */ @@ -4167,35 +4168,6 @@ static uint64_t getPeerCandidateScore(tr_torrent const* tor, struct peer_atom co return score; } -static int comparePeerCandidates(void const* va, void const* vb) -{ - int ret; - auto const* const a = static_cast(va); - auto const* const b = static_cast(vb); - - if (a->score < b->score) - { - ret = -1; - } - else if (a->score > b->score) - { - ret = 1; - } - else - { - ret = 0; - } - - return ret; -} - -/* Partial sorting -- selecting the k best candidates - Adapted from http://en.wikipedia.org/wiki/Selection_algorithm */ -static void selectPeerCandidates(struct peer_candidate* candidates, int candidate_count, int select_count) -{ - tr_quickfindFirstK(candidates, candidate_count, sizeof(struct peer_candidate), comparePeerCandidates, select_count); -} - #ifdef TR_ENABLE_ASSERTS static bool checkBestScoresComeFirst(struct peer_candidate const* candidates, int n, int k) @@ -4337,11 +4309,15 @@ static struct peer_candidate* getPeerCandidates(tr_session* session, int* candid } } - *candidateCount = walk - candidates; - - if (walk != candidates) + auto const n_candidates = walk - candidates; + *candidateCount = n_candidates; + if (n_candidates > max) { - selectPeerCandidates(candidates, walk - candidates, max); + std::partial_sort( + candidates, + candidates + max, + candidates + n_candidates, + [](auto const& a, auto const& b) { return a.score < b.score; }); } TR_ASSERT(checkBestScoresComeFirst(candidates, *candidateCount, max)); diff --git a/libtransmission/session.cc b/libtransmission/session.cc index 7d3356181..2c92a3d9f 100644 --- a/libtransmission/session.cc +++ b/libtransmission/session.cc @@ -6,10 +6,12 @@ * */ +#include // std::partial_sort #include /* ENOENT */ #include /* INT_MAX */ #include #include /* memcpy */ +#include #include @@ -3017,85 +3019,40 @@ int tr_sessionGetAntiBruteForceThreshold(tr_session const* session) return tr_rpcGetAntiBruteForceThreshold(session->rpcServer); } -struct TorrentAndPosition -{ - tr_torrent* tor; - int position; -}; - -static int compareTorrentAndPositions(void const* va, void const* vb) -{ - int ret; - auto const* a = static_cast(va); - auto const* b = static_cast(vb); - - if (a->position > b->position) - { - ret = 1; - } - else if (a->position < b->position) - { - ret = -1; - } - else - { - ret = 0; - } - - return ret; -} - void tr_sessionGetNextQueuedTorrents(tr_session* session, tr_direction direction, size_t num_wanted, tr_ptrArray* setme) { TR_ASSERT(tr_isSession(session)); TR_ASSERT(tr_isDirection(direction)); - /* build an array of the candidates */ - size_t n = tr_sessionCountTorrents(session); - struct TorrentAndPosition* candidates = tr_new(struct TorrentAndPosition, n); - size_t num_candidates = 0; + // build an array of the candidates + auto candidates = std::vector{}; + candidates.reserve(tr_sessionCountTorrents(session)); tr_torrent* tor = nullptr; - while ((tor = tr_torrentNext(session, tor)) != nullptr) { - if (!tr_torrentIsQueued(tor)) + if (tr_torrentIsQueued(tor) && (direction == tr_torrentGetQueueDirection(tor))) { - continue; + candidates.push_back(tor); } - - if (direction != tr_torrentGetQueueDirection(tor)) - { - continue; - } - - candidates[num_candidates].tor = tor; - candidates[num_candidates].position = tr_torrentGetQueuePosition(tor); - ++num_candidates; } - /* find the best n candidates */ - if (num_wanted > num_candidates) + // find the best n candidates + num_wanted = std::min(num_wanted, candidates.size()); + if (num_wanted < candidates.size()) { - num_wanted = num_candidates; - } - else if (num_wanted < num_candidates) - { - tr_quickfindFirstK( - candidates, - num_candidates, - sizeof(struct TorrentAndPosition), - compareTorrentAndPositions, - num_wanted); + std::partial_sort( + std::begin(candidates), + std::begin(candidates) + num_wanted, + std::end(candidates), + [](auto const* a, auto const* b) { return tr_torrentGetQueuePosition(a) < tr_torrentGetQueuePosition(b); }); + candidates.resize(num_wanted); } - /* add them to the return array */ - for (size_t i = 0; i < num_wanted; ++i) + // add them to the return array + for (auto* candidate : candidates) { - tr_ptrArrayAppend(setme, candidates[i].tor); + tr_ptrArrayAppend(setme, candidate); } - - /* cleanup */ - tr_free(candidates); } int tr_sessionCountQueueFreeSlots(tr_session* session, tr_direction dir) @@ -3118,7 +3075,9 @@ int tr_sessionCountQueueFreeSlots(tr_session* session, tr_direction dir) { /* is it the right activity? */ if (activity != tr_torrentGetActivity(tor)) + { continue; + } /* is it stalled? */ if (stalled_enabled) @@ -3132,7 +3091,9 @@ int tr_sessionCountQueueFreeSlots(tr_session* session, tr_direction dir) /* if we've reached the limit, no need to keep counting */ if (active_count >= max) + { return 0; + } } return max - active_count; diff --git a/libtransmission/utils.cc b/libtransmission/utils.cc index febdbea3c..e3bb32371 100644 --- a/libtransmission/utils.cc +++ b/libtransmission/utils.cc @@ -1072,136 +1072,6 @@ int tr_lowerBound( return first; } -/*** -**** -**** -***/ - -/* Byte-wise swap two items of size SIZE. - From glibc, written by Douglas C. Schmidt, LGPL 2.1 or higher */ -#define SWAP(a, b, size) \ - do \ - { \ - size_t __size = (size); \ - char* __a = (a); \ - char* __b = (b); \ - if (__a != __b) \ - { \ - do \ - { \ - char __tmp = *__a; \ - *__a++ = *__b; \ - *__b++ = __tmp; \ - } while (--__size > 0); \ - } \ - } while (0) - -static size_t quickfindPartition( - char* base, - size_t left, - size_t right, - size_t size, - tr_voidptr_compare_func compar, - size_t pivotIndex) -{ - size_t storeIndex; - - /* move pivot to the end */ - SWAP(base + (size * pivotIndex), base + (size * right), size); - - storeIndex = left; - - for (size_t i = left; i < right; ++i) - { - if ((*compar)(base + (size * i), base + (size * right)) <= 0) - { - SWAP(base + (size * storeIndex), base + (size * i), size); - ++storeIndex; - } - } - - /* move pivot to its final place */ - SWAP(base + (size * right), base + (size * storeIndex), size); - - /* sanity check the partition */ -#ifdef TR_ENABLE_ASSERTS - - TR_ASSERT(storeIndex >= left); - TR_ASSERT(storeIndex <= right); - - for (size_t i = left; i < storeIndex; ++i) - { - TR_ASSERT((*compar)(base + (size * i), base + (size * storeIndex)) <= 0); - } - - for (size_t i = storeIndex + 1; i <= right; ++i) - { - TR_ASSERT((*compar)(base + (size * i), base + (size * storeIndex)) >= 0); - } - -#endif - - return storeIndex; -} - -static void quickfindFirstK(char* base, size_t left, size_t right, size_t size, tr_voidptr_compare_func compar, size_t k) -{ - if (right > left) - { - size_t const pivotIndex = left + (right - left) / 2U; - - size_t const pivotNewIndex = quickfindPartition(base, left, right, size, compar, pivotIndex); - - if (pivotNewIndex > left + k) /* new condition */ - { - quickfindFirstK(base, left, pivotNewIndex - 1, size, compar, k); - } - else if (pivotNewIndex < left + k) - { - quickfindFirstK(base, pivotNewIndex + 1, right, size, compar, k + left - pivotNewIndex - 1); - } - } -} - -#ifdef TR_ENABLE_ASSERTS - -static void checkBestScoresComeFirst(char const* base, size_t nmemb, size_t size, tr_voidptr_compare_func compar, size_t k) -{ - size_t worstFirstPos = 0; - - for (size_t i = 1; i < k; ++i) - { - if ((*compar)(base + (size * worstFirstPos), base + (size * i)) < 0) - { - worstFirstPos = i; - } - } - - for (size_t i = 0; i < k; ++i) - { - TR_ASSERT((*compar)(base + (size * i), base + (size * worstFirstPos)) <= 0); - } - - for (size_t i = k; i < nmemb; ++i) - { - TR_ASSERT((*compar)(base + (size * i), base + (size * worstFirstPos)) >= 0); - } -} - -#endif - -void tr_quickfindFirstK(void* base, size_t nmemb, size_t size, tr_voidptr_compare_func compar, size_t k) -{ - if (k < nmemb) - { - quickfindFirstK(static_cast(base), 0, nmemb - 1, size, compar, k); - -#ifdef TR_ENABLE_ASSERTS - checkBestScoresComeFirst(static_cast(base), nmemb, size, compar, k); -#endif - } -} - /*** **** ***/ diff --git a/libtransmission/utils.h b/libtransmission/utils.h index 1154bab6e..dce2ce97a 100644 --- a/libtransmission/utils.h +++ b/libtransmission/utils.h @@ -218,9 +218,6 @@ int tr_lowerBound( tr_voidptr_compare_func compar, bool* exact_match) TR_GNUC_HOT TR_GNUC_NONNULL(1, 5, 6); -/** @brief moves the best k items to the first slots in the array. O(n) */ -void tr_quickfindFirstK(void* base, size_t nmemb, size_t size, tr_voidptr_compare_func compar, size_t k); - /** * @brief sprintf() a string into a newly-allocated buffer large enough to hold it * @return a newly-allocated string that can be freed with tr_free() diff --git a/tests/libtransmission/utils-test.cc b/tests/libtransmission/utils-test.cc index 1821565ef..27d47460a 100644 --- a/tests/libtransmission/utils-test.cc +++ b/tests/libtransmission/utils-test.cc @@ -197,31 +197,6 @@ TEST_F(UtilsTest, lowerbound) } } -TEST_F(UtilsTest, trQuickfindfirstk) -{ - auto const run_test = [](size_t const k, size_t const n, int* buf, int range) - { - // populate buf with random ints - std::generate(buf, buf + n, [range]() { return tr_rand_int_weak(range); }); - - // find the best k - tr_quickfindFirstK(buf, n, sizeof(int), compareInts, k); - - // confirm that the smallest K ints are in the first slots K slots in buf - auto const* highest_low = std::max_element(buf, buf + k); - auto const* lowest_high = std::min_element(buf + k, buf + n); - EXPECT_LE(highest_low, lowest_high); - }; - - auto constexpr K = size_t{ 10 }; - auto constexpr NumTrials = size_t{ 1000 }; - auto buf = std::array{}; - for (auto i = 0; i != NumTrials; ++i) - { - run_test(K, buf.size(), buf.data(), 100); - } -} - TEST_F(UtilsTest, trMemmem) { auto const haystack = std::string{ "abcabcabcabc" };