Separate speedtest setup, process review comments

Author: omoerbeek

pdns/recursordist/pdns_recursor.cc

@@ -1513,6 +1513,9 @@ void startDoResolve(void* arg) // NOLINT(readability-function-cognitive-complexi
 
       if (!ret.empty()) {
 #ifdef notyet
+        // As dedupping is relatively expensive do not dedup in general. We do have a few cases
+        // where we call dedup explicitly, e.g. when doing NAT64 or when adding NSEC records in
+        // doCNAMECacheCheck
         pdns::dedupRecords(ret);
 #endif
         pdns::orderAndShuffle(ret, false);

pdns/recursordist/syncres.cc

@@ -4448,6 +4448,7 @@ void SyncRes::sanitizeRecordsPass2(const std::string& prefix, LWResult& lwr, con
     lwr.d_records = std::move(vec);
   }
 #ifdef notyet
+  // As dedupping is relatively expensive and having dup records not really hurts as far as we have seen, do not dedup.
   if (auto count = pdns::dedupRecords(lwr.d_records); count > 0) {
     LOG(prefix << qname << ": Removed " << count << " duplicate records from response received from " << auth << endl);
   }

pdns/shuffle.cc

@@ -143,7 +143,7 @@ void pdns::orderAndShuffle(vector<DNSRecord>& rrs, bool includingAdditionals)
 
 unsigned int pdns::dedupRecords(vector<DNSRecord>& rrs)
 {
-  // This function tries to avoid unneccesary work
+  // This function tries to avoid unnecessary work
   // First a vector with zero or one element does not need dedupping
   if (rrs.size() <= 1) {
     return 0;
@@ -174,7 +174,7 @@ unsigned int pdns::dedupRecords(vector<DNSRecord>& rrs)
   }
 
   // We avoid calling erase, as it calls a lot of move constructors. This can hurt, especially if
-  // you call it on a large vector muliple times.
+  // you call it on a large vector multiple times.
   // So we just take the elements that are unique
   std::vector<DNSRecord> ret;
   ret.reserve(rrs.size() - numDups);

pdns/speedtest.cc

@@ -1186,18 +1186,7 @@ struct DedupRecordsTest
 {
   explicit DedupRecordsTest(size_t howmany, bool dedup, bool withdup = false) : d_howmany(howmany), d_dedup(dedup), d_withdup(withdup)
   {
-  }
-
-  [[nodiscard]] string getName() const
-  {
-    return std::to_string(d_howmany) + " DedupRecords" + std::string(d_dedup ? "" : " (generate only)") +
-      std::string(d_withdup ? " (with dup)" : "");
-  }
-
-  void operator()() const
-  {
-    std::vector<DNSRecord> vec;
-    vec.reserve(d_howmany);
+    d_vec.reserve(d_howmany);
     std::string name("some.name.in.some.domain");
     auto count = d_howmany;
     if (d_withdup) {
@@ -1207,17 +1196,28 @@ struct DedupRecordsTest
       auto content = DNSRecordContent::make(QType::TXT, QClass::IN, "\"a text " + std::to_string(i) + "\"");
       DNSRecord rec(name, content, QType::TXT);
       if (i == 0 && d_withdup) {
-        vec.emplace_back(rec);
+        d_vec.emplace_back(rec);
       }
-      vec.emplace_back(std::move(rec));
+      d_vec.emplace_back(std::move(rec));
     }
+  }
+
+  [[nodiscard]] string getName() const
+  {
+    return std::to_string(d_howmany) + " DedupRecords" + std::string(d_dedup ? "" : " (setup only)") +
+      std::string(d_withdup ? " (with dup)" : "");
+  }
 
+  void operator()() const
+  {
+    auto vec{d_vec};
     if (d_dedup) {
       pdns::dedupRecords(vec);
     }
   }
 
 private:
+  vector<DNSRecord> d_vec;
   size_t d_howmany;
   bool d_dedup;
   bool d_withdup;