refactor: Use merge_sort instead of qsort for sorting.

.circleci/config.yml

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

other/analysis/gen-file.sh

@@ -57,6 +57,7 @@ FIND_QUERY="$FIND_QUERY -and -not -name amalgamation.cc"
 FIND_QUERY="$FIND_QUERY -and -not -name av_test.c"
 FIND_QUERY="$FIND_QUERY -and -not -name cracker.c"
 FIND_QUERY="$FIND_QUERY -and -not -name version_test.c"
+FIND_QUERY="$FIND_QUERY -and -not -name '*_bench.cc'"
 FIND_QUERY="$FIND_QUERY -and -not -name '*_fuzz_test.cc'"
 FIND_QUERY="$FIND_QUERY -and -not -wholename './testing/fuzzing/*'"
 FIND_QUERY="$FIND_QUERY -and -not -wholename './third_party/cmp/examples/*'"

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"
@@ -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(
@@ -95,19 +95,37 @@ cc_library(
     ],
 )
 
+cc_library(
+    name = "util_test_util",
+    testonly = True,
+    srcs = ["util_test_util.cc"],
+    hdrs = ["util_test_util.hh"],
+    deps = [":util"],
+)
+
 cc_test(
     name = "util_test",
     size = "small",
     srcs = ["util_test.cc"],
     deps = [
-        ":crypto_core",
-        ":crypto_core_test_util",
         ":util",
+        ":util_test_util",
         "@com_google_googletest//:gtest",
         "@com_google_googletest//:gtest_main",
     ],
 )
 
+cc_binary(
+    name = "util_bench",
+    testonly = True,
+    srcs = ["util_bench.cc"],
+    deps = [
+        ":util",
+        ":util_test_util",
+        "@benchmark",
+    ],
+)
+
 cc_library(
     name = "logger",
     srcs = ["logger.c"],
@@ -968,6 +986,7 @@ cc_library(
         ":crypto_core",
         ":friend_connection",
         ":logger",
+        ":mem",
         ":mono_time",
         ":net_crypto",
         ":network",

toxcore/DHT.c

@@ -25,6 +25,7 @@
 #include "ping_array.h"
 #include "shared_key_cache.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 +756,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 +872,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/crypto_core_test.cc

@@ -17,6 +17,35 @@ using SecretKey = std::array<uint8_t, CRYPTO_SECRET_KEY_SIZE>;
 using Signature = std::array<uint8_t, CRYPTO_SIGNATURE_SIZE>;
 using Nonce = std::array<uint8_t, CRYPTO_NONCE_SIZE>;
 
+TEST(PkEqual, TwoRandomIdsAreNotEqual)
+{
+    std::mt19937 rng;
+    std::uniform_int_distribution<unsigned short> dist{0, UINT8_MAX};
+
+    uint8_t pk1[CRYPTO_PUBLIC_KEY_SIZE];
+    uint8_t pk2[CRYPTO_PUBLIC_KEY_SIZE];
+
+    std::generate(std::begin(pk1), std::end(pk1), [&]() { return dist(rng); });
+    std::generate(std::begin(pk2), std::end(pk2), [&]() { return dist(rng); });
+
+    EXPECT_FALSE(pk_equal(pk1, pk2));
+}
+
+TEST(PkEqual, IdCopyMakesKeysEqual)
+{
+    std::mt19937 rng;
+    std::uniform_int_distribution<unsigned short> dist{0, UINT8_MAX};
+
+    uint8_t pk1[CRYPTO_PUBLIC_KEY_SIZE];
+    uint8_t pk2[CRYPTO_PUBLIC_KEY_SIZE] = {0};
+
+    std::generate(std::begin(pk1), std::end(pk1), [&]() { return dist(rng); });
+
+    pk_copy(pk2, pk1);
+
+    EXPECT_TRUE(pk_equal(pk1, pk2));
+}
+
 TEST(CryptoCore, EncryptLargeData)
 {
     Test_Random rng;