Add Answer hash to prepare switch std::set to std::unordered_set or folly::FastSet

Yu-Ho Hsieh · meta-codesync[bot] · commit 25fbabb71854 · 2026-01-26T11:16:47.000-08:00
Summary: context is here https://fburl.com/workplace/3r5iucen inorder to transit from std::set to hash-based, we need the hasher. I separate the commit so its easier to rollback. Reviewed By: hanidamlaj Differential Revision: D91178557 fbshipit-source-id: dd1e182d884400c43048526ae7346f1694ee886d
diff --git a/proxygen/lib/dns/CachingDNSResolver.cpp b/proxygen/lib/dns/CachingDNSResolver.cpp
@@ -29,7 +29,7 @@ void CachingDNSResolver::resolveHostname(DNSResolver::ResolutionCallback* cb,
 
   if ((c_iter = cache_.find(name)) != cache_.end()) {
     CacheEntry& entry = c_iter->second;
-    std::set<Answer>& res = entry.answers_;
+    auto& res = entry.answers_;
     bool needQuery = false;       // true if no or only expired answer exists
     bool hasCachedAnswer = false; // with respect to TTR
     bool isPartialMiss = false;
@@ -103,7 +103,7 @@ void CachingDNSResolver::insertCache(
   auto iter = cache.find(name);
   if (iter != cache.end()) {
     CacheEntry& entry = iter->second;
-    std::set<Answer>& res = entry.answers_;
+    auto& res = entry.answers_;
     for (auto& a : res) {
       if (entry.baseTime_ + a.ttl >= newEntry.baseTime_) {
         Answer ans(a);
@@ -150,7 +150,7 @@ void CachingDNSResolver::searchCache(std::string name,
   auto iter = cache.find(name);
   if (iter != cache.end()) {
     CacheEntry& entry = iter->second;
-    std::set<Answer>& res = entry.answers_;
+    auto& res = entry.answers_;
     for (auto& answer : res) {
       if (answer.type != Answer::AnswerType::AT_ADDRESS) {
         continue;
diff --git a/proxygen/lib/dns/CachingDNSResolver.h b/proxygen/lib/dns/CachingDNSResolver.h
@@ -13,6 +13,7 @@
 #include <vector>
 
 #include <folly/container/EvictingCacheMap.h>
+#include <folly/container/F14Set.h>
 #include <proxygen/lib/utils/Time.h>
 
 #include "proxygen/lib/dns/DNSResolver.h"
@@ -93,7 +94,7 @@ class CachingDNSResolver : public DNSResolver {
     // The answers are stored in a set to aid in removing duplicates when we
     // merge in answers from the old set. We explicitly shuffle the results
     // when we return them from the cache.
-    std::set<Answer> answers_;
+    folly::F14FastSet<Answer, AnswerHash> answers_;
     TimePoint baseTime_;
   };
 
diff --git a/proxygen/lib/dns/DNSResolver.cpp b/proxygen/lib/dns/DNSResolver.cpp
@@ -19,6 +19,17 @@ constexpr std::string_view kNoAddr{"No valid address or name found"};
 };
 
 namespace proxygen {
+bool DNSResolver::Answer::operator==(const Answer& rhs) const noexcept {
+  return type == rhs.type && address == rhs.address && name == rhs.name &&
+         canonicalName == rhs.canonicalName && port == rhs.port &&
+         priority == rhs.priority;
+}
+
+size_t DNSResolver::AnswerHash::operator()(const Answer& a) const noexcept {
+  // Hash by discriminant + relevant payload used in operator<
+  return folly::hash::hash_combine(
+      a.type, a.address.hash(), a.priority, a.name, a.canonicalName, a.port);
+}
 
 // public static
 folly::exception_wrapper DNSResolver::makeNoNameException() noexcept {
diff --git a/proxygen/lib/dns/DNSResolver.h b/proxygen/lib/dns/DNSResolver.h
@@ -191,35 +191,15 @@ class DNSResolver : public folly::DelayedDestruction {
     //default ctor
     Answer() = default;
 
-    // for unit tests
-    bool operator==(const Answer& rhs) const {
-      return !(operator<(rhs)) && !(rhs < (*this));
-    }
+    bool operator==(const Answer& rhs) const noexcept;
 
-    bool operator<(const Answer& rhs) const {
-      if (type != rhs.type) {
-        return type < rhs.type;
-      }
+  };
+
+  struct AnswerHash {
+  size_t operator()(const Answer& a) const noexcept;
+};
 
-      if (type == AT_ADDRESS) {
-        if (address != rhs.address) {
-          return address < rhs.address;
-        }
-      } else if (type == AT_MX) {
-        if (priority != rhs.priority) {
-          return priority < rhs.priority;
-        } else if (name != rhs.name) {
-          return name < rhs.name;
-        }
-      } else {  // AT_NAME or AT_CNAME
-        if (name != rhs.name) {
-          return name < rhs.name;
-        }
-      }
 
-      return false;
-    }
-  };
 
   /**
    * Callback interface for resolution requests.
diff --git a/proxygen/lib/dns/test/DNSResolverTest.cpp b/proxygen/lib/dns/test/DNSResolverTest.cpp
@@ -0,0 +1,222 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ * All rights reserved.
+ *
+ * This source code is licensed under the BSD-style license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <folly/SocketAddress.h>
+#include <gtest/gtest.h>
+#include <proxygen/lib/dns/DNSResolver.h>
+
+#include <unordered_set>
+
+using folly::SocketAddress;
+using proxygen::DNSResolver;
+using std::chrono::seconds;
+
+namespace {
+
+size_t h(const DNSResolver::Answer& a) {
+  return DNSResolver::AnswerHash{}(a);
+}
+
+} // namespace
+
+TEST(DNSResolverAnswerHashTest, EqualAnswersHaveEqualHash_Address) {
+  // Same IP/port should be equal and have equal hash regardless of TTL
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 80));
+  DNSResolver::Answer a2(seconds(30), SocketAddress("127.0.0.1", 80));
+
+  EXPECT_TRUE(a1 == a2);
+  EXPECT_EQ(h(a1), h(a2));
+}
+
+TEST(DNSResolverAnswerHashTest, EqualAnswersHaveEqualHash_Name) {
+  DNSResolver::Answer n1(
+      seconds(10), std::string("example.com"), DNSResolver::Answer::AT_NAME);
+  DNSResolver::Answer n2(
+      seconds(1), std::string("example.com"), DNSResolver::Answer::AT_NAME);
+
+  EXPECT_TRUE(n1 == n2);
+  EXPECT_EQ(h(n1), h(n2));
+}
+
+TEST(DNSResolverAnswerHashTest, EqualAnswersHaveEqualHash_CName) {
+  DNSResolver::Answer c1(seconds(10),
+                         std::string("cname.example.com"),
+                         DNSResolver::Answer::AT_CNAME);
+  DNSResolver::Answer c2(seconds(10),
+                         std::string("cname.example.com"),
+                         DNSResolver::Answer::AT_CNAME);
+  EXPECT_TRUE(c1 == c2);
+  EXPECT_EQ(h(c1), h(c2));
+}
+
+TEST(DNSResolverAnswerHashTest, EqualAnswersHaveEqualHash_MX) {
+  DNSResolver::Answer m1(
+      seconds(60), /*priority*/ 10, std::string("mail.example.com"));
+  DNSResolver::Answer m2(
+      seconds(5), /*priority*/ 10, std::string("mail.example.com"));
+  EXPECT_TRUE(m1 == m2);
+  EXPECT_EQ(h(m1), h(m2));
+}
+
+TEST(DNSResolverAnswerHashTest, DistinctAnswersBehaveInUnorderedSet) {
+  // Build a set with distinct answers of different types and payloads
+  std::unordered_set<DNSResolver::Answer, DNSResolver::AnswerHash> set;
+
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 0));
+  DNSResolver::Answer a2(seconds(5), SocketAddress("::1", 0));
+  DNSResolver::Answer n1(
+      seconds(5), std::string("example.com"), DNSResolver::Answer::AT_NAME);
+  DNSResolver::Answer n2(
+      seconds(5), std::string("www.example.com"), DNSResolver::Answer::AT_NAME);
+  DNSResolver::Answer c1(seconds(5),
+                         std::string("cname.example.com"),
+                         DNSResolver::Answer::AT_CNAME);
+  DNSResolver::Answer m1(
+      seconds(5), 5 /*priority*/, std::string("mail.example.com"));
+  DNSResolver::Answer m2(
+      seconds(5), 10 /*priority*/, std::string("mail.example.com"));
+
+  EXPECT_TRUE(set.insert(a1).second);
+  EXPECT_TRUE(set.insert(a2).second);
+  EXPECT_TRUE(set.insert(n1).second);
+  EXPECT_TRUE(set.insert(n2).second);
+  EXPECT_TRUE(set.insert(c1).second);
+  EXPECT_TRUE(set.insert(m1).second);
+  EXPECT_TRUE(set.insert(m2).second);
+
+  // Reinserting equal values should not increase size
+  EXPECT_FALSE(set.insert(DNSResolver::Answer(seconds(999),
+                                              SocketAddress("127.0.0.1", 0)))
+                   .second);
+  EXPECT_FALSE(set.insert(DNSResolver::Answer(seconds(1),
+                                              std::string("example.com"),
+                                              DNSResolver::Answer::AT_NAME))
+                   .second);
+  EXPECT_FALSE(set.insert(DNSResolver::Answer(
+                              seconds(1), 5, std::string("mail.example.com")))
+                   .second);
+
+  EXPECT_EQ(set.size(), 7);
+}
+
+TEST(DNSResolverAnswerHashTest, IgnoresTTLAndCreationTime) {
+  // creationTime is set at construction time; we can't set it directly,
+  // but we can construct the two objects at different times and ensure they
+  // still compare equal and have equal hashes.
+  DNSResolver::Answer a1(seconds(1), SocketAddress("1.2.3.4", 0));
+  /* Small sleep to ensure creationTime differs if measured in seconds */
+  DNSResolver::Answer a2(seconds(999), SocketAddress("1.2.3.4", 0));
+
+  EXPECT_TRUE(a1 == a2);
+  EXPECT_EQ(h(a1), h(a2));
+}
+
+// Tests to ensure each field in operator== and AnswerHash is properly checked.
+// If any field is missing from the implementation, these tests will fail.
+
+TEST(DNSResolverAnswerHashTest, DifferentTypeNotEqual) {
+  DNSResolver::Answer a1(
+      seconds(5), std::string("example.com"), DNSResolver::Answer::AT_NAME);
+  DNSResolver::Answer a2(
+      seconds(5), std::string("example.com"), DNSResolver::Answer::AT_CNAME);
+
+  EXPECT_FALSE(a1 == a2);
+  EXPECT_NE(h(a1), h(a2));
+}
+
+TEST(DNSResolverAnswerHashTest, DifferentAddressNotEqual) {
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 80));
+  DNSResolver::Answer a2(seconds(5), SocketAddress("127.0.0.2", 80));
+
+  EXPECT_FALSE(a1 == a2);
+  EXPECT_NE(h(a1), h(a2));
+}
+
+TEST(DNSResolverAnswerHashTest, DifferentNameNotEqual) {
+  DNSResolver::Answer n1(
+      seconds(5), std::string("example.com"), DNSResolver::Answer::AT_NAME);
+  DNSResolver::Answer n2(
+      seconds(5), std::string("www.example.com"), DNSResolver::Answer::AT_NAME);
+
+  EXPECT_FALSE(n1 == n2);
+  EXPECT_NE(h(n1), h(n2));
+}
+
+TEST(DNSResolverAnswerHashTest, DifferentCanonicalNameNotEqual) {
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 80));
+  a1.canonicalName = "canonical1.example.com";
+
+  DNSResolver::Answer a2(seconds(5), SocketAddress("127.0.0.1", 80));
+  a2.canonicalName = "canonical2.example.com";
+
+  EXPECT_FALSE(a1 == a2);
+  EXPECT_NE(h(a1), h(a2));
+}
+
+TEST(DNSResolverAnswerHashTest, DifferentPortNotEqual) {
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 0));
+  a1.port = 8080;
+
+  DNSResolver::Answer a2(seconds(5), SocketAddress("127.0.0.1", 0));
+  a2.port = 9090;
+
+  EXPECT_FALSE(a1 == a2);
+  EXPECT_NE(h(a1), h(a2));
+}
+
+TEST(DNSResolverAnswerHashTest, DifferentPriorityNotEqual) {
+  DNSResolver::Answer m1(
+      seconds(5), /*priority*/ 5, std::string("mail.example.com"));
+  DNSResolver::Answer m2(
+      seconds(5), /*priority*/ 10, std::string("mail.example.com"));
+
+  EXPECT_FALSE(m1 == m2);
+  EXPECT_NE(h(m1), h(m2));
+}
+
+TEST(DNSResolverAnswerHashTest, SameCanonicalNameAndPortEqual) {
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 80));
+  a1.canonicalName = "canonical.example.com";
+  a1.port = 8080;
+
+  DNSResolver::Answer a2(seconds(30), SocketAddress("127.0.0.1", 80));
+  a2.canonicalName = "canonical.example.com";
+  a2.port = 8080;
+
+  EXPECT_TRUE(a1 == a2);
+  EXPECT_EQ(h(a1), h(a2));
+}
+
+TEST(DNSResolverAnswerHashTest, CanonicalNameAndPortInUnorderedSet) {
+  std::unordered_set<DNSResolver::Answer, DNSResolver::AnswerHash> set;
+
+  DNSResolver::Answer a1(seconds(5), SocketAddress("127.0.0.1", 80));
+  a1.canonicalName = "canonical1.example.com";
+  a1.port = 8080;
+
+  DNSResolver::Answer a2(seconds(5), SocketAddress("127.0.0.1", 80));
+  a2.canonicalName = "canonical2.example.com";
+  a2.port = 8080;
+
+  DNSResolver::Answer a3(seconds(5), SocketAddress("127.0.0.1", 80));
+  a3.canonicalName = "canonical1.example.com";
+  a3.port = 9090;
+
+  EXPECT_TRUE(set.insert(a1).second);
+  EXPECT_TRUE(set.insert(a2).second);
+  EXPECT_TRUE(set.insert(a3).second);
+  EXPECT_EQ(set.size(), 3);
+
+  // Same canonicalName and port as a1 should not be inserted
+  DNSResolver::Answer a4(seconds(999), SocketAddress("127.0.0.1", 80));
+  a4.canonicalName = "canonical1.example.com";
+  a4.port = 8080;
+
+  EXPECT_FALSE(set.insert(a4).second);
+  EXPECT_EQ(set.size(), 3);
+}
