refactor: Use merge_sort instead of qsort for sorting.

.circleci/config.yml

@@ -68,6 +68,7 @@ jobs:
             clang
             cmake
             git
+            libbenchmark-dev
             libconfig-dev
             libgmock-dev
             libgtest-dev

CMakeLists.txt

@@ -323,6 +323,8 @@ set(toxcore_SOURCES
   toxcore/ping.h
   toxcore/shared_key_cache.c
   toxcore/shared_key_cache.h
+  toxcore/sort.c
+  toxcore/sort.h
   toxcore/state.c
   toxcore/state.h
   toxcore/TCP_client.c

other/analysis/gen-file.sh

@@ -15,7 +15,7 @@ CPPFLAGS+=("-Itoxav")
 CPPFLAGS+=("-Itoxencryptsave")
 CPPFLAGS+=("-Ithird_party/cmp")
 
-LDFLAGS=("-lopus" "-lsodium" "-lvpx" "-lpthread" "-lconfig" "-lgmock" "-lgtest")
+LDFLAGS=("-lopus" "-lsodium" "-lvpx" "-lpthread" "-lconfig" "-lgmock" "-lgtest" "-lbenchmark")
 LDFLAGS+=("-fuse-ld=gold")
 LDFLAGS+=("-Wl,--detect-odr-violations")
 LDFLAGS+=("-Wl,--warn-common")
@@ -27,7 +27,7 @@ put() {
   if [ "$SKIP_LINES" = "" ]; then
     echo "#line 1 \"$1\"" >>amalgamation.cc
   fi
-  cat "$1" >>amalgamation.cc
+  grep -v '^BENCHMARK_MAIN' "$1" >>amalgamation.cc
 }
 
 putmain() {

other/docker/modules/check

@@ -50,6 +50,10 @@ module "//c-toxcore/third_party:cmp" {
 module "//c-toxcore/toxencryptsave:defines" {
   header "toxencryptsave/defines.h"
 }
+module "@benchmark" {
+  textual header "/usr/include/benchmark/benchmark.h"
+  use std
+}
 module "@com_google_googletest//:gtest" {
   textual header "/usr/include/gmock/gmock.h"
   textual header "/usr/include/gtest/gtest.h"
@@ -83,9 +87,9 @@ class Context:
         pass
 
     def bzl_exports_files(
-            self,
-            srcs: list[str],
-            visibility: Optional[list[str]] = None,
+        self,
+        srcs: list[str],
+        visibility: Optional[list[str]] = None,
     ) -> None:
         pass
 
@@ -110,7 +114,7 @@ class Context:
             hdrs,
         }
 
-    def bzl_cc_test(
+    def bzl_cc_binary(
             self,
             name: str,
             srcs: Iterable[str] = tuple(),
@@ -161,7 +165,8 @@ def main() -> None:
                     "load": ctx.bzl_load,
                     "exports_files": ctx.bzl_exports_files,
                     "cc_library": ctx.bzl_cc_library,
-                    "cc_test": ctx.bzl_cc_test,
+                    "cc_binary": ctx.bzl_cc_binary,
+                    "cc_test": ctx.bzl_cc_binary,
                     "cc_fuzz_test": ctx.bzl_cc_fuzz_test,
                     "select": ctx.bzl_select,
                     "glob": ctx.bzl_glob,

other/docker/modules/modules.Dockerfile

@@ -3,6 +3,7 @@ FROM alpine:3.19.0
 
 RUN ["apk", "add", "--no-cache", \
  "bash", \
+ "benchmark-dev", \
  "clang", \
  "gtest-dev", \
  "libconfig-dev", \

toxcore/BUILD.bazel

@@ -1,4 +1,4 @@
-load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test")
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library", "cc_test")
 load("@rules_fuzzing//fuzzing:cc_defs.bzl", "cc_fuzz_test")
 
 exports_files(
@@ -100,14 +100,55 @@ cc_test(
     size = "small",
     srcs = ["util_test.cc"],
     deps = [
-        ":crypto_core",
-        ":crypto_core_test_util",
         ":util",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",
     ],
 )
 
+cc_library(
+    name = "sort",
+    srcs = ["sort.c"],
+    hdrs = ["sort.h"],
+    deps = [
+        ":attributes",
+        ":ccompat",
+        ":util",
+    ],
+)
+
+cc_library(
+    name = "sort_test_util",
+    testonly = True,
+    srcs = ["sort_test_util.cc"],
+    hdrs = ["sort_test_util.hh"],
+    deps = [":sort"],
+)
+
+cc_test(
+    name = "sort_test",
+    size = "small",
+    srcs = ["sort_test.cc"],
+    deps = [
+        ":sort",
+        ":sort_test_util",
+        "@com_google_googletest//:gtest",
+        "@com_google_googletest//:gtest_main",
+    ],
+)
+
+cc_binary(
+    name = "sort_bench",
+    testonly = True,
+    srcs = ["sort_bench.cc"],
+    deps = [
+        ":mem",
+        ":sort",
+        ":sort_test_util",
+        "@benchmark",
+    ],
+)
+
 cc_library(
     name = "logger",
     srcs = ["logger.c"],
@@ -439,6 +480,7 @@ cc_library(
         ":network",
         ":ping_array",
         ":shared_key_cache",
+        ":sort",
         ":state",
         ":util",
     ],
@@ -718,6 +760,7 @@ cc_library(
         ":network",
         ":onion",
         ":shared_key_cache",
+        ":sort",
         ":timed_auth",
         ":util",
     ],
@@ -821,6 +864,7 @@ cc_library(
         ":onion",
         ":onion_announce",
         ":ping_array",
+        ":sort",
         ":timed_auth",
         ":util",
     ],
@@ -986,9 +1030,11 @@ cc_library(
         ":crypto_core",
         ":friend_connection",
         ":logger",
+        ":mem",
         ":mono_time",
         ":net_crypto",
         ":network",
+        ":sort",
         ":state",
         ":util",
     ],

toxcore/DHT.c

@@ -24,7 +24,9 @@
 #include "ping.h"
 #include "ping_array.h"
 #include "shared_key_cache.h"
+#include "sort.h"
 #include "state.h"
+#include "util.h"
 
 /** The timeout after which a node is discarded completely. */
 #define KILL_NODE_TIMEOUT (BAD_NODE_TIMEOUT + PING_INTERVAL)
@@ -755,49 +757,6 @@ int get_close_nodes(
                is_lan, want_announce);
 }
 
-typedef struct DHT_Cmp_Data {
-    uint64_t cur_time;
-    const uint8_t *base_public_key;
-    Client_data entry;
-} DHT_Cmp_Data;
-
-non_null()
-static int dht_cmp_entry(const void *a, const void *b)
-{
-    const DHT_Cmp_Data *cmp1 = (const DHT_Cmp_Data *)a;
-    const DHT_Cmp_Data *cmp2 = (const DHT_Cmp_Data *)b;
-    const Client_data entry1 = cmp1->entry;
-    const Client_data entry2 = cmp2->entry;
-    const uint8_t *cmp_public_key = cmp1->base_public_key;
-
-    const bool t1 = assoc_timeout(cmp1->cur_time, &entry1.assoc4) && assoc_timeout(cmp1->cur_time, &entry1.assoc6);
-    const bool t2 = assoc_timeout(cmp2->cur_time, &entry2.assoc4) && assoc_timeout(cmp2->cur_time, &entry2.assoc6);
-
-    if (t1 && t2) {
-        return 0;
-    }
-
-    if (t1) {
-        return -1;
-    }
-
-    if (t2) {
-        return 1;
-    }
-
-    const int closest = id_closest(cmp_public_key, entry1.public_key, entry2.public_key);
-
-    if (closest == 1) {
-        return 1;
-    }
-
-    if (closest == 2) {
-        return -1;
-    }
-
-    return 0;
-}
-
 #ifdef CHECK_ANNOUNCE_NODE
 non_null()
 static void set_announce_node_in_list(Client_data *list, uint32_t list_len, const uint8_t *public_key)
@@ -914,31 +873,117 @@ static bool store_node_ok(const Client_data *client, uint64_t cur_time, const ui
            || id_closest(comp_public_key, client->public_key, public_key) == 2;
 }
 
+typedef struct Client_data_Cmp {
+    const Memory *mem;
+    uint64_t cur_time;
+    const uint8_t *comp_public_key;
+} Client_data_Cmp;
+
 non_null()
-static void sort_client_list(const Memory *mem, Client_data *list, uint64_t cur_time, unsigned int length,
-                             const uint8_t *comp_public_key)
+static int client_data_cmp(const Client_data_Cmp *cmp, const Client_data *entry1, const Client_data *entry2)
 {
-    // Pass comp_public_key to qsort with each Client_data entry, so the
-    // comparison function can use it as the base of comparison.
-    DHT_Cmp_Data *cmp_list = (DHT_Cmp_Data *)mem_valloc(mem, length, sizeof(DHT_Cmp_Data));
+    const bool t1 = assoc_timeout(cmp->cur_time, &entry1->assoc4) && assoc_timeout(cmp->cur_time, &entry1->assoc6);
+    const bool t2 = assoc_timeout(cmp->cur_time, &entry2->assoc4) && assoc_timeout(cmp->cur_time, &entry2->assoc6);
 
-    if (cmp_list == nullptr) {
-        return;
+    if (t1 && t2) {
+        return 0;
     }
 
-    for (uint32_t i = 0; i < length; ++i) {
-        cmp_list[i].cur_time = cur_time;
-        cmp_list[i].base_public_key = comp_public_key;
-        cmp_list[i].entry = list[i];
+    if (t1) {
+        return -1;
     }
 
-    qsort(cmp_list, length, sizeof(DHT_Cmp_Data), dht_cmp_entry);
+    if (t2) {
+        return 1;
+    }
 
-    for (uint32_t i = 0; i < length; ++i) {
-        list[i] = cmp_list[i].entry;
+    const int closest = id_closest(cmp->comp_public_key, entry1->public_key, entry2->public_key);
+
+    if (closest == 1) {
+        return 1;
+    }
+
+    if (closest == 2) {
+        return -1;
+    }
+
+    return 0;
+}
+
+non_null()
+static bool client_data_less_handler(const void *object, const void *a, const void *b)
+{
+    const Client_data_Cmp *cmp = (const Client_data_Cmp *)object;
+    const Client_data *entry1 = (const Client_data *)a;
+    const Client_data *entry2 = (const Client_data *)b;
+
+    return client_data_cmp(cmp, entry1, entry2) < 0;
+}
+
+non_null()
+static const void *client_data_get_handler(const void *arr, uint32_t index)
+{
+    const Client_data *entries = (const Client_data *)arr;
+    return &entries[index];
+}
+
+non_null()
+static void client_data_set_handler(void *arr, uint32_t index, const void *val)
+{
+    Client_data *entries = (Client_data *)arr;
+    const Client_data *entry = (const Client_data *)val;
+    entries[index] = *entry;
+}
+
+non_null()
+static void *client_data_subarr_handler(void *arr, uint32_t index, uint32_t size)
+{
+    Client_data *entries = (Client_data *)arr;
+    return &entries[index];
+}
+
+non_null()
+static void *client_data_alloc_handler(const void *object, uint32_t size)
+{
+    const Client_data_Cmp *cmp = (const Client_data_Cmp *)object;
+    Client_data *tmp = (Client_data *)mem_valloc(cmp->mem, size, sizeof(Client_data));
+
+    if (tmp == nullptr) {
+        return nullptr;
     }
 
-    mem_delete(mem, cmp_list);
+    return tmp;
+}
+
+non_null()
+static void client_data_delete_handler(const void *object, void *arr, uint32_t size)
+{
+    const Client_data_Cmp *cmp = (const Client_data_Cmp *)object;
+    mem_delete(cmp->mem, arr);
+}
+
+static const Sort_Funcs client_data_cmp_funcs = {
+    client_data_less_handler,
+    client_data_get_handler,
+    client_data_set_handler,
+    client_data_subarr_handler,
+    client_data_alloc_handler,
+    client_data_delete_handler,
+};
+
+non_null()
+static void sort_client_list(const Memory *mem, Client_data *list, uint64_t cur_time, unsigned int length,
+                             const uint8_t *comp_public_key)
+{
+    // Pass comp_public_key to merge_sort with each Client_data entry, so the
+    // comparison function can use it as the base of comparison.
+    const Client_data_Cmp cmp = {
+        mem,
+        cur_time,
+        comp_public_key,
+    };
+
+    merge_sort(list, length, &cmp, &client_data_cmp_funcs);
 }
 
 non_null()

toxcore/Makefile.inc

@@ -86,6 +86,8 @@ libtoxcore_la_SOURCES = ../third_party/cmp/cmp.c \
                         ../toxcore/ping.c \
                         ../toxcore/shared_key_cache.h \
                         ../toxcore/shared_key_cache.c \
+                        ../toxcore/sort.h \
+                        ../toxcore/sort.c \
                         ../toxcore/state.h \
                         ../toxcore/state.c \
                         ../toxcore/tox.h \