r12349@catbus: nickm | 2007-04-11 09:18:15 -0400

 Add code to shrink the cell memory pool by discarding empty chunks that have been empty for the last 60 seconds.  Also, instead of having test.c duplicate declarations for exposed functions, put them inside #ifdef foo_PRIVATE blocks in the headers.  This prevents bugs where test.c gets out of sync.


svn:r9944

doc/TODO

@@ -79,7 +79,7 @@ Things we'd like to do in 0.2.0.x:
         o Implement pool-allocation
         o Have Tor use it for packed cells.
         o Document it.
-        - Do something smart with freeing unused chunks.
+        o Do something smart with freeing unused chunks.
         - Benchmark pool-allocation vs straightforward malloc.
       - Can we stop doing so many memcpys on cells?
       o Also, only package data from exitconns when there is space on the
@@ -271,7 +271,7 @@ Minor items for 0.1.2.x as time permits:
 R - add d64 and fp64 along-side d and fp so people can paste status
     entries into a url. since + is a valid base64 char, only allow one
     at a time. spec and then do.
-  - When we export something from foo.c file for testing purposes only,
+  o When we export something from foo.c file for testing purposes only,
     make a foo_test.h file for test.c to include... or put them behind an
     #ifdef FOO_PRIVATE.
   - The Debian package now uses --verify-config when (re)starting,

src/common/mempool.c

@@ -210,6 +210,8 @@ mp_pool_get(mp_pool_t *pool)
 
     ASSERT(!chunk->prev);
     --pool->n_empty_chunks;
