rec: Speed up the packet cache

* Don't parse the response's qname for every call to
`getResponsePacket()`, this leads to a ~15% speed up on pure retrieval
* Only hash once, keep the hash result around, leading to a ~40%
speed up on insertion

diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc
index 1776ccf15e327ecbb0da2e825be5f7509d48daae..048f4b5ff1659fab9694f943ebdd2d572df96391 100644 (file)
--- a/pdns/pdns_recursor.cc
+++ b/pdns/pdns_recursor.cc
@@ -205,6 +205,7 @@ struct DNSComboWriter {
   bool d_tcp;
   int d_socket;
   int d_tag{0};
+  uint32_t d_qhash{0};
   string d_query;
   shared_ptr<TCPConnection> d_tcpConnection;
   vector<pair<uint16_t, string> > d_ednsOpts;
@@ -1162,7 +1163,7 @@ void startDoResolve(void *p)
       if(sendmsg(dc->d_socket, &msgh, 0) < 0 && g_logCommonErrors) 
         L<<Logger::Warning<<"Sending UDP reply to client "<<dc->d_remote.toStringWithPort()<<" failed with: "<<strerror(errno)<<endl;
       if(!SyncRes::s_nopacketcache && !variableAnswer && !sr.wasVariable() ) {
-        t_packetCache->insertResponsePacket(dc->d_tag, dc->d_mdp.d_qname, dc->d_mdp.d_qtype, dc->d_query,
+        t_packetCache->insertResponsePacket(dc->d_tag, dc->d_qhash, dc->d_mdp.d_qname, dc->d_mdp.d_qtype, dc->d_mdp.d_qclass,
                                             string((const char*)&*packet.begin(), packet.size()),
                                             g_now.tv_sec,
                                             pw.getHeader()->rcode == RCode::ServFail ? SyncRes::s_packetcacheservfailttl :
@@ -1540,6 +1541,7 @@ string* doProcessUDPQuestion(const std::string& question, const ComboAddress& fr
   string response;
   const struct dnsheader* dh = (struct dnsheader*)question.c_str();
   unsigned int ctag=0;
+  uint32_t qhash;
   bool needECS = false;
   std::vector<std::string> policyTags;
   std::unordered_map<string,string> data;
@@ -1605,7 +1607,7 @@ string* doProcessUDPQuestion(const std::string& question, const ComboAddress& fr
     }
 #endif /* HAVE_PROTOBUF */
 
-    cacheHit = (!SyncRes::s_nopacketcache && t_packetCache->getResponsePacket(ctag, question, g_now.tv_sec, &response, &age, &pbMessage));
+    cacheHit = (!SyncRes::s_nopacketcache && t_packetCache->getResponsePacket(ctag, question, g_now.tv_sec, &response, &age, &qhash, &pbMessage));
     if (cacheHit) {
 #ifdef HAVE_PROTOBUF
       if(luaconfsLocal->protobufServer && (!luaconfsLocal->protobufTaggedOnly || !pbMessage.getAppliedPolicy().empty() || !pbMessage.getPolicyTags().empty())) {
@@ -1669,6 +1671,7 @@ string* doProcessUDPQuestion(const std::string& question, const ComboAddress& fr
   DNSComboWriter* dc = new DNSComboWriter(question.c_str(), question.size(), g_now);
   dc->setSocket(fd);
   dc->d_tag=ctag;
+  dc->d_qhash=qhash;
   dc->d_query = question;
   dc->setRemote(&fromaddr);
   dc->setLocal(destaddr);

diff --git a/pdns/recpacketcache.cc b/pdns/recpacketcache.cc
index 1d24831795c796248be2d8294e93789c00a94cc3..dd71567a45180e2c9967bea7de487420bfbeb014 100644 (file)
--- a/pdns/recpacketcache.cc
+++ b/pdns/recpacketcache.cc
@@ -31,7 +31,7 @@ int RecursorPacketCache::doWipePacketCache(const DNSName& name, uint16_t qtype,
       if(iter->d_name != name)
        break;
     }
-    
+
     if(qtype==0xffff || iter->d_type == qtype) {
       iter=idx.erase(iter);
       count++;
@@ -42,14 +42,12 @@ int RecursorPacketCache::doWipePacketCache(const DNSName& name, uint16_t qtype,
   return count;
 }
 
-// one day this function could be really fast by doing only a case insensitive compare
-static bool qrMatch(const std::string& query, const std::string& response)
+static bool qrMatch(const std::string& query, const DNSName& rname, uint16_t rtype, uint16_t rclass)
 {
-  uint16_t rqtype, rqclass, qqtype, qqclass;
-  DNSName queryname(query.c_str(), query.length(), sizeof(dnsheader), false, &qqtype, &qqclass, 0);
-  DNSName respname(response.c_str(), response.length(), sizeof(dnsheader), false, &rqtype, &rqclass, 0);
+  uint16_t qtype, qclass;
+  DNSName qname(query.c_str(), query.length(), sizeof(dnsheader), false, &qtype, &qclass, 0);
   // this ignores checking on the EDNS subnet flags! 
-  return queryname==respname && rqtype == qqtype && rqclass == qqclass;
+  return qname==rname && rtype == qtype && rclass == qclass;
 }
 
 uint32_t RecursorPacketCache::canHashPacket(const std::string& origPacket)
@@ -96,17 +94,17 @@ uint32_t RecursorPacketCache::canHashPacket(const std::string& origPacket)
 }
 
 bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now,
-                                            std::string* responsePacket, uint32_t* age)
+                                            std::string* responsePacket, uint32_t* age, uint32_t* qhash)
 {
-  return getResponsePacket(tag, queryPacket, now, responsePacket, age, nullptr);
+  return getResponsePacket(tag, queryPacket, now, responsePacket, age, qhash, nullptr);
 }
 
 bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now,
-                                            std::string* responsePacket, uint32_t* age, RecProtoBufMessage* protobufMessage)
+                                            std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage)
 {
-  uint32_t h = canHashPacket(queryPacket);
+  *qhash = canHashPacket(queryPacket);
   auto& idx = d_packetCache.get<HashTag>();
-  auto range = idx.equal_range(tie(tag,h)); 
+  auto range = idx.equal_range(tie(tag,*qhash));
 
   if(range.first == range.second) {
     d_misses++;
@@ -115,7 +113,7 @@ bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string&
     
   for(auto iter = range.first ; iter != range.second ; ++ iter) {
     // the possibility is VERY real that we get hits that are not right - birthday paradox
-    if(!qrMatch(queryPacket, iter->d_packet))
+    if(!qrMatch(queryPacket, iter->d_name, iter->d_type, iter->d_class))
       continue;
     if((uint32_t)now < iter->d_ttd) { // it is right, it is fresh!
       *age = now - iter->d_creation;
@@ -153,20 +151,19 @@ bool RecursorPacketCache::getResponsePacket(unsigned int tag, const std::string&
 }
 
 
-void RecursorPacketCache::insertResponsePacket(unsigned int tag, const DNSName& qname, uint16_t qtype, const std::string& queryPacket, const std::string& responsePacket, time_t now, uint32_t ttl)
+void RecursorPacketCache::insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttl)
 {
-  insertResponsePacket(tag, qname, qtype, queryPacket, responsePacket, now, ttl, nullptr);
+  insertResponsePacket(tag, qhash, qname, qtype, qclass, responsePacket, now, ttl, nullptr);
 }
 
-void RecursorPacketCache::insertResponsePacket(unsigned int tag, const DNSName& qname, uint16_t qtype, const std::string& queryPacket, const std::string& responsePacket, time_t now, uint32_t ttl, const RecProtoBufMessage* protobufMessage)
+void RecursorPacketCache::insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttl, const RecProtoBufMessage* protobufMessage)
 {
-  auto qhash = canHashPacket(queryPacket);
   auto& idx = d_packetCache.get<HashTag>();
   auto range = idx.equal_range(tie(tag,qhash));
   auto iter = range.first;
 
   for( ; iter != range.second ; ++iter)  {
-    if(iter->d_type != qtype)
+    if(iter->d_type != qtype || iter->d_class != qclass)
       continue;
     // this only happens on insert which is relatively rare and does not need to be super fast
     DNSName respname(iter->d_packet.c_str(), iter->d_packet.length(), sizeof(dnsheader), false, 0, 0, 0);
@@ -191,6 +188,7 @@ void RecursorPacketCache::insertResponsePacket(unsigned int tag, const DNSName&
     e.d_name = qname;
     e.d_qhash = qhash;
     e.d_type = qtype;
+    e.d_class = qclass;
     e.d_ttd = now+ttl;
     e.d_creation = now;
     e.d_tag = tag;

diff --git a/pdns/recpacketcache.hh b/pdns/recpacketcache.hh
index 65a6baad9e1cd1ddd7aeeff6161d87944898b365..8c198feb6dc8794fbc2844f5fc72c0d1b815b73a 100644 (file)
--- a/pdns/recpacketcache.hh
+++ b/pdns/recpacketcache.hh
@@ -50,10 +50,10 @@ class RecursorPacketCache
 {
 public:
   RecursorPacketCache();
-  bool getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, std::string* responsePacket, uint32_t* age);
-  void insertResponsePacket(unsigned int tag, const DNSName& qname, uint16_t qtype, const std::string& queryPacket, const std::string& responsePacket, time_t now, uint32_t ttd);
-  bool getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, std::string* responsePacket, uint32_t* age, RecProtoBufMessage* protobufMessage);
-  void insertResponsePacket(unsigned int tag, const DNSName& qname, uint16_t qtype, const std::string& queryPacket, const std::string& responsePacket, time_t now, uint32_t ttd, const RecProtoBufMessage* protobufMessage);
+  bool getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, std::string* responsePacket, uint32_t* age, uint32_t* qhash);
+  bool getResponsePacket(unsigned int tag, const std::string& queryPacket, time_t now, std::string* responsePacket, uint32_t* age, uint32_t* qhash, RecProtoBufMessage* protobufMessage);
+  void insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttd);
+  void insertResponsePacket(unsigned int tag, uint32_t qhash, const DNSName& qname, uint16_t qtype, uint16_t qclass, const std::string& responsePacket, time_t now, uint32_t ttd, const RecProtoBufMessage* protobufMessage);
   void doPruneTo(unsigned int maxSize=250000);
   uint64_t doDump(int fd);
   int doWipePacketCache(const DNSName& name, uint16_t qtype=0xffff, bool subtree=false);
@@ -72,6 +72,7 @@ private:
     mutable uint32_t d_creation; // so we can 'age' our packets
     DNSName d_name;
     uint16_t d_type;
+    uint16_t d_class;
     mutable std::string d_packet; // "I know what I am doing"
 #ifdef HAVE_PROTOBUF
     mutable RecProtoBufMessage d_protobufMessage;

diff --git a/pdns/test-recpacketcache_cc.cc b/pdns/test-recpacketcache_cc.cc
index 8d723253b0f8bebd688e16bf0313705b906cc922..bdd9addc410e8fcfc7089856c1d79396f2cd87d3 100644 (file)
--- a/pdns/test-recpacketcache_cc.cc
+++ b/pdns/test-recpacketcache_cc.cc
@@ -17,6 +17,11 @@ BOOST_AUTO_TEST_SUITE(recpacketcache_cc)
 
 BOOST_AUTO_TEST_CASE(test_recPacketCacheSimple) {
   RecursorPacketCache rpc;
+  string fpacket;
+  int tag=0;
+  uint32_t age=0;
+  uint32_t qhash=0;
+  uint32_t ttd=3600;
   BOOST_CHECK_EQUAL(rpc.size(), 0);
 
   DNSName qname("www.powerdns.com");
@@ -26,27 +31,29 @@ BOOST_AUTO_TEST_CASE(test_recPacketCacheSimple) {
   pw.getHeader()->qr=false;
   pw.getHeader()->id=random();
   string qpacket((const char*)&packet[0], packet.size());
-  pw.startRecord(qname, QType::A, 3600);
+  pw.startRecord(qname, QType::A, ttd);
+
+  BOOST_CHECK_EQUAL(rpc.getResponsePacket(tag, qpacket, time(nullptr), &fpacket, &age, &qhash), false);
 
   ARecordContent ar("127.0.0.1");
   ar.toPacket(pw);
   pw.commit();
   string rpacket((const char*)&packet[0], packet.size());
 
-  rpc.insertResponsePacket(0,qname, QType::A, qpacket, rpacket, time(0), 3600);
+  rpc.insertResponsePacket(tag, qhash, qname, QType::A, QClass::IN, rpacket, time(0), ttd);
   BOOST_CHECK_EQUAL(rpc.size(), 1);
   rpc.doPruneTo(0);
   BOOST_CHECK_EQUAL(rpc.size(), 0);
-  rpc.insertResponsePacket(0,qname, QType::A, qpacket, rpacket, time(0), 3600);
+  rpc.insertResponsePacket(tag, qhash, qname, QType::A, QClass::IN, rpacket, time(0), ttd);
   BOOST_CHECK_EQUAL(rpc.size(), 1);
   rpc.doWipePacketCache(qname);
   BOOST_CHECK_EQUAL(rpc.size(), 0);
 
-  rpc.insertResponsePacket(0,qname, QType::A, qpacket, rpacket, time(0), 3600);
-  uint32_t age=0;
-  string fpacket;
-  bool found = rpc.getResponsePacket(0, qpacket, time(0), &fpacket, &age);
-  BOOST_CHECK_EQUAL(found, 1);
+  rpc.insertResponsePacket(tag, qhash, qname, QType::A, QClass::IN, rpacket, time(0), ttd);
+  uint32_t qhash2 = 0;
+  bool found = rpc.getResponsePacket(tag, qpacket, time(nullptr), &fpacket, &age, &qhash2);
+  BOOST_CHECK_EQUAL(found, true);
+  BOOST_CHECK_EQUAL(qhash, qhash2);
   BOOST_CHECK_EQUAL(fpacket, rpacket);
 
   packet.clear();
@@ -57,14 +64,12 @@ BOOST_AUTO_TEST_CASE(test_recPacketCacheSimple) {
   pw2.getHeader()->qr=false;
   pw2.getHeader()->id=random();
   qpacket.assign((const char*)&packet[0], packet.size());
-  found = rpc.getResponsePacket(0, qpacket, time(0), &fpacket, &age);
-  BOOST_CHECK_EQUAL(found, 0);
+
+  found = rpc.getResponsePacket(tag, qpacket, time(nullptr), &fpacket, &age, &qhash);
+  BOOST_CHECK_EQUAL(found, false);
 
   rpc.doWipePacketCache(DNSName("com"), 0xffff, true);
   BOOST_CHECK_EQUAL(rpc.size(), 0);
-
-
-
 } 
 
 BOOST_AUTO_TEST_SUITE_END()