Put fish on a diet. Tracked down the biggest memory hogs and fixed them. Total allocations down by a factor of 3 or so, live allocations a few KB.

builtin.cpp

@@ -3255,7 +3255,7 @@ static int builtin_end( parser_t &parser, wchar_t **argv )
 											parser.get_job_pos()-parser.current_block->tok_pos );
 					d->definition = def;
 		
-					function_add( d, parser );	
+					function_add( *d, parser );	
 					free( def );
 				}
 				else

builtin_set.cpp

@@ -92,7 +92,7 @@ static int my_env_set( const wchar_t *key, wcstring_list_t &val, int scope )
 			{
 				const wchar_t *colon;
 				
-                append_format(stderr_buffer, _(BUILTIN_SET_PATH_ERROR), L"set", dir, key);				
+                append_format(stderr_buffer, _(BUILTIN_SET_PATH_ERROR), L"set", dir, key);
 				colon = wcschr( dir, L':' );
 				
 				if( colon && *(colon+1) ) 

common.cpp

@@ -726,18 +726,19 @@ void debug_safe(int level, const char *msg, const char *param1, const char *para
     errno = errno_old;
 }
 
-void format_int_safe(char buff[128], int val) {
+void format_long_safe(char buff[128], long val) {
     if (val == 0) {
         strcpy(buff, "0");
     } else {
         /* Generate the string in reverse */
         size_t idx = 0;
         bool negative = (val < 0);
-        if (negative)
+        
-            val = -val;
+        /* Note that we can't just negate val if it's negative, because it may be the most negative value. We do rely on round-towards-zero division though. */
 
-        while (val > 0) {
+        while (val != 0) {
-            buff[idx++] = '0' + (val % 10);
+            long rem = val % 10;
+            buff[idx++] = '0' + (rem < 0 ? -rem : rem);
             val /= 10;
         }
         if (negative)
@@ -753,6 +754,33 @@ void format_int_safe(char buff[128], int val) {
     }
 }
 
+void format_long_safe(wchar_t buff[128], long val) {
+    if (val == 0) {
+        wcscpy(buff, L"0");
+    } else {
+        /* Generate the string in reverse */
+        size_t idx = 0;
+        bool negative = (val < 0);
+
+        while (val > 0) {
+            long rem = val % 10;
+            /* Here we're assuming that wide character digits are contiguous - is that a correct assumption? */
+            buff[idx++] = L'0' + (rem < 0 ? -rem : rem);
+            val /= 10;
+        }
+        if (negative)
+            buff[idx++] = L'-';
+        buff[idx] = 0;
+        
+        size_t left = 0, right = idx - 1;
+        while (left < right)  {
+            wchar_t tmp = buff[left];
+            buff[left++] = buff[right];
+            buff[right--] = tmp;
+        }
+    }
+}
+
 void write_screen( const wcstring &msg, wcstring &buff )
 {
 	const wchar_t *start, *pos;

common.h

@@ -281,15 +281,6 @@ void assert_is_locked(void *mutex, const char *who);
 */
 char *wcs2str_internal( const wchar_t *in, char *out );
 
-/**
- Converts some type to a wstring.
- */
-template<typename T>
-wcstring format_val(T x) {
-    std::wstringstream stream;
-    stream << x;
-    return stream.str();
-}
 
 template<typename T>
 T from_string(const wcstring &x) {
@@ -678,8 +669,31 @@ void format_size_safe(char buff[128], unsigned long long sz);
 /** Our crappier versions of debug which is guaranteed to not allocate any memory, or do anything other than call write(). This is useful after a call to fork() with threads. */
 void debug_safe(int level, const char *msg, const char *param1 = NULL, const char *param2 = NULL, const char *param3 = NULL);
 
-/** Writes out an int safely */
+/** Writes out a long safely */
-void format_int_safe(char buff[128], int val);
+void format_long_safe(char buff[128], long val);
+void format_long_safe(wchar_t buff[128], long val);
+
+
+/** Converts some type to a wstring. */
+template<typename T>
+inline wcstring format_val(T x) {
+    std::wstringstream stream;
+    stream << x;
+    return stream.str();
+}
+
+/* wstringstream is a huge memory pig. Let's provide some specializations where we can. */
+template<>
+inline wcstring format_val(long x) {
+    wchar_t buff[128];
+    format_long_safe(buff, x);
+    return wcstring(buff);
+}
+
+template<>
+inline wcstring format_val(int x) {
+    return format_val(static_cast<long>(x));
+}
 
 
 /**

event.cpp

@@ -136,7 +136,7 @@ static int event_match( const event_t *classv, const event_t *instance )
    Create an identical copy of an event. Use deep copying, i.e. make
    duplicates of any strings used as well.
 */
-static event_t *event_copy( event_t *event, int copy_arguments )
+static event_t *event_copy( const event_t *event, int copy_arguments )
 {
     event_t *e = new event_t(*event);
     
@@ -232,7 +232,7 @@ static void show_all_handlers(void) {
 }
 #endif
 
-void event_add_handler( event_t *event )
+void event_add_handler( const event_t *event )
 {
 	event_t *e;
 

event.h

@@ -100,7 +100,7 @@ struct event_t
 
    May not be called by a signal handler, since it may allocate new memory.
 */
-void event_add_handler( event_t *event );
+void event_add_handler( const event_t *event );
 
 /**
    Remove all events matching the specified criterion. 

fish_tests.cpp

@@ -171,10 +171,17 @@ static void test_format(void) {
     
     for (int j=-129; j <= 129; j++) {
         char buff1[128], buff2[128];
-        format_int_safe(buff1, j);
+        format_long_safe(buff1, j);
         sprintf(buff2, "%d", j);
         assert( ! strcmp(buff1, buff2));
-    }    
+    }
+    
+    long q = LONG_MIN;
+    char buff1[128], buff2[128];
+    format_long_safe(buff1, q);
+    sprintf(buff2, "%ld", q);
+    assert( ! strcmp(buff1, buff2));
+    
 }
 
 /**

function.cpp

@@ -79,7 +79,7 @@ void function_autoload_t::command_removed(const wcstring &cmd) {
    that the function being defined is autoloaded. There should be a
    better way to do this...
 */
-static int is_autoload = 0;
+static bool is_autoload = false;
 
 /**
    Make sure that if the specified function is a dynamically loaded
@@ -89,7 +89,7 @@ static int load( const wcstring &name )
 {
     ASSERT_IS_MAIN_THREAD();
     scoped_lock lock(functions_lock);
-	int was_autoload = is_autoload;
+	bool was_autoload = is_autoload;
 	int res;
     function_map_t::iterator iter = loaded_functions.find(name);
 	if( iter !=  loaded_functions.end() && !iter->second->is_autoload ) {
@@ -97,7 +97,7 @@ static int load( const wcstring &name )
 		return 0;
     }
 	
-	is_autoload = 1;	
+	is_autoload = true;	
 	res = function_autoloader.load( name, true );
 	is_autoload = was_autoload;
 	return res;
@@ -155,36 +155,51 @@ void function_init()
     VOMIT_ON_FAILURE(pthread_mutexattr_destroy(&a));
 }
 
-
+function_info_t::function_info_t(const function_data_t &data, const wchar_t *filename, int def_offset, bool autoload) :
-void function_add( function_data_t *data, const parser_t &parser )
+    definition(data.definition),
+    description(data.description),
+    definition_file(intern(filename)),
+    definition_offset(def_offset),
+    named_arguments(data.named_arguments),
+    is_autoload(autoload),
+    shadows(data.shadows)
 {
-	CHECK( ! data->name.empty(), );
+}
-	CHECK( data->definition, );
-	scoped_lock lock(functions_lock);
-	function_remove( data->name );
-	    
-    shared_ptr<function_info_t> &info = loaded_functions[data->name];
-    if (! info) {
-        info.reset(new function_info_t);
-    }
-    
-	info->definition_offset = parse_util_lineno( parser.get_buffer(), parser.current_block->tok_pos )-1;
-	info->definition = data->definition;
 
-    if (! data->named_arguments.empty()) {
+function_info_t::function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, bool autoload) :
-        info->named_arguments.insert(info->named_arguments.end(), data->named_arguments.begin(), data->named_arguments.end());
+    definition(data.definition),
-    }	
+    description(data.description),
+    definition_file(intern(filename)),
+    definition_offset(def_offset),
+    named_arguments(data.named_arguments),
+    is_autoload(autoload),
+    shadows(data.shadows)
+{
+}
+
+void function_add( const function_data_t &data, const parser_t &parser )
+{
+    ASSERT_IS_MAIN_THREAD();
+    
+	CHECK( ! data.name.empty(), );
+	CHECK( data.definition, );
+	scoped_lock lock(functions_lock);
+    
+    /* Remove the old function */
+	function_remove( data.name );
+    
+    /* Create a new function */
+    const wchar_t *filename = reader_current_filename();
+    int def_offset = parse_util_lineno( parser.get_buffer(), parser.current_block->tok_pos )-1;
+    function_info_t *new_info = new function_info_t(data, filename, def_offset, is_autoload);
+    
+    /* Store it in the loaded_functions map */
+    loaded_functions[data.name].reset(new_info);
 	
-	if (! data->description.empty())
+    /* Add event handlers */
-        info->description = data->description;
+	for( std::vector<event_t>::const_iterator iter = data.events.begin(); iter != data.events.end(); ++iter )
-	info->definition_file = intern(reader_current_filename());
-	info->is_autoload = is_autoload;
-	info->shadows = data->shadows;
-	
-	
-	for( size_t i=0; i< data->events.size(); i++ )
 	{
-		event_add_handler( &data->events.at(i) );
+		event_add_handler( &*iter );
 	}
 }
 
@@ -273,21 +288,17 @@ void function_set_desc( const wcstring &name, const wcstring &desc )
     if (func) func->description = desc;
 }
 
-int function_copy( const wcstring &name, const wcstring &new_name )
+bool function_copy( const wcstring &name, const wcstring &new_name )
 {
-    int result = 0;
+    bool result = false;
     scoped_lock lock(functions_lock);
     function_map_t::const_iterator iter = loaded_functions.find(name);
     if (iter != loaded_functions.end()) {
         shared_ptr<function_info_t> &new_info = loaded_functions[new_name];
-        new_info.reset(new function_info_t(*iter->second));
         
-		// This new instance of the function shouldn't be tied to the def 
+		// This new instance of the function shouldn't be tied to the definition file of the original, so pass NULL filename, etc.
-		// file of the original. 
+        new_info.reset(new function_info_t(*iter->second, NULL, 0, false));
-		new_info->definition_file = 0;
+        result = true;
-		new_info->is_autoload = 0;
-        
-        result = 1;
     }
 	return result;
 }

function.h

@@ -60,37 +60,32 @@ struct function_data_t
 
 class function_info_t {
 public:
-    /** Function definition */
+    /** Constructs relevant information from the function_data */
-    wcstring definition;
+    function_info_t(const function_data_t &data, const wchar_t *filename, int def_offset, bool autoload);
     
-    /** Function description */
+    /** Used by function_copy */
+    function_info_t(const function_info_t &data, const wchar_t *filename, int def_offset, bool autoload);
+
+    /** Function definition */
+    const wcstring definition;
+    
+    /** Function description. Only the description may be changed after the function is created. */
     wcstring description;
     
-	/**
+	/** File where this function was defined (intern'd string) */
-	   File where this function was defined (intern'd string)
+    const wchar_t * const definition_file;
-	*/
-    const wchar_t *definition_file;
     
-	/**
+	/** Line where definition started */
-	   Line where definition started
+	const int definition_offset;
-	*/
-	int definition_offset;
     
-	/**
+	/** List of all named arguments for this function */
-	   List of all named arguments for this function
+    const wcstring_list_t named_arguments;
-	 */
-    wcstring_list_t named_arguments;
     
-	/**
+	/** Flag for specifying that this function was automatically loaded */
-	   Flag for specifying that this function was automatically loaded
+    const bool is_autoload;
-	*/
-    bool is_autoload;
     
-	/**
+	/** Set to true if invoking this function shadows the variables of the underlying function. */
-	   Set to non-zero if invoking this function shadows the variables
+	const bool shadows;
-	   of the underlying function.
-	 */
-	bool shadows;
 };
 
 
@@ -99,11 +94,8 @@ public:
 */
 void function_init();
 
-/**
+/** Add a function. */
-   Add a function. The parameters values are copied and should be
+void function_add( const function_data_t &data, const parser_t &parser );
-   freed by the caller.
-*/
-void function_add( function_data_t *data, const parser_t &parser );
 
 /**
    Remove the function with the specified name.
@@ -172,9 +164,9 @@ wcstring_list_t function_get_named_arguments( const wcstring &name );
 
 /**
    Creates a new function using the same definition as the specified function.
-   Returns non-zero if copy is successful.
+   Returns true if copy is successful.
 */
-int function_copy( const wcstring &name, const wcstring &new_name );
+bool function_copy( const wcstring &name, const wcstring &new_name );
 
 
 /**

highlight.cpp

@@ -1331,7 +1331,7 @@ static void highlight_universal_internal( const wcstring &buffstr,
 		*/
 		if( (buffstr.at(pos) == L'\'') || (buffstr.at(pos) == L'\"') )
 		{
-			std::deque<long> lst;
+			std::vector<long> lst;
 		
 			int level=0;
 			wchar_t prev_q=0;

intern.cpp

@@ -11,6 +11,8 @@
 #include <wchar.h>
 #include <unistd.h>
 #include <set>
+#include <deque>
+#include <algorithm>
 
 #include "fallback.h"
 #include "util.h"
@@ -20,13 +22,24 @@
 #include "intern.h"
 
 /** Comparison function for intern'd strings */
-static bool string_table_compare(const wchar_t *a, const wchar_t *b) {
+class string_table_compare_t {
-    return wcscmp(a, b) < 0;
+    public:
-}
+    bool operator()(const wchar_t *a, const wchar_t *b) const {
+        return wcscmp(a, b) < 0;
+    }
+};
 
+/* A sorted deque ends up being a little more memory efficient than a std::set for the intern'd string table */
+#define USE_SET 0
+#if USE_SET
 /** The table of intern'd strings */
-typedef std::set<const wchar_t *, bool (*)(const wchar_t *, const wchar_t *b)> string_table_t;
+typedef std::set<const wchar_t *, string_table_compare_t> string_table_t;
-static string_table_t string_table(string_table_compare);
+#else
+/** The table of intern'd strings */
+typedef std::deque<const wchar_t *> string_table_t;
+#endif
+
+static string_table_t string_table;
 
 /** The lock to provide thread safety for intern'd strings */
 static pthread_mutex_t intern_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -39,6 +52,8 @@ static const wchar_t *intern_with_dup( const wchar_t *in, bool dup )
 //	debug( 0, L"intern %ls", in );
     scoped_lock lock(intern_lock);
     const wchar_t *result;
+    
+#if USE_SET
     string_table_t::const_iterator iter = string_table.find(in);
     if (iter != string_table.end()) {
         result = *iter; 
@@ -46,6 +61,15 @@ static const wchar_t *intern_with_dup( const wchar_t *in, bool dup )
         result = dup ? wcsdup(in) : in;
         string_table.insert(result);
     }
+#else
+    string_table_t::iterator iter = std::lower_bound(string_table.begin(), string_table.end(), in, string_table_compare_t());
+    if (iter != string_table.end() && wcscmp(*iter, in) == 0) {
+        result = *iter;
+    } else {
+        result = dup ? wcsdup(in) : in;
+        string_table.insert(iter, result);
+    }
+#endif
     return result;
 }
 

parser.h

@@ -12,7 +12,6 @@
 #include "event.h"
 #include "function.h"
 #include <vector>
-#include <deque>
 #include <memory>
 
 #define PARSER_TEST_ERROR 1
@@ -34,7 +33,7 @@ struct event_block_t
 	unsigned int typemask;
 };
 
-typedef std::deque<event_block_t> event_block_list_t;
+typedef std::list<event_block_t> event_block_list_t;
 
 inline bool event_block_list_blocks_type(const event_block_list_t &ebls, int type) {
     for (event_block_list_t::const_iterator iter = ebls.begin(); iter != ebls.end(); ++iter) {

reader.cpp

@@ -324,10 +324,8 @@ static int is_interactive_read;
 */
 static int end_loop = 0;
 
-/**
+/** The stack containing names of files that are being parsed */
-   The stack containing names of files that are being parsed
+static std::stack<const wchar_t *, std::list<const wchar_t *> > current_filename;
-*/
-static std::stack<const wchar_t *> current_filename;
 
 
 /**