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

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;

libtransmission/announcer.cc

@@ -6,10 +6,12 @@
  *
  */
 
+#include <algorithm> // std::partial_sort
 #include <limits.h> /* INT_MAX */
 #include <stdio.h>
 #include <stdlib.h> /* qsort() */
 #include <string.h> /* strcmp(), memcpy(), strncmp() */
+#include <vector>
 
 #include <event2/buffer.h>
 #include <event2/event.h> /* 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<tr_tier*> 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_tier*>(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 announce_me = std::vector<tr_tier*>{};
-    auto scrapeMe = tr_ptrArray{};
+    auto scrape_me = std::vector<tr_tier*>{};
     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);
+    if (announce_me.size() > MAX_ANNOUNCES_PER_UPKEEP)
-    for (int i = 0; i < n; ++i)
     {
-        auto* tier = static_cast<tr_tier*>(tr_ptrArrayNth(&announceMe, i));
+        std::partial_sort(
-        tr_logAddTorDbg(tier->tor, "%s", "Announcing to tracker");
+            std::begin(announce_me),
-        dbgmsg(tier, "announcing tier %d of %d", i, n);
+            std::begin(announce_me) + MAX_ANNOUNCES_PER_UPKEEP,
-        tierAnnounce(announcer, tier);
+            std::end(announce_me),
+            [](auto const* a, auto const* b) { return compareAnnounceTiers(a, b) < 0; });
+        announce_me.resize(MAX_ANNOUNCES_PER_UPKEEP);
     }
 
-    /* cleanup */
+    for (auto*& tier : announce_me)
-    tr_ptrArrayDestruct(&scrapeMe, nullptr);
+    {
-    tr_ptrArrayDestruct(&announceMe, nullptr);
+        tr_logAddTorDbg(tier->tor, "%s", "Announcing to tracker");
+        tierAnnounce(announcer, tier);
+    }
 }
 
 static void onUpkeepTimer(evutil_socket_t fd, short what, void* vannouncer)

libtransmission/peer-mgr.cc

@@ -6,6 +6,7 @@
  *
  */
 
+#include <algorithm> // std::partial_sort
 #include <errno.h> /* error codes ERANGE, ... */
 #include <limits.h> /* INT_MAX */
 #include <string.h> /* 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<struct peer_candidate const*>(va);
-    auto const* const b = static_cast<struct peer_candidate const*>(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;
+    auto const n_candidates = walk - candidates;
-
+    *candidateCount = n_candidates;
-    if (walk != 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));

libtransmission/session.cc

@@ -6,10 +6,12 @@
  *
  */
 
+#include <algorithm> // std::partial_sort
 #include <errno.h> /* ENOENT */
 #include <limits.h> /* INT_MAX */
 #include <stdlib.h>
 #include <string.h> /* memcpy */
+#include <vector>
 
 #include <signal.h>
 
@@ -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<struct TorrentAndPosition const*>(va);
-    auto const* b = static_cast<struct TorrentAndPosition const*>(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 */
+    // build an array of the candidates
-    size_t n = tr_sessionCountTorrents(session);
+    auto candidates = std::vector<tr_torrent*>{};
-    struct TorrentAndPosition* candidates = tr_new(struct TorrentAndPosition, n);
+    candidates.reserve(tr_sessionCountTorrents(session));
-    size_t num_candidates = 0;
     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 */
+    // find the best n candidates
-    if (num_wanted > num_candidates)
+    num_wanted = std::min(num_wanted, candidates.size());
+    if (num_wanted < candidates.size())
     {
-        num_wanted = num_candidates;
+        std::partial_sort(
-    }
+            std::begin(candidates),
-    else if (num_wanted < num_candidates)
+            std::begin(candidates) + num_wanted,
-    {
+            std::end(candidates),
-        tr_quickfindFirstK(
+            [](auto const* a, auto const* b) { return tr_torrentGetQueuePosition(a) < tr_torrentGetQueuePosition(b); });
-            candidates,
+        candidates.resize(num_wanted);
-            num_candidates,
-            sizeof(struct TorrentAndPosition),
-            compareTorrentAndPositions,
-            num_wanted);
     }
 
-    /* add them to the return array */
+    // add them to the return array
-    for (size_t i = 0; i < num_wanted; ++i)
+    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;

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<char*>(base), 0, nmemb - 1, size, compar, k);
-
-#ifdef TR_ENABLE_ASSERTS
-        checkBestScoresComeFirst(static_cast<char const*>(base), nmemb, size, compar, k);
-#endif
-    }
-}
-
 /***
 ****
 ***/

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()

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<int, 100>{};
-    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" };