Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

setting variables is slow #4341

krader1961 opened this issue Aug 18, 2017 · 2 comments

setting variables is slow #4341

krader1961 opened this issue Aug 18, 2017 · 2 comments


Copy link

@krader1961 krader1961 commented Aug 18, 2017

I was testing my fix for #4200 and was surprised that the speedup was barely noticeable. So I used the excellent macOS tool instruments to find out why. What I saw was that a synthetic benchmark like the one below causes 25% of the time to be spent in react_to_variable_change() with most of that time being spent in var_is_locale() which is a trivial function that simply tests if the var name is in a vector of locale var names.

I suspect that setting vars like x each time through the for loop is also triggering this code. Which it has to do for correctness since you might be assigning to one of the special vars that we monitor. Which makes it even more critical that code like react_to_variable_change() have as close to zero cost as possible if the var is not being monitored.

Time to rewrite that function to use a dispatch table. That is a map of magic var names to the relevant function to run.

# Test performance of using and manipulating fish vars.
set a1 abc(seq 10)
set a2 xyz(seq 100)
for i in (seq 1000)
    for x in $a1$a2
        # echo $x
set a3
for x in $a1$a2
    set a3 $a3 $x
count $a3
@krader1961 krader1961 added this to the fish-3.0 milestone Aug 18, 2017
@krader1961 krader1961 self-assigned this Aug 18, 2017
Copy link

@faho faho commented Aug 18, 2017

Just switching to std::find from hand-written for-loops speeds that benchmark up from 3.7s to 2.7s for me.

For some weird reason, std::set does not seem better.

diff --git a/src/env.cpp b/src/env.cpp
index 5a4b1b0d..329a31cf 100644
--- a/src/env.cpp
+++ b/src/env.cpp
@@ -617,11 +617,11 @@ static void react_to_variable_change(const wchar_t *op, const wcstring &key) {
     // of loading the universal variables for the first time.
     if (!env_initialized) return;
-    if (var_is_locale(key)) {
+    if (std::find(locale_variables.begin(), locale_variables.end(), key) != locale_variables.end()) {
-    } else if (variable_is_colon_delimited_var(key)) {
+    } else if (std::find(colon_delimited_variable.begin(), colon_delimited_variable.end(), key) != colon_delimited_variable.end()) {
-    } else if (var_is_curses(key)) {
+    } else if (std::find(curses_variables.begin(), curses_variables.end(), key) != curses_variables.end()) {
     } else if (var_is_timezone(key)) {
Copy link
Contributor Author

@krader1961 krader1961 commented Aug 19, 2017

The change I just committed makes my synthetic benchmark 36% above faster (best of five runs with and without the change).

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants