From d8a1928c243b35fd399a56d69ed526f8c2b64a0a Mon Sep 17 00:00:00 2001 From: Mahmoud Al-Qudsi Date: Tue, 3 Apr 2018 14:57:02 -0500 Subject: [PATCH] Convert list of builtins from sorted array to unordered_set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The order of this list does not need to be strictly maintained any longer. Benchmarked with `hyperfine` as follows, where `bench1` is the existing approach of binary search and `bench2` is the new unordered_set code, (executed under bash because fish would always return non-zero). The benchmark code checks each argv to see if it is a builtin keyword (both return the same result): ``` hyperfine './bench1 $(shuf /usr/share/dict/words)' './bench2 $(shuf /usr/share/dict/words)' Benchmark #1: ./bench1 $(shuf /usr/share/dict/words) Time (mean ± σ): 68.4 ms ± 3.0 ms [User: 28.8 ms, System: 38.9 ms] Range (min … max): 60.4 ms … 75.4 ms Benchmark #2: ./bench2 $(shuf /usr/share/dict/words) Time (mean ± σ): 61.4 ms ± 2.3 ms [User: 23.1 ms, System: 39.8 ms] Range (min … max): 58.1 ms … 67.1 ms Summary './bench2 $(shuf /usr/share/dict/words)' ran 1.11x faster than './bench1 $(shuf /usr/share/dict/words)' ``` --- src/builtin.cpp | 50 +++++++++++++++++++------------------------------ src/builtin.h | 20 +++++++++++++++----- 2 files changed, 34 insertions(+), 36 deletions(-) diff --git a/src/builtin.cpp b/src/builtin.cpp index 53484f842..8bf723a68 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -26,6 +26,7 @@ #include #include #include +#include #include "builtin.h" #include "builtin_argparse.h" @@ -75,14 +76,6 @@ #include "wgetopt.h" #include "wutil.h" // IWYU pragma: keep -bool builtin_data_t::operator<(const wcstring &other) const { - return wcscmp(this->name, other.c_str()) < 0; -} - -bool builtin_data_t::operator<(const builtin_data_t *other) const { - return wcscmp(this->name, other->name) < 0; -} - /// Counts the number of arguments in the specified null-terminated array int builtin_count_args(const wchar_t *const *argv) { int argc; @@ -401,12 +394,10 @@ int builtin_false(parser_t &parser, io_streams_t &streams, wchar_t **argv) { // END OF BUILTIN COMMANDS // Below are functions for handling the builtin commands. -// THESE MUST BE SORTED BY NAME! Completion lookup uses binary search. // Data about all the builtin commands in fish. // Functions that are bound to builtin_generic are handled directly by the parser. -// NOTE: These must be kept in sorted order! -static const builtin_data_t builtin_datas[] = { +static const std::unordered_set builtin_datas = { {L"[", &builtin_test, N_(L"Test a condition")}, {L"and", &builtin_generic, N_(L"Execute command if previous command suceeded")}, {L"argparse", &builtin_argparse, N_(L"Parse options in fish script")}, @@ -415,8 +406,7 @@ static const builtin_data_t builtin_datas[] = { {L"bind", &builtin_bind, N_(L"Handle fish key bindings")}, {L"block", &builtin_block, N_(L"Temporarily block delivery of events")}, {L"break", &builtin_break_continue, N_(L"Stop the innermost loop")}, - {L"breakpoint", &builtin_breakpoint, - N_(L"Temporarily halt execution of a script and launch an interactive debug prompt")}, + {L"breakpoint", &builtin_breakpoint, N_(L"Temporarily halt execution of a script and launch an interactive debug prompt")}, {L"builtin", &builtin_builtin, N_(L"Run a builtin command instead of a function")}, {L"case", &builtin_generic, N_(L"Conditionally execute a block of commands")}, {L"cd", &builtin_cd, N_(L"Change working directory")}, @@ -424,8 +414,7 @@ static const builtin_data_t builtin_datas[] = { {L"commandline", &builtin_commandline, N_(L"Set or get the commandline")}, {L"complete", &builtin_complete, N_(L"Edit command specific completions")}, {L"contains", &builtin_contains, N_(L"Search for a specified string in a list")}, - {L"continue", &builtin_break_continue, - N_(L"Skip the rest of the current lap of the innermost loop")}, + {L"continue", &builtin_break_continue, N_(L"Skip the rest of the current lap of the innermost loop")}, {L"count", &builtin_count, N_(L"Count the number of arguments")}, {L"disown", &builtin_disown, N_(L"Remove job from job list")}, {L"echo", &builtin_echo, N_(L"Print arguments")}, @@ -463,8 +452,6 @@ static const builtin_data_t builtin_datas[] = { {L"wait", &builtin_wait, N_(L"Wait for background processes completed")}, {L"while", &builtin_generic, N_(L"Perform a command multiple times")}}; -#define BUILTIN_COUNT (sizeof builtin_datas / sizeof *builtin_datas) - /// Look up a builtin_data_t for a specified builtin /// /// @param name @@ -474,18 +461,19 @@ static const builtin_data_t builtin_datas[] = { /// Pointer to a builtin_data_t /// static const builtin_data_t *builtin_lookup(const wcstring &name) { - const builtin_data_t *array_end = builtin_datas + BUILTIN_COUNT; - const builtin_data_t *found = std::lower_bound(builtin_datas, array_end, name); - if (found != array_end && name == found->name) { - return found; + auto search = builtin_data_t { name }; + auto result = builtin_datas.find(search); + if (result == builtin_datas.end()) { + return NULL; } - return NULL; + + return &*result; } /// Initialize builtin data. void builtin_init() { - for (size_t i = 0; i < BUILTIN_COUNT; i++) { - intern_static(builtin_datas[i].name); + for (auto &bi : builtin_datas) { + intern_static(bi.name.c_str()); } } @@ -529,9 +517,9 @@ int builtin_run(parser_t &parser, const wchar_t *const *argv, io_streams_t &stre /// Returns a list of all builtin names. wcstring_list_t builtin_get_names() { wcstring_list_t result; - result.reserve(BUILTIN_COUNT); - for (size_t i = 0; i < BUILTIN_COUNT; i++) { - result.push_back(builtin_datas[i].name); + result.reserve(builtin_datas.size()); + for (auto &bi : builtin_datas) { + result.push_back(bi.name); } return result; } @@ -539,9 +527,9 @@ wcstring_list_t builtin_get_names() { /// Insert all builtin names into list. void builtin_get_names(std::vector *list) { assert(list != NULL); - list->reserve(list->size() + BUILTIN_COUNT); - for (size_t i = 0; i < BUILTIN_COUNT; i++) { - append_completion(list, builtin_datas[i].name); + list->reserve(list->size() + builtin_datas.size()); + for (auto &bi : builtin_datas) { + append_completion(list, bi.name); } } @@ -550,7 +538,7 @@ wcstring builtin_get_desc(const wcstring &name) { wcstring result; const builtin_data_t *builtin = builtin_lookup(name); if (builtin) { - result = _(builtin->desc); + result = _(builtin->desc.c_str()); } return result; } diff --git a/src/builtin.h b/src/builtin.h index 58434d7fa..9dd03abe8 100644 --- a/src/builtin.h +++ b/src/builtin.h @@ -16,16 +16,26 @@ struct io_streams_t; /// Data structure to describe a builtin. struct builtin_data_t { // Name of the builtin. - const wchar_t *name; - // Function pointer tothe builtin implementation. + const wcstring name; + // Function pointer to the builtin implementation. int (*func)(parser_t &parser, io_streams_t &streams, wchar_t **argv); // Description of what the builtin does. - const wchar_t *desc; + const wcstring desc; - bool operator<(const wcstring &) const; - bool operator<(const builtin_data_t *) const; + bool operator == (const builtin_data_t &other) const { + return name == other.name; + } }; +namespace std { + template<> + struct hash { + std::size_t operator()(const builtin_data_t &data) const { + return std::hash()(data.name); + } + }; +} + /// The default prompt for the read command. #define DEFAULT_READ_PROMPT L"set_color green; echo -n read; set_color normal; echo -n \"> \""