From a597b0e6e1f672ce4608db16566778fb8d0cc488 Mon Sep 17 00:00:00 2001 From: ridiculousfish Date: Wed, 10 Apr 2019 16:15:47 -0700 Subject: [PATCH] Remove get_proc_had_barrier Prior to this change, fish used a global flag to decide if we should check for changes to universal variables. This flag was then checked at arbitrary locations, potentially triggering variable updates and event handlers for those updates; this was very hard to reason about. Switch to triggering a universal variable update at a fixed location, after running an external command. The common case is that the variable file has not changed, which we can identify with just a stat() call, so this is pretty cheap. --- src/env.cpp | 13 +------------ src/exec.cpp | 14 -------------- src/parse_execution.cpp | 5 +++-- src/proc.cpp | 12 ------------ src/proc.h | 9 --------- 5 files changed, 4 insertions(+), 49 deletions(-) diff --git a/src/env.cpp b/src/env.cpp index 0b6c22b99..fa071cd42 100644 --- a/src/env.cpp +++ b/src/env.cpp @@ -658,10 +658,6 @@ int env_stack_t::set_internal(const wcstring &key, env_mode_flags_t input_var_mo } } } else { - if (!get_proc_had_barrier()) { - set_proc_had_barrier(true); - env_universal_barrier(); - } if (uvars() && uvars()->get(key)) { // Modifying an existing universal variable. env_set_internal_universal(key, std::move(val), var_mode, this); @@ -907,13 +903,6 @@ maybe_t env_stack_t::get(const wcstring &key, env_mode_flags_t mode) if (!search_universal) return none(); - // Another hack. Only do a universal barrier on the main thread (since it can change variable - // values). Make sure we do this outside the env_lock because it may itself call `get()`. - if (is_main_thread() && !get_proc_had_barrier()) { - set_proc_had_barrier(true); - env_universal_barrier(); - } - // Okay, we couldn't find a local or global var given the requirements. If there is a matching // universal var return that. if (uvars()) { @@ -926,7 +915,7 @@ maybe_t env_stack_t::get(const wcstring &key, env_mode_flags_t mode) return none(); } -void env_universal_barrier() { env_stack_t::principal().universal_barrier(); } +void env_universal_barrier() {env_stack_t::principal().universal_barrier(); } /// Returns true if the specified scope or any non-shadowed non-global subscopes contain an exported /// variable. diff --git a/src/exec.cpp b/src/exec.cpp index 70f50c0f2..1f2827d38 100644 --- a/src/exec.cpp +++ b/src/exec.cpp @@ -926,20 +926,6 @@ static bool exec_process_in_job(parser_t &parser, process_t *p, std::shared_ptr< } } - // This call is used so the global environment variable array is regenerated, if needed, - // before the fork. That way, we avoid a lot of duplicate work where EVERY child would need - // to generate it, since that result would not get written back to the parent. This call - // could be safely removed, but it would result in slightly lower performance - at least on - // uniprocessor systems. - if (p->type == process_type_t::external) { - // Apply universal barrier so we have the most recent uvar changes - if (!get_proc_had_barrier()) { - set_proc_had_barrier(true); - env_universal_barrier(); - } - parser.vars().export_arr(); - } - // Execute the process. p->check_generations_before_launch(); switch (p->type) { diff --git a/src/parse_execution.cpp b/src/parse_execution.cpp index 694c50fb3..e2eccca1e 100644 --- a/src/parse_execution.cpp +++ b/src/parse_execution.cpp @@ -1274,9 +1274,10 @@ parse_execution_result_t parse_execution_context_t::run_1_job(tnode_t jo remove_job(job.get()); } - // Only external commands require a new fishd barrier. + // Update universal vaiables on external conmmands. + // TODO: justify this, why not on every command? if (job_contained_external_command) { - set_proc_had_barrier(false); + parser->vars().universal_barrier(); } } diff --git a/src/proc.cpp b/src/proc.cpp index 4d64bbaa2..1d1817583 100644 --- a/src/proc.cpp +++ b/src/proc.cpp @@ -75,8 +75,6 @@ int no_exec = 0; static int is_interactive = -1; -static bool proc_had_barrier = false; - bool shell_is_interactive() { ASSERT_IS_MAIN_THREAD(); // is_interactive is statically initialized to -1. Ensure it has been dynamically set @@ -85,16 +83,6 @@ bool shell_is_interactive() { return is_interactive > 0; } -bool get_proc_had_barrier() { - ASSERT_IS_MAIN_THREAD(); - return proc_had_barrier; -} - -void set_proc_had_barrier(bool flag) { - ASSERT_IS_MAIN_THREAD(); - proc_had_barrier = flag; -} - /// A stack containing the values of is_interactive. Used by proc_push_interactive and /// proc_pop_interactive. static std::vector interactive_stack; diff --git a/src/proc.h b/src/proc.h index c39e1a123..c306a9817 100644 --- a/src/proc.h +++ b/src/proc.h @@ -467,15 +467,6 @@ bool job_list_is_empty(void); /// A helper function to more easily access the job list job_list_t &jobs(); -/// Whether a universal variable barrier roundtrip has already been made for the currently executing -/// command. Such a roundtrip only needs to be done once on a given command, unless a universal -/// variable value is changed. Once this has been done, this variable is set to 1, so that no more -/// roundtrips need to be done. -/// -/// Both setting it to one when it should be zero and the opposite may cause concurrency bugs. -bool get_proc_had_barrier(); -void set_proc_had_barrier(bool flag); - /// The current job control mode. /// /// Must be one of job_control_t::all, job_control_t::interactive and job_control_t::none.