LRU work to load functions off of the main thread.

We'll have to reevaluate this after we fix function autocomplete

autoload.cpp

@@ -13,6 +13,9 @@ The classes responsible for autoloading functions and completions.
 #include "exec.h"
 #include <assert.h>
 
+/* The time before we'll recheck an autoloaded file */
+static const int kAutoloadStalenessInterval = 1;
+
 file_access_attempt_t access_file(const wcstring &path, int mode) {
     file_access_attempt_t result = {0};
     struct stat statbuf;
@@ -91,6 +94,19 @@ void lru_cache_impl_t::promote_node(lru_node_t *node) {
 }
 
 bool lru_cache_impl_t::add_node(lru_node_t *node) {
+    /* Add our node without eviction */
+    if (! this->add_node_without_eviction(node))
+        return false;
+    
+    /* Evict */
+    while (node_count > max_node_count)
+        evict_last_node();
+    
+    /* Success */
+    return true;
+}
+
+bool lru_cache_impl_t::add_node_without_eviction(lru_node_t *node) {
     assert(node != NULL && node != &mouth);
     
     /* Try inserting; return false if it was already in the set */
@@ -144,9 +160,17 @@ autoload_t::autoload_t(const wcstring &env_var_name_var, const builtin_script_t
                        builtin_scripts(scripts),
                        builtin_script_count(script_count)
 {
+    pthread_mutex_init(&lock, NULL);
+}
+
+autoload_t::~autoload_t() {
+    pthread_mutex_destroy(&lock);
 }
 
 void autoload_t::node_was_evicted(autoload_function_t *node) {
+    // This should only ever happen on the main thread
+    ASSERT_IS_MAIN_THREAD();
+    
     // Tell ourselves that the command was removed if it was loaded
     if (! node->is_loaded)
         this->command_removed(node->key);
@@ -163,28 +187,33 @@ int autoload_t::load( const wcstring &cmd, bool reload )
 	int res;
 	
 	CHECK_BLOCK( 0 );
+    ASSERT_IS_MAIN_THREAD();
     
-	const env_var_t path_var = env_get_string( env_var_name.c_str() );
+	env_var_t path_var;
     
-	/*
+    /* Do some work while locked, including determing the path variable */
-	  Do we know where to look?
-	*/
-	if( path_var.empty() )
-		return 0;
-    
-    /*
-      Check if the lookup path has changed. If so, drop all loaded
-      files.
-    */
-    if( path_var != this->path )
     {
-        this->path = path_var;
+        scoped_lock locker(lock); 
-        this->evict_all_nodes();
+        path_var = env_get_string( env_var_name.c_str() );
+        
+        /*
+          Do we know where to look?
+        */
+        if( path_var.empty() )
+            return 0;
+        
+        /*
+          Check if the lookup path has changed. If so, drop all loaded
+          files.
+        */
+        if( path_var != this->path )
+        {
+            this->path = path_var;
+            this->evict_all_nodes();
+        }
     }
     
-    /**
+    /** Warn and fail on infinite recursion. It's OK to do this because this function is only called on the main thread. */
-       Warn and fail on infinite recursion
-    */
     if (this->is_loading(cmd))
     {
         debug( 0, 
@@ -194,26 +223,31 @@ int autoload_t::load( const wcstring &cmd, bool reload )
         return 1;
     }
     
+    /* Mark that we're loading this */
+    is_loading_set.insert(cmd);
 
+    
+    /* Get the list of paths from which we will try to load */
     std::vector<wcstring> path_list;
 	tokenize_variable_array2( path_var, path_list );
 
-    is_loading_set.insert(cmd);
+	/* Try loading it */
-
-	/*
-	  Do the actual work in the internal helper function
-	*/
 	res = this->locate_file_and_maybe_load_it( cmd, true, reload, path_list );
     
+    /* Clean up */
     int erased = is_loading_set.erase(cmd);
     assert(erased);
 		
 	return res;
 }
 
-bool autoload_t::can_load( const wcstring &cmd )
+bool autoload_t::can_load( const wcstring &cmd, const env_vars &vars )
 {
-    const env_var_t path_var = env_get_string( env_var_name.c_str() );
+    const wchar_t *path_var_ptr = vars.get(env_var_name.c_str());
+    if (! path_var_ptr || ! path_var_ptr[0])
+        return false;
+    
+    const wcstring path_var(path_var_ptr);        
     std::vector<wcstring> path_list;
 	tokenize_variable_array2( path_var, path_list );
     return this->locate_file_and_maybe_load_it( cmd, false, false, path_list );
@@ -225,36 +259,64 @@ static bool script_name_precedes_script_name(const builtin_script_t &script1, co
 }
 
 void autoload_t::unload_all(void) {
+    scoped_lock locker(lock);
     this->evict_all_nodes();
 }
 
+bool autoload_t::file_already_autoloaded(const wcstring &cmd, bool require_loaded, bool allow_stale_functions) {
+    bool result = false;
+    
+    /* Take a lock */
+    scoped_lock locker(lock);
+    
+    /* Get the function */
+    autoload_function_t * func = this->get_node(cmd);
+    
+    if (func != NULL) {
+        if (require_loaded && ! func->is_loaded) {
+            /* If the function is not loaded, and we're only interested in loaded functions, return false */
+            result = false;
+        } else if (! allow_stale_functions && time(NULL) - func->access.last_checked > kAutoloadStalenessInterval) {
+            /* If the function is stale, and we are not interested in stale functions, we return false */
+            result = false;
+        } else {
+            /* Success */
+            result = true;
+        }
+    }
+    return result;
+}
+
 /**
    This internal helper function does all the real work. By using two
    functions, the internal function can return on various places in
    the code, and the caller can take care of various cleanup work.
-*/
    
+     cmd: the command name ('grep')
+     really_load: whether to actually parse it as a function, or just check it it exists
+     reload: whether to reload it if it's already loaded
+     path_list: the set of paths to check
+     
+     Result: if really_load is true, returns whether the function was loaded. Otherwise returns whether the function existed.
+*/
 bool autoload_t::locate_file_and_maybe_load_it( const wcstring &cmd, bool really_load, bool reload, const wcstring_list_t &path_list )
 {
-
+    /* Note that we are NOT locked in this function! */
 	size_t i;
 	bool reloaded = 0;
 
-    /* Get the function */
+    /* Return if the file is already loaded. If we really want the function to be loaded, require that it be really loaded. If we're not reloading, allow stale functions. */
-    autoload_function_t * func = this->get_node(cmd);
+    if (file_already_autoloaded(cmd, really_load, ! reload)) {
-
+        return true;
-    /* Return if already loaded and we are skipping reloading */
+    }
-	if( !reload && func && func->is_loaded )
-		return 0;
-
-    /* Nothing to do if we just checked it */
-    if (func && func->is_loaded && time(NULL) - func->access.last_checked <= 1)
-        return 0;
     
     /* The source of the script will end up here */
     wcstring script_source;
     bool has_script_source = false;
     
+    /* Whether we found an accessible file */
+    bool found_file = false;
+    
     /* Look for built-in scripts via a binary search */
     const builtin_script_t *matching_builtin_script = NULL;
     if (builtin_script_count > 0)
@@ -283,60 +345,78 @@ bool autoload_t::locate_file_and_maybe_load_it( const wcstring &cmd, bool really
 
             const file_access_attempt_t access = access_file(path, R_OK);
             if (access.accessible) {
-                if (! func || access.mod_time != func->access.mod_time) {
+                /* Found it! */
+                found_file = true;
+                
+                /* Now we're actually going to take the lock. */
+                scoped_lock locker(lock);
+                autoload_function_t *func = this->get_node(cmd);
+                
+                /* Generate the source if we need to load it */
+                bool need_to_load_function = really_load && (func == NULL || func->access.mod_time == access.mod_time || ! func->is_loaded);
+                if (need_to_load_function) {
+                
+                    /* Generate the script source */
                     wcstring esc = escape_string(path, 1);
                     script_source = L". " + esc;
                     has_script_source = true;
                     
-                    /* Remove this command because we are going to reload it */
+                    /* Remove any loaded command because we are going to reload it. Note that this will deadlock if command_removed calls back into us. */
                     if (func && func->is_loaded) {
                         command_removed(cmd);
-                        func->is_loaded = false;
                         func->is_placeholder = false;
                     }
                     
-                    if (!func) {
+                    /* Mark that we're reloading it */
-                        func = new autoload_function_t(cmd);
-                        this->add_node(func);
-                        
-                    }
-                    
-                    /* Record our access time */
-                    func->access = access;
-                                                            
                     reloaded = true;
                 }
-                else if( func )
+                
-                {
+                /* Create the function if we haven't yet. This does not load it. Do not trigger eviction unless we are actually loading, because we don't want to evict off of the main thread. */
-                    /*
+                if (! func) {
-                      If we are rechecking an autoload file, and it hasn't
+                    func = new autoload_function_t(cmd);
-                      changed, update the 'last check' timestamp.
+                    if (really_load) {
-                    */
+                        this->add_node(func);
-                    func->access = access;
+                    } else {
+                        this->add_node_without_eviction(func);
+                    }
                 }
                 
+                /* It's a fiction to say the script is loaded at this point, but we're definitely going to load it down below. */
+                if (need_to_load_function) func->is_loaded = true;
+                                                                
+                /* Unconditionally record our access time */
+                func->access = access;
+
                 break;
             }
         }
 
         /*
-          If no file was found we insert a placeholder function. Later we only
+          If no file or builtin script was found we insert a placeholder function.
-          research if the current time is at least five seconds later.
+          Later we only research if the current time is at least five seconds later.
           This way, the files won't be searched over and over again.
         */
-        if( !func )
+        if( ! found_file && ! has_script_source )
         {
-            func = new autoload_function_t(cmd);
+            scoped_lock locker(lock);
+            /* Generate a placeholder */
+            autoload_function_t *func = this->get_node(cmd);
+            if (! func) {
+                func = new autoload_function_t(cmd);
+                func->is_placeholder = true;
+                if (really_load) {
+                    this->add_node(func);
+                } else {
+                    this->add_node_without_eviction(func);
+                }
+            }
             func->access.last_checked = time(NULL);
-            func->is_placeholder = true;
-            this->add_node(func);
         }
     }
     
     /* If we have a script, either built-in or a file source, then run it */
     if (really_load && has_script_source)
     {
-        if (func) func->is_loaded = true;
         if( exec_subshell( script_source.c_str(), 0 ) == -1 )
         {
             /*
@@ -346,5 +426,9 @@ bool autoload_t::locate_file_and_maybe_load_it( const wcstring &cmd, bool really
 
     }
 
-	return reloaded;	
+    if (really_load) {
+        return reloaded;
+    } else {
+        return found_file || has_script_source;
+    }
 }

autoload.h

@@ -79,6 +79,9 @@ public:
     /** Adds a node under the given key. Returns true if the node was added, false if the node was not because a node with that key is already in the set. */
     bool add_node(lru_node_t *node);
     
+    /** Adds a node under the given key without triggering eviction. Returns true if the node was added, false if the node was not because a node with that key is already in the set. */
+    bool add_node_without_eviction(lru_node_t *node);
+    
     /** Counts nodes */
     size_t size(void) { return node_count; }
     
@@ -105,6 +108,7 @@ struct autoload_function_t : public lru_node_t
 
 
 struct builtin_script_t;
+class env_vars;
 
 /**
   A class that represents a path from which we can autoload, and the autoloaded contents.
@@ -112,6 +116,9 @@ struct builtin_script_t;
 class autoload_t : private lru_cache_t<autoload_function_t> {
 private:
 
+    /** Lock for thread safety */
+    pthread_mutex_t lock;
+
     /** The environment variable name */
     const wcstring env_var_name;
     
@@ -138,6 +145,8 @@ private:
         this->evict_all_nodes();
     }
         
+    bool file_already_autoloaded(const wcstring &cmd, bool require_loaded, bool allow_stale_functions);
+    
     bool locate_file_and_maybe_load_it( const wcstring &cmd, bool really_load, bool reload, const wcstring_list_t &path_list );
     
     virtual void node_was_evicted(autoload_function_t *node);
@@ -152,6 +161,9 @@ private:
     /** Create an autoload_t for the given environment variable name */
     autoload_t(const wcstring &env_var_name_var, const builtin_script_t *scripts, size_t script_count );
     
+    /** Destructor */
+    virtual ~autoload_t();
+    
     /**
        Autoload the specified file, if it exists in the specified path. Do
        not load it multiple times unless it's timestamp changes or
@@ -181,7 +193,7 @@ private:
     void unload_all( );
     
     /** Check whether the given command could be loaded, but do not load it. */
-    bool can_load( const wcstring &cmd );
+    bool can_load( const wcstring &cmd, const env_vars &vars );
 
 };
 

common.h

@@ -19,6 +19,7 @@
 #include <string.h>
 
 #include <errno.h>
+#include <assert.h>
 #include "util.h"
 
 /* Common string type */
@@ -289,14 +290,28 @@ T from_string(const wcstring &x) {
 }
 
 class scoped_lock {
-    pthread_mutex_t *lock;
+    pthread_mutex_t *lock_obj;
+    bool locked;
 public:
-    scoped_lock(pthread_mutex_t &mutex) : lock(&mutex) {
+
-        VOMIT_ON_FAILURE(pthread_mutex_lock(lock));
+    void lock(void) {
+        assert(! locked);
+        VOMIT_ON_FAILURE(pthread_mutex_lock(lock_obj));
+        locked = true;
+    }
+    
+    void unlock(void) {
+        assert(locked);
+        VOMIT_ON_FAILURE(pthread_mutex_unlock(lock_obj));
+        locked = false;
+    }
+    
+    scoped_lock(pthread_mutex_t &mutex) : lock_obj(&mutex), locked(false) {
+        this->lock();
     }
     
     ~scoped_lock() {
-        VOMIT_ON_FAILURE(pthread_mutex_unlock(lock));
+        if (locked) this->unlock();
     }
 };
 

env.cpp

@@ -1836,4 +1836,4 @@ const wchar_t *env_vars::get(const wchar_t *key) const
     return (iter == vars.end() ? NULL : iter->second.c_str());
 }
 
-const wchar_t * const env_vars::highlighting_keys[] = {L"PATH", L"CDPATH", L"HIGHLIGHT_DELAY", NULL};
+const wchar_t * const env_vars::highlighting_keys[] = {L"PATH", L"CDPATH", L"HIGHLIGHT_DELAY", L"fish_function_path", NULL};

function.cpp

@@ -95,6 +95,7 @@ static int load( const wchar_t *name )
 	int res;
     function_map_t::iterator iter = loaded_functions.find(name);
 	if( iter !=  loaded_functions.end() && !iter->second->is_autoload ) {
+        /* We have a non-autoload version already */
 		return 0;
     }
 	
@@ -196,28 +197,23 @@ void function_add( function_data_t *data, const parser_t &parser )
 	}
 }
 
-static int function_exists_internal( const wchar_t *cmd, bool autoload )
-{
-	int res;
-	CHECK( cmd, 0 );
-	
-	if( parser_keywords_is_reserved(cmd) )
-		return 0;
-	
-    scoped_lock lock(functions_lock);
-	if ( autoload ) load( cmd );
-    res = loaded_functions.find(cmd) != loaded_functions.end();
-    return res;
-}
-
 int function_exists( const wchar_t *cmd )
 {
-    return function_exists_internal( cmd, true );
+	CHECK( cmd, 0 );
+	if( parser_keywords_is_reserved(cmd) )
+		return 0;
+    scoped_lock lock(functions_lock);
+    load(cmd);
+    return loaded_functions.find(cmd) != loaded_functions.end();            
 }
 
-int function_exists_no_autoload( const wchar_t *cmd )
+int function_exists_no_autoload( const wchar_t *cmd, const env_vars &vars )
 {
-    return function_exists_internal( cmd, false );    
+	CHECK( cmd, 0 );
+	if( parser_keywords_is_reserved(cmd) )
+		return 0;
+    scoped_lock lock(functions_lock);
+    return loaded_functions.find(cmd) != loaded_functions.end() || function_autoloader.can_load(cmd, vars);            
 }
 
 static bool function_remove_ignore_autoload(const wcstring &name)

function.h

@@ -19,6 +19,7 @@
 using std::tr1::shared_ptr;
 
 class parser_t;
+class env_vars;
 
 /**
    Structure describing a function. This is used by the parser to
@@ -136,7 +137,7 @@ int function_exists( const wchar_t *name );
 /**
    Returns true if the function with the name name exists, without triggering autoload.
 */
-int function_exists_no_autoload( const wchar_t *name );
+int function_exists_no_autoload( const wchar_t *name, const env_vars &vars );
 
 /**
    Returns all function names.

highlight.cpp

@@ -716,7 +716,7 @@ void tokenize( const wchar_t * const buff, int * const color, const int pos, arr
 								is_cmd |= builtin_exists( cmd );
 							
 							if( use_function )
-								is_cmd |= function_exists_no_autoload( cmd );
+								is_cmd |= function_exists_no_autoload( cmd, vars );
 							
 							/*
 							 Moving on to expensive tests