[tor-commits] [tor/master] Memoize summarize_protover_flags()

[nickm at torproject.org]

commit 6d93820499a8bfb19128759893b18c1437f99c6b
Author: Nick Mathewson <nickm at torproject.org>
Date:   Fri Oct 19 15:04:45 2018 -0400

    Memoize summarize_protover_flags()
    
    Our tests showed that this function is responsible for a huge number
    of our malloc/free() calls.  It's a prime candidate for being
    memoized.
    
    Closes ticket 27225.
---
 changes/ticket27225    |  5 +++
 src/app/main/main.c    |  2 ++
 src/core/or/versions.c | 82 +++++++++++++++++++++++++++++++++++++++++---------
 src/core/or/versions.h |  2 ++
 4 files changed, 76 insertions(+), 15 deletions(-)

diff --git a/changes/ticket27225 b/changes/ticket27225
new file mode 100644
index 000000000..4c05a269d
--- /dev/null
+++ b/changes/ticket27225
@@ -0,0 +1,5 @@
+  o Minor features (performance):
+    - Avoid parsing the same protocol-versions string over and over
+      in summarize_protover_flags(). This should save us a huge number
+      of malloc calls on startup, and may reduce memory fragmentation with
+      some allocators. Closes ticket 27225.
diff --git a/src/app/main/main.c b/src/app/main/main.c
index ae87add67..04bbfadcb 100644
--- a/src/app/main/main.c
+++ b/src/app/main/main.c
@@ -33,6 +33,7 @@
 #include "core/or/relay.h"
 #include "core/or/scheduler.h"
 #include "core/or/status.h"
+#include "core/or/versions.h"
 #include "feature/api/tor_api.h"
 #include "feature/api/tor_api_internal.h"
 #include "feature/client/addressmap.h"
@@ -791,6 +792,7 @@ tor_free_all(int postfork)
   dos_free_all();
   circuitmux_ewma_free_all();
   accounting_free_all();
+  protover_summary_cache_free_all();
 
   if (!postfork) {
     config_free_all();
diff --git a/src/core/or/versions.c b/src/core/or/versions.c
index 06274996a..6f8eea7a6 100644
--- a/src/core/or/versions.c
+++ b/src/core/or/versions.c
@@ -377,6 +377,62 @@ sort_version_list(smartlist_t *versions, int remove_duplicates)
     smartlist_uniq(versions, compare_tor_version_str_ptr_, tor_free_);
 }
 
+/** If there are more than this many entries, we're probably under
+ * some kind of weird DoS. */
+static const int MAX_PROTOVER_SUMMARY_MAP_LEN = 1024;
+
+/**
+ * Map from protover string to protover_summary_flags_t.
+ */
+static strmap_t *protover_summary_map = NULL;
+
+/**
+ * Helper.  Given a non-NULL protover string <b>protocols</b>, set <b>out</b>
+ * to its summary, and memoize the result in <b>protover_summary_map</b>.
+ */
+static void
+memoize_protover_summary(protover_summary_flags_t *out,
+                         const char *protocols)
+{
+  if (!protover_summary_map)
+    protover_summary_map = strmap_new();
+
+  if (strmap_size(protover_summary_map) >= MAX_PROTOVER_SUMMARY_MAP_LEN) {
+    protover_summary_cache_free_all();
+  }
+
+  const protover_summary_flags_t *cached =
+    strmap_get(protover_summary_map, protocols);
+
+  if (cached != NULL) {
+    /* We found a cached entry; no need to parse this one. */
+    memcpy(out, cached, sizeof(protover_summary_flags_t));
+    tor_assert(out->protocols_known);
+    return;
+  }
+
+  memset(out, 0, sizeof(*out));
+  out->protocols_known = 1;
+  out->supports_extend2_cells =
+    protocol_list_supports_protocol(protocols, PRT_RELAY, 2);
+  out->supports_ed25519_link_handshake_compat =
+    protocol_list_supports_protocol(protocols, PRT_LINKAUTH, 3);
+  out->supports_ed25519_link_handshake_any =
+    protocol_list_supports_protocol_or_later(protocols, PRT_LINKAUTH, 3);
+  out->supports_ed25519_hs_intro =
+    protocol_list_supports_protocol(protocols, PRT_HSINTRO, 4);
+  out->supports_v3_hsdir =
+    protocol_list_supports_protocol(protocols, PRT_HSDIR,
+                                    PROTOVER_HSDIR_V3);
+  out->supports_v3_rendezvous_point =
+    protocol_list_supports_protocol(protocols, PRT_HSREND,
+                                    PROTOVER_HS_RENDEZVOUS_POINT_V3);
+
+  protover_summary_flags_t *new_cached = tor_memdup(out, sizeof(*out));
+  cached = strmap_set(protover_summary_map, protocols, new_cached);
+  tor_assert(!cached);
+}
+
 /** Summarize the protocols listed in <b>protocols</b> into <b>out</b>,
  * falling back or correcting them based on <b>version</b> as appropriate.
  */
@@ -388,21 +444,7 @@ summarize_protover_flags(protover_summary_flags_t *out,
   tor_assert(out);
   memset(out, 0, sizeof(*out));
   if (protocols) {
-    out->protocols_known = 1;
-    out->supports_extend2_cells =
-      protocol_list_supports_protocol(protocols, PRT_RELAY, 2);
-    out->supports_ed25519_link_handshake_compat =
-      protocol_list_supports_protocol(protocols, PRT_LINKAUTH, 3);
-    out->supports_ed25519_link_handshake_any =
-      protocol_list_supports_protocol_or_later(protocols, PRT_LINKAUTH, 3);
-    out->supports_ed25519_hs_intro =
-      protocol_list_supports_protocol(protocols, PRT_HSINTRO, 4);
-    out->supports_v3_hsdir =
-      protocol_list_supports_protocol(protocols, PRT_HSDIR,
-                                      PROTOVER_HSDIR_V3);
-    out->supports_v3_rendezvous_point =
-      protocol_list_supports_protocol(protocols, PRT_HSREND,
-                                      PROTOVER_HS_RENDEZVOUS_POINT_V3);
+    memoize_protover_summary(out, protocols);
   }
   if (version && !strcmpstart(version, "Tor ")) {
     if (!out->protocols_known) {
@@ -420,3 +462,13 @@ summarize_protover_flags(protover_summary_flags_t *out,
     }
   }
 }
+
+/**
+ * Free all space held in the protover_summary_map.
+ */
+void
+protover_summary_cache_free_all(void)
+{
+  strmap_free(protover_summary_map, tor_free_);
+  protover_summary_map = NULL;
+}
diff --git a/src/core/or/versions.h b/src/core/or/versions.h
index 0c773f3f4..4fc50a001 100644
--- a/src/core/or/versions.h
+++ b/src/core/or/versions.h
@@ -41,4 +41,6 @@ void summarize_protover_flags(protover_summary_flags_t *out,
                               const char *protocols,
                               const char *version);
 
+void protover_summary_cache_free_all(void);
+
 #endif /* !defined(TOR_VERSIONS_H) */