+    if (pool->n_empty_chunks < pool->min_empty_chunks)
+      pool->min_empty_chunks = pool->n_empty_chunks;
   } else {
     /* We have no used or empty chunks: allocate a new chunk. */
     chunk = mp_chunk_new(pool);
@@ -375,11 +377,20 @@ mp_pool_new(size_t item_size, size_t chunk_capacity)
 }
 
 /** If there are more than <b>n</b> empty chunks in <b>pool</b>, free the
- * exces ones that have been empty for the longest. */
+ * exces ones that have been empty for the longest.   (If <b>n</b> is less
+ * than zero, free only empty chunks that were not used since the last
+ * call to mp_pool_clean(), leaving only -<b>n</b>.) */
 void
 mp_pool_clean(mp_pool_t *pool, int n)
 {
   mp_chunk_t *chunk, **first_to_free;
+  if (n < 0) {
+    n = pool->min_empty_chunks + (-n);
+    if (n < pool->n_empty_chunks)
+      pool->min_empty_chunks = n;
+  }
+  ASSERT(n>=0);
+
   first_to_free = &pool->empty_chunks;
   while (*first_to_free && n > 0) {
     first_to_free = &(*first_to_free)->next;

src/common/mempool.h

@@ -38,6 +38,9 @@ struct mp_pool_t {
   struct mp_chunk_t *full_chunks;
   /** Length of <b>empty_chunks</b>. */
   int n_empty_chunks;
+  /** Lowest value of <b>empty_chunks</b> since last call to
+   * mp_pool_clean(-1). */
+  int min_empty_chunks;
   /** Size of each chunk (in items). */
   int new_chunk_capacity;
   /** Size to allocate for each item, including overhead and alignment

src/or/config.c

@@ -11,6 +11,8 @@ const char config_c_id[] = \
  * \brief Code to parse and interpret configuration files.
  **/
 
+#define CONFIG_PRIVATE
+
 #include "or.h"
 #ifdef MS_WINDOWS
 #include <shlobj.h>
@@ -594,8 +596,6 @@ static le_version_t decode_libevent_version(void);
 static void check_libevent_version(const char *m, int server);
 #endif
 
-/*static*/ or_options_t *options_new(void);
-
 /** Magic value for or_options_t. */
 #define OR_OPTIONS_MAGIC 9090909
 

src/or/control.c

@@ -10,6 +10,8 @@ const char control_c_id[] =
  *   See control-spec.txt for full details on protocol.
  **/
 
+#define CONTROL_PRIVATE
+
 #include "or.h"
 
 /** Yield true iff <b>s</b> is the state of a control_connection_t that has
@@ -84,10 +86,6 @@ typedef int event_format_t;
 static void connection_printf_to_buf(control_connection_t *conn,
                                      const char *format, ...)
   CHECK_PRINTF(2,3);
-/*static*/ size_t write_escaped_data(const char *data, size_t len,
-                                     int translate_newlines, char **out);
-/*static*/ size_t read_escaped_data(const char *data, size_t len,
-                                    int translate_newlines,  char **out);
 static void send_control_done(control_connection_t *conn);
 static void send_control_event(uint16_t event, event_format_t which,
                                const char *format, ...)
@@ -317,7 +315,7 @@ write_escaped_data(const char *data, size_t len, int translate_newlines,
  * that appears at the start of a line.  If <b>translate_newlines</b>
  * is true, replace all CRLF sequences with LF.  Return the number of
  * bytes in *<b>out</b>. */
-/*static*/ size_t
+/* static */ size_t
 read_escaped_data(const char *data, size_t len, int translate_newlines,
                   char **out)
 {

src/or/dirserv.c

@@ -35,6 +35,7 @@ static void directory_remove_invalid(void);
 static cached_dir_t *dirserv_regenerate_directory(void);
 static char *format_versions_list(config_line_t *ln);
 /* Should be static; exposed for testing */
+/* XXXX020 not actually tested. */
 struct authdir_config_t;
 int add_fingerprint_to_dir(const char *nickname, const char *fp,
                            struct authdir_config_t *list);
@@ -72,6 +73,7 @@ typedef struct authdir_config_t {
 } authdir_config_t;
 
 /** Should be static; exposed for testing. */
+/* XXXX020 not actually tested. */
 authdir_config_t *fingerprint_list = NULL;
 
 /** Allocate and return a new, empty, authdir_config_t. */
@@ -88,7 +90,7 @@ authdir_config_new(void)
  * the smartlist of fingerprint_entry_t's <b>list</b>. Return 0 if it's
  * new, or 1 if we replaced the old value.
  */
-int /* Should be static; exposed for testing */
+/* static */ int
 add_fingerprint_to_dir(const char *nickname, const char *fp,
                        authdir_config_t *list)
 {

src/or/main.c

@@ -117,8 +117,9 @@ static char* nt_strerror(uint32_t errnum);
 #define CHECK_DESCRIPTOR_INTERVAL (60)
 /** How often do we (as a router) check whether our IP address has changed? */
 #define CHECK_IPADDRESS_INTERVAL (15*60)
-/** How often do we check buffers for empty space that can be deallocated? */
-#define BUF_SHRINK_INTERVAL (60)
+/** How often do we check buffers and pools for empty space that can be
+ * deallocated? */
+#define MEM_SHRINK_INTERVAL (60)
 /** How often do we check for router descriptors that we should download? */
 #define DESCRIPTOR_RETRY_INTERVAL (10)
 /** How often do we 'forgive' undownloadable router descriptors and attempt
@@ -747,7 +748,7 @@ run_scheduled_events(time_t now)
   static time_t time_to_check_listeners = 0;
   static time_t time_to_check_descriptor = 0;
   static time_t time_to_check_ipaddress = 0;
-  static time_t time_to_shrink_buffers = 0;
+  static time_t time_to_shrink_memory = 0;
   static time_t time_to_try_getting_descriptors = 0;
   static time_t time_to_reset_descriptor_failures = 0;
   static time_t time_to_add_entropy = 0;
@@ -941,7 +942,7 @@ run_scheduled_events(time_t now)
   for (i=0;i<n_conns;i++) {
     run_connection_housekeeping(i, now);
   }
-  if (time_to_shrink_buffers < now) {
+  if (time_to_shrink_memory < now) {
     for (i=0;i<n_conns;i++) {
       connection_t *conn = connection_array[i];
       if (conn->outbuf)
@@ -949,7 +950,8 @@ run_scheduled_events(time_t now)
       if (conn->inbuf)
         buf_shrink(conn->inbuf);
     }
-    time_to_shrink_buffers = now + BUF_SHRINK_INTERVAL;
+    clean_cell_pool();
+    time_to_shrink_memory = now + MEM_SHRINK_INTERVAL;
   }
 
   /** 6. And remove any marked circuits... */

src/or/or.h

@@ -2137,6 +2137,11 @@ int or_state_save(time_t now);
 int getinfo_helper_config(control_connection_t *conn,
                           const char *question, char **answer);
 
+#ifdef CONFIG_PRIVATE
+/* Used only by config.c and test.c */
+or_options_t *options_new(void);
+#endif
+
 /********************************* connection.c ***************************/
 
 const char *conn_type_to_string(int type);
@@ -2406,6 +2411,14 @@ int decode_hashed_password(char *buf, const char *hashed);
 void disable_control_logging(void);
 void enable_control_logging(void);
 
+#ifdef CONTROL_PRIVATE
+/* Used only by control.c and test.c */
+size_t write_escaped_data(const char *data, size_t len,
+                          int translate_newlines, char **out);
+size_t read_escaped_data(const char *data, size_t len,
+                         int translate_newlines,  char **out);
+#endif
+
 /********************************* cpuworker.c *****************************/
 
 void cpu_init(void);
@@ -2669,6 +2682,7 @@ extern uint64_t stats_n_data_bytes_received;
 
 void init_cell_pool(void);
 void free_cell_pool(void);
+void clean_cell_pool(void);
 
 void cell_queue_clear(cell_queue_t *queue);
 void cell_queue_append(cell_queue_t *queue, packed_cell_t *cell);
@@ -2885,6 +2899,11 @@ void router_reset_warnings(void);
 void router_reset_reachability(void);
 void router_free_all(void);
 
+#ifdef ROUTER_PRIVATE
+/* Used only by router.c and test.c */
+void get_platform_str(char *platform, size_t len);
+#endif
+
 /********************************* routerlist.c ***************************/
 
 /** Represents information about a single trusted directory server. */

src/or/relay.c

@@ -1497,6 +1497,14 @@ free_cell_pool(void)
   cell_pool = NULL;
 }
 
+/** Free excess storage in cell pool. */
+void
+clean_cell_pool(void)
+{
+  tor_assert(cell_pool);
+  mp_pool_clean(cell_pool, -1);
+}
+
 /** Release storage held by <b>cell</b>. */
 static INLINE void
 packed_cell_free(packed_cell_t *cell)
@@ -1523,6 +1531,11 @@ free_cell_pool(void)
 {
 }
 
+void
+clean_cell_pool(void)
+{
+}
+
 static INLINE void
 packed_cell_free(packed_cell_t *cell)
 {

src/or/router.c

@@ -6,6 +6,8 @@
 const char router_c_id[] =
   "$Id$";
 
+#define ROUTER_PRIVATE
+
 #include "or.h"
 
 /**
@@ -16,8 +18,6 @@ const char router_c_id[] =
 
 extern long stats_n_seconds_working;
 
-/* Exposed for test.c. */ void get_platform_str(char *platform, size_t len);
-
 /************************************************************/
 
 /*****

src/or/test.c

@@ -23,7 +23,12 @@ const char test_c_id[] =
 #include <dirent.h>
 #endif
 
+/* These macros pull in declarations for some functions and structures that
+ * are typically file-private. */
+#define CONFIG_PRIVATE
+#define CONTROL_PRIVATE
 #define MEMPOOL_PRIVATE
+#define ROUTER_PRIVATE
 
 #include "or.h"
 #include "../common/test.h"
@@ -32,14 +37,6 @@ const char test_c_id[] =
 
 int have_failed = 0;
 
-/* These functions are file-local, but are exposed so we can test. */
-void get_platform_str(char *platform, size_t len);
-size_t read_escaped_data(const char *data, size_t len, int translate_newlines,
-                         char **out);
-or_options_t *options_new(void);
-int parse_addr_policy(config_line_t *cfg, addr_policy_t **dest,
-                      int assume_action);
-
 static char temp_dir[256];
 
 static void
@@ -2142,7 +2139,7 @@ test_mempool(void)
       //mp_pool_assert_ok(pool);
     }
     if (crypto_rand_int(777)==0)
-      mp_pool_clean(pool, 2);
+      mp_pool_clean(pool, -1);
 
     if (i % 777)
       mp_pool_assert_ok(pool);