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

make `abbr` faster; possibly by using a separate var for each abbreviation #4048

Closed
krader1961 opened this Issue May 22, 2017 · 8 comments

Comments

Projects
None yet
3 participants
@krader1961
Copy link
Contributor

krader1961 commented May 22, 2017

@faho and I were chatting on Gitter earlier today after helping someone with the abbr command about how to make it more efficient. I've long felt that putting each abbreviation in a separate variable would be a) faster, and b) simpler to handle. So I created a abbr_new function that does that and compared it to the current abbr function. Setting 100 abbreviations twice in a row (so the second set is a no-op) takes 1.53 seconds on my server (best of five runs) using the current function. My hastily written new version takes 0.42 seconds. Which is 73% faster. Eliminating the duplicate definitions slightly increases the gap to 78% faster. With some more thought and care in writing the new implementation it may be possible to further improve the speed.

This approach also has the advantage that if you just type set (or set --names) the abbreviations stand out. For example, set on my server now includes these vars:

abbr_gs 'git status'
abbr_gsb 'git show-branch'
abbr_h history
abbr_ha 'home attach'
abbr_hd 'home detach'
abbr_hg 'history search --contains'
abbr_hr 'history merge'

My new abbr function also supports -g and -U flags to explicitly set the scope to global or universal with the default being universal. This allows users to explicitly make the abbreviations global in their config.fish and thus not pay the overhead of interacting with the uvar mechanism. Conversely they can type "abbr new expansion" on the fly and have it be immediately visible to the other shells since the default is to make the abbreviation universal.

Doing this properly will require a new string subcommand to encode/decode arbitrary strings to a string that can be used as a variable name. But that is trivial to implement and use. I propose string encode_var $string and string decode_var $string. The encoding will replace any non-alphanum character with an underscore followed by its hex code point value. Since a quick survey of fish abbreviations suggests that less than 10% employ non-alphanum characters in the key this encoding should be seldom needed.

Feedback is encouraged. Obviously there would need to be a transition period of at least one release where the existing var is recognized and updated in parallel with the new scheme since we have documented the $fish_user_abbreviations variable.

@krader1961 krader1961 added this to the fish-future milestone May 22, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented May 22, 2017

I should also mention that the major motivation for this change is that most people expect to be able to define abbreviations in their config.fish script without having to worry about the overhead of uvars. More importantly, because uvars are currently specific to a particular machine you can't just run abbr once to add an abbreviation. You need to put them in your config.fish so that they get defined when you run fish on a new system. Or, for that matter, if you change the definition on one system and want that change to be reflected on all your other systems which share that fish configuration.

@zanchey

This comment has been minimized.

Copy link
Member

zanchey commented May 23, 2017

There's some discussion of the implementation in #731; in particular I suggested scoping as something we could add although that was suggested against at the time (#731 (comment)).

I don't think we should have documented the fish_user_abbreviations implementation, but I'm worried about the churn of changing it. Are there other options? Making abbr a builtin?

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented May 24, 2017

Thanks for the pointer, @zanchy, to the original issue where how abbreviations should work was discussed. The comment on that issue a little under a year ago by ElijahLynn underscores why we need to make it cheap to simply define abbreviations as part of initializing every interactive shell.

I really like that fish makes it very easy to implement many of its behaviors via fish script rather than C++ code. We should be striving to implement as much functionality as possible via fish script. Which is to say I would only support implementing abbr as a builtin as a last resort to solving this problem.

We will definitely annoy some users by replacing $fish_user_abbreviations with an alternative mechanism. However, it should be possible to hide that deprecation by including a function that uses --on-variable to monitor changes to the var and mapping the manipulation to the new scheme I proposed above. So people directly manipulating that var can continue to do so albeit at a slightly increased cost.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jun 12, 2017

I'm going to go ahead and put my change out for review as soon as I implement the necessary string subcommand to convert an arbitrary string to a valid variable name. It is pretty clear we need a cheap way to create abbreviations for every interactive shell by running commands from ~/.config/fish/config.fish. Universal vars are useful but problematic for managing command abbreviations.

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jun 12, 2017

Ignore my previous comment. While we need a cheap way to implement abbreviations I do not think my proposed solution is optimal. Mostly because it should not be necessary to introduce new string subcommands to support this use case. I would prefer to introduce a abbr builtin. But whatever we do we need to make the cost essentially zero for defining abbreviations in someones ~/.config/fish/config.fish script.

@krader1961 krader1961 self-assigned this Jun 19, 2017

@krader1961 krader1961 removed the RFC label Jun 19, 2017

@krader1961 krader1961 modified the milestones: fish 2.7.0, fish-future Jun 19, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jun 21, 2017

I'm flip flopping again 😄 We need a string escape --style=var to fix issue #4147. We need the same feature for this issue if we want to implement abbreviations as distinct vars using fish script. So I'm back to thinking we should implement this using fish script rather than a builtin. As I've noted earlier we should strive to implement as many features as possible using fish script rather than C++ code. Think about the emacs editor and it's LISP based script language. When people want new emacs behavior they don't change the core C/C++ code they write emacs LISP.

@krader1961 krader1961 modified the milestones: fish-3.0, fish 2.7.0 Jun 22, 2017

@krader1961 krader1961 changed the title change `abbr` implementation to use a separate var for each abbreviation make `abbr` faster; possibly by using a separate var for each abbreviation Jun 22, 2017

@krader1961

This comment has been minimized.

Copy link
Contributor

krader1961 commented Jun 27, 2017

Note that we now have the necessary core support to convert arbitrary strings to valid var names: string escape --style=var xxx. This turned out to be necessary for another issue involving git completions. So we can now readily implement my original design. Whether or not we should instead turn abbr into a builtin is TBD. However, there are benefits to continuing to do it via fish functions and it isn't obvious switching to a builtin will be noticeably faster.

krader1961 added a commit that referenced this issue Aug 3, 2017

rewrite `abbr` function
Rewrite the `abbr` function to store each abbreviation in a separate
variable. This greatly improves the efficiency. For the common case
it is 5x faster. For pathological cases it is upwards of 100x faster.
Most people should be able to unconditionally define abbreviations in
their config.fish without a noticable slow down.

Fixes #4048

@krader1961 krader1961 closed this Aug 3, 2017

@maletor

This comment has been minimized.

Copy link

maletor commented Aug 22, 2017

This change looks great. Good work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment