github-mirror/fish-shell
2
0
Fork 0
mirror of https://github.com/fish-shell/fish-shell.git synced 2025-02-02 09:03:59 +08:00
Code Issues Actions Packages Projects Releases Wiki Activity

Another fish var performance improvement

Browse Source 
Make setting fish vars more efficient by avoiding creating a
wcstring_list_t for the case where we're setting one value. For the case
where we're passing a list of values swap it with the list in the var
rather than copying it. This makes the benchmark in #4200 approximately
6% faster.
This commit is contained in:
Kurtis Rader 2017-08-19 15:45:46 -07:00
parent a77cd98136
commit 11400fb313
8 changed files with 112 additions and 113 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
src/builtin_argparse.cpp
View File

@ -645,7 +645,8 @@ static void set_argparse_result_vars(argparse_cmd_opts_t &opts) {
if (!opt_spec->num_seen) continue;
if (opt_spec->short_flag_valid) {
env_set(var_name_prefix + opt_spec->short_flag, ENV_LOCAL, opt_spec->vals);
env_set(var_name_prefix + opt_spec->short_flag, ENV_LOCAL, opt_spec->vals,
opt_spec->long_flag.empty());
}
if (!opt_spec->long_flag.empty()) {
// We do a simple replacement of all non alphanum chars rather than calling

4
src/builtin_set.cpp
View File

@ -284,8 +284,8 @@ static int my_env_path_setup(const wchar_t *cmd, const wchar_t *key, //!OCLINT(
/// Call env_set. If this is a path variable, e.g. PATH, validate the elements. On error, print a
/// description of the problem to stderr.
static int my_env_set(const wchar_t *cmd, const wchar_t *key, int scope,
const wcstring_list_t &list, io_streams_t &streams) {
static int my_env_set(const wchar_t *cmd, const wchar_t *key, int scope, wcstring_list_t &list,
io_streams_t &streams) {
int retval;
if (is_path_variable(key)) {

90
src/env.cpp
View File

@ -1029,17 +1029,18 @@ static env_node_t *env_get_node(const wcstring &key) {
return env;
}
/// Sets the variable with the specified name to a single value.
int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val) {
wcstring_list_t vals({
val,
});
return env_set(key, mode, vals);
}
static int set_umask(const wcstring *single_val, wcstring_list_t *list_val) {
long mask = -1;
if (single_val && !single_val->empty()) {
mask = fish_wcstol(single_val->c_str(), NULL, 8);
} else if (list_val && list_val->size() == 1 && !(*list_val)[0].empty()) {
mask = fish_wcstol((*list_val)[0].c_str(), NULL, 8);
}
/// Sets the variable with the specified name to no (i.e., zero) values.
int env_set_empty(const wcstring &key, env_mode_flags_t mode) {
return env_set(key, mode, wcstring_list_t());
if (errno || mask > 0777 || mask < 0) return ENV_INVALID;
// Do not actually create a umask variable. On env_get() it will be calculated.
umask(mask);
return ENV_OK;
}
/// Set the value of the environment variable whose name matches key to val.
@ -1048,7 +1049,6 @@ int env_set_empty(const wcstring &key, env_mode_flags_t mode) {
/// caller afterwards
///
/// \param key The key
/// \param val The value
/// \param var_mode The type of the variable. Can be any combination of ENV_GLOBAL, ENV_LOCAL,
/// ENV_EXPORT and ENV_USER. If mode is ENV_DEFAULT, the current variable space is searched and the
/// current mode is used. If no current variable with the same name is found, ENV_LOCAL is assumed.
@ -1061,18 +1061,18 @@ int env_set_empty(const wcstring &key, env_mode_flags_t mode) {
/// * ENV_SCOPE, the variable cannot be set in the given scope. This applies to readonly/electric
/// variables set from the local or universal scopes, or set as exported.
/// * ENV_INVALID, the variable value was invalid. This applies only to special variables.
int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_t &vals) {
static int env_set_internal(const wcstring &key, env_mode_flags_t var_mode, bool swap_vals,
const wcstring *single_val, wcstring_list_t *list_val) {
ASSERT_IS_MAIN_THREAD();
bool has_changed_old = vars_stack().has_changed_exported;
int done = 0;
if (!vals.empty() && (key == L"PWD" || key == L"HOME")) {
if (single_val && !single_val->empty() && (key == L"PWD" || key == L"HOME")) {
// Canonicalize our path; if it changes, recurse and try again.
assert(vals.size() == 1);
wcstring val_canonical = vals[0];
wcstring val_canonical = *single_val;
path_make_canonical(val_canonical);
if (vals[0] != val_canonical) {
return env_set_one(key, var_mode, val_canonical);
if (*single_val != val_canonical) {
return env_set_internal(key, var_mode, swap_vals, &val_canonical, nullptr);
}
}
@ -1087,18 +1087,8 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_
return ENV_PERM;
}
if (key == L"umask") {
// Set the new umask.
if (!vals.empty() && vals[0].size()) {
long mask = fish_wcstol(vals[0].c_str(), NULL, 8);
if (!errno && mask <= 0777 && mask >= 0) {
umask(mask);
// Do not actually create a umask variable, on env_get, it will be calculated
// dynamically.
return ENV_OK;
}
}
return ENV_INVALID;
if (key == L"umask") { // set new umask
return set_umask(single_val, list_val);
}
if (var_mode & ENV_UNIVERSAL) {
@ -1115,7 +1105,13 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_
new_export = old_export;
}
if (uvars()) {
uvars()->set(key, vals, new_export);
if (list_val) {
uvars()->set(key, *list_val, new_export);
} else {
wcstring_list_t vals;
if (single_val) vals.push_back(*single_val);
uvars()->set(key, vals, new_export);
}
env_universal_barrier();
if (old_export || new_export) {
vars_stack().mark_changed_exported();
@ -1164,7 +1160,13 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_
exportv = uvars()->get_export(key);
}
uvars()->set(key, vals, exportv);
if (list_val) {
uvars()->set(key, *list_val, exportv);
} else {
wcstring_list_t vals;
if (single_val) vals.push_back(*single_val);
uvars()->set(key, vals, exportv);
}
env_universal_barrier();
done = 1;
@ -1184,7 +1186,15 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_
has_changed_new = true;
}
var.set_vals(vals);
if (single_val) {
var.set_val(*single_val);
} else if (list_val) {
if (swap_vals) {
var.swap_vals(*list_val);
} else {
var.set_vals(*list_val);
}
}
if (var_mode & ENV_EXPORT) {
// The new variable is exported.
var.exportv = true;
@ -1215,6 +1225,22 @@ int env_set(const wcstring &key, env_mode_flags_t var_mode, const wcstring_list_
return ENV_OK;
}
/// Sets the variable with the specified name to the given values. The `vals` will be set to an
/// empty list on return since its content will be swapped into the `env_var_t` that is created.
int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t &vals, bool swap_vals) {
return env_set_internal(key, mode, swap_vals, nullptr, &vals);
}
/// Sets the variable with the specified name to a single value.
int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val) {
return env_set_internal(key, mode, false, &val, nullptr);
}
/// Sets the variable with the specified name without any (i.e., zero) values.
int env_set_empty(const wcstring &key, env_mode_flags_t mode) {
return env_set_internal(key, mode, false, nullptr, nullptr);
}
/// Attempt to remove/free the specified key/value pair from the specified map.
///
/// \return zero if the variable was not found, non-zero otherwise

9
src/env.h
View File

@ -116,12 +116,13 @@ class env_var_t {
const wcstring get_name() const { return name; }
void set_vals(wcstring_list_t &l) { vals = l; }
void swap_vals(wcstring_list_t &l) { vals.swap(l); }
void set_val(const wcstring &s) {
vals = {
s,
};
}
void set_vals(const wcstring_list_t &l) { vals = l; }
env_var_t &operator=(const env_var_t &var) {
this->vals = var.vals;
@ -156,8 +157,10 @@ wcstring_list_t decode_serialized(const wcstring &s);
/// Gets the variable with the specified name, or env_var_t::missing_var if it does not exist.
env_var_t env_get(const wcstring &key, env_mode_flags_t mode = ENV_DEFAULT);
/// Sets the variable with the specified name to the given values.
int env_set(const wcstring &key, env_mode_flags_t mode, const wcstring_list_t &vals);
/// Sets the variable with the specified name to the given values. The `vals` will be set to an
/// empty list on return since its content will be swapped into the `env_var_t` that is created.
int env_set(const wcstring &key, env_mode_flags_t mode, wcstring_list_t &vals,
bool swap_vals = true);
/// Sets the variable with the specified name to a single value.
int env_set_one(const wcstring &key, env_mode_flags_t mode, const wcstring &val);

8
src/env_universal_common.cpp
View File

@ -280,7 +280,7 @@ bool env_universal_t::get_export(const wcstring &name) const {
return result;
}
void env_universal_t::set_internal(const wcstring &key, const wcstring_list_t &vals, bool exportv,
void env_universal_t::set_internal(const wcstring &key, wcstring_list_t &vals, bool exportv,
bool overwrite) {
ASSERT_IS_LOCKED(lock);
if (!overwrite && this->modified.find(key) != this->modified.end()) {
@ -290,7 +290,7 @@ void env_universal_t::set_internal(const wcstring &key, const wcstring_list_t &v
env_var_t &entry = vars[key];
if (entry.exportv != exportv || entry.as_list() != vals) {
entry.set_vals(vals);
entry.swap_vals(vals);
entry.exportv = exportv;
// If we are overwriting, then this is now modified.
@ -300,7 +300,7 @@ void env_universal_t::set_internal(const wcstring &key, const wcstring_list_t &v
}
}
void env_universal_t::set(const wcstring &key, const wcstring_list_t &vals, bool exportv) {
void env_universal_t::set(const wcstring &key, wcstring_list_t &vals, bool exportv) {
scoped_lock locker(lock);
this->set_internal(key, vals, exportv, true /* overwrite */);
}
@ -839,7 +839,7 @@ void env_universal_t::parse_message_internal(const wcstring &msgstr, var_table_t
env_var_t &entry = (*vars)[key];
entry.exportv = exportv;
wcstring_list_t values = decode_serialized(val);
entry.set_vals(values);
entry.swap_vals(values);
}
} else {
debug(1, PARSE_ERR, msg);

5
src/env_universal_common.h
View File

@ -44,8 +44,7 @@ class env_universal_t {
bool load_from_path(const wcstring &path, callback_data_list_t &callbacks);
void load_from_fd(int fd, callback_data_list_t &callbacks);
void set_internal(const wcstring &key, const wcstring_list_t &val, bool exportv,
bool overwrite);
void set_internal(const wcstring &key, wcstring_list_t &val, bool exportv, bool overwrite);
bool remove_internal(const wcstring &name);
// Functions concerned with saving.
@ -78,7 +77,7 @@ class env_universal_t {
bool get_export(const wcstring &name) const;
// Sets a variable.
void set(const wcstring &key, const wcstring_list_t &val, bool exportv);
void set(const wcstring &key, wcstring_list_t &val, bool exportv);
// Removes a variable. Returns true if it was found, false if not.
bool remove(const wcstring &name);

103
src/fish_tests.cpp
View File

@ -2543,11 +2543,10 @@ static int test_universal_helper(int x) {
for (int j = 0; j < UVARS_PER_THREAD; j++) {
const wcstring key = format_string(L"key_%d_%d", x, j);
const wcstring val = format_string(L"val_%d_%d", x, j);
uvars.set(key,
wcstring_list_t({
wcstring_list_t vals({
val,
}),
false);
});
uvars.set(key, vals, false);
bool synced = uvars.sync(callbacks);
if (!synced) {
err(L"Failed to sync universal variables after modification");
@ -2611,77 +2610,47 @@ static void test_universal_callbacks() {
if (system("mkdir -p test/fish_uvars_test/")) err(L"mkdir failed");
env_universal_t uvars1(UVARS_TEST_PATH);
env_universal_t uvars2(UVARS_TEST_PATH);
wcstring_list_t vals;
// Put some variables into both.
uvars1.set(L"alpha",
wcstring_list_t({
L"1",
}),
false);
uvars1.set(L"beta",
wcstring_list_t({
L"1",
}),
false);
uvars1.set(L"delta",
wcstring_list_t({
L"1",
}),
false);
uvars1.set(L"epsilon",
wcstring_list_t({
L"1",
}),
false);
uvars1.set(L"lambda",
wcstring_list_t({
L"1",
}),
false);
uvars1.set(L"kappa",
wcstring_list_t({
L"1",
}),
false);
uvars1.set(L"omicron",
wcstring_list_t({
L"1",
}),
false);
vals.push_back(L"a1");
uvars1.set(L"alpha", vals, false);
assert(vals.size() == 0);
vals.push_back(L"b1");
uvars1.set(L"beta", vals, false);
vals.push_back(L"d1");
uvars1.set(L"delta", vals, false);
vals.push_back(L"e1");
uvars1.set(L"epsilon", vals, false);
vals.push_back(L"l1");
uvars1.set(L"lambda", vals, false);
vals.push_back(L"k1");
uvars1.set(L"kappa", vals, false);
vals.push_back(L"o1");
uvars1.set(L"omicron", vals, false);
uvars1.sync(callbacks);
uvars2.sync(callbacks);
// Change uvars1.
uvars1.set(L"alpha",
wcstring_list_t({
L"2",
}),
false); // changes value
uvars1.set(L"beta",
wcstring_list_t({
L"1",
}),
true); // changes export
uvars1.remove(L"delta"); // erases value
uvars1.set(L"epsilon",
wcstring_list_t({
L"1",
}),
false); // changes nothing
vals.push_back(L"a2");
uvars1.set(L"alpha", vals, false); // changes value
vals.clear(); // clear the old value
vals.push_back(L"b2");
uvars1.set(L"beta", vals, true); // changes export
uvars1.remove(L"delta"); // erases value
vals.clear(); // clear the old value
vals.push_back(L"e1");
uvars1.set(L"epsilon", vals, false); // changes nothing
uvars1.sync(callbacks);
// Change uvars2. It should treat its value as correct and ignore changes from uvars1.
uvars2.set(L"lambda",
wcstring_list_t({
L"1",
}),
false); // same value
uvars2.set(L"kappa",
wcstring_list_t({
L"2",
}),
false); // different value
vals.push_back(L"l2");
uvars2.set(L"lambda", vals, false); // same value
vals.push_back(L"k2");
uvars2.set(L"kappa", vals, false); // different value
// Now see what uvars2 sees.
callbacks.clear();
@ -2694,10 +2663,10 @@ static void test_universal_callbacks() {
do_test(callbacks.size() == 3);
do_test(callbacks.at(0).type == SET);
do_test(callbacks.at(0).key == L"alpha");
do_test(callbacks.at(0).val == L"2");
do_test(callbacks.at(0).val == L"a2");
do_test(callbacks.at(1).type == SET_EXPORT);
do_test(callbacks.at(1).key == L"beta");
do_test(callbacks.at(1).val == L"1");
do_test(callbacks.at(1).val == L"b2");
do_test(callbacks.at(2).type == ERASE);
do_test(callbacks.at(2).key == L"delta");
do_test(callbacks.at(2).val == L"");

3
src/function.cpp
View File

@ -345,6 +345,7 @@ void function_prepare_environment(const wcstring &name, const wchar_t *const *ar
// It should be impossible for the var to be missing since we're inheriting it from an outer
// scope. So we now die horribly if it is missing.
assert(!it->second.missing());
env_set(it->first, ENV_LOCAL | ENV_USER, it->second.as_const_list());
wcstring_list_t vals = it->second.as_const_list(); // we need a copy
env_set(it->first, ENV_LOCAL | ENV_USER, vals); // because this mutates the list
}
}