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

Speed up highlighting by about 2x #764

Open
wants to merge 21 commits into
base: master
Choose a base branch
from
Open

Conversation

romkatv
Copy link
Contributor

@romkatv romkatv commented Aug 16, 2020

As the optimization target I measured how long it takes to type a command one character at a time when syntax highlighting is triggered after every character. This matches the most common use case. I used zsh 5.8 with the following .zshrc:

source ~/zsh-syntax-highlighting/zsh-syntax-highlighting.zsh

zmodload zsh/datetime

function bm-zsyh() {
  emulate -L zsh
  local -i n=100
  local f char
  zle -R "$WIDGET: ..."
  local -F start=EPOCHREALTIME
  repeat $n; do
    BUFFER=
    for f in $precmd_functions; do $f; done
    for char in "${(@s::)ZSYH_BM_BUFFER}"; do
      BUFFER+=$char
      _zsh_highlight
    done
  done
  local -F end=EPOCHREALTIME
  BUFFER=
  _zsh_highlight
  local -F2 ms='1e3 * (end - start) / n'
  zle -M "$WIDGET: $ms ms"
}

zle -N bm-zsyh
bindkey '^F' bm-zsyh

To run the benchmark, set ZSYH_BM_BUFFER to the command you want to get typed (e.g., ZSYH_BM_BUFFER='ls abc') and press Ctrl+F.

This benchmark encourages reuse of computation artifacts on successive _zsh_highlight calls when BUFFER changes only slightly.

benchmark buffer old (ms) new (ms) speedup
invalid 136 13.3 +923%
ls abc 31.4 11.8 +166%
ls abc abc abc abc abc abc abc abc abc 270 127 +113%
ls 1 2 3 4 5 6 7 8 9 140 67.2 +108%

In addition to bm-zsyh I also used a slightly modified version that appends characters to the front of BUFFER, as if a user is typing a command backwards, starting from the last character. I'm not showing the results here because they are identical. The optimizations in the PR take advantage of BUFFER changing slightly between calls to _zsh_highlight but they are agnostic about the position and nature of those changes.

This PR has minor positive effect on zsh tests/test-perfs.zsh main: 17.332s with the PR vs 21.064s without. If _zsh_highlight is defined as an empty function, the benchmark reports 9.680s. This implies higher impact of the PR: 7.652s vs 11.384s, a speedup of 49%. I believe this benchmark is less representative of user activity and thus less useful as a target of optimization than the one I presented above.

make test reports no failures.

There is nothing really interesting in the PR. No new algorithms or architectural changes. Just dumb local micro optimizations here and there.

Every commit in the PR has measurable performance benefits compared to the state of the repository to which it's applied.

@romkatv romkatv force-pushed the perf branch 2 times, most recently from f8a64ee to 94a5c86 Compare August 16, 2020 17:48
@romkatv
Copy link
Contributor Author

romkatv commented Aug 16, 2020

I see that travis ci build is failing because my code doesn't work on older versions of zsh (I've only tested it on zsh 5.8). If you are open to accepting the PR, I'll port it to older zsh so that travis ci build passes.

@danielshahaf
Copy link
Member

@romkatv This looks great; thanks a lot!

@phy1729 Could you liaise with Roman regarding getting both this and #758 merged? They're both pretty large diffs and I'd like not to waste anyone's time on manual rebases.

@danielshahaf
Copy link
Member

Milestoning 0.8.0 for review purposes. (Not expressing an opinion about whether we should release 0.8.0 before or after this is merged.)

@danielshahaf danielshahaf added this to the 0.8.0 milestone Aug 17, 2020
@danielshahaf
Copy link
Member

Context for the release timing question: We merged redrawhook last week, which was the main release blocker, and the merge went well, so I for one was in a "let's clear the milestone and release" mindset; and this change and #758, though both very welcome, are also a bit invasive — so I'm not sure whether I'd like to merge them before or after cutting a release. I commented to this effect on #722. If we release first, then we could have an 0.9.0 release not long afterwards, of course. ("Release early and often")

@romkatv Thoughts?

@romkatv
Copy link
Contributor Author

romkatv commented Aug 17, 2020

I don't have an opinion on the matter of release timing. I'll defer to your judgement.

I believe the vast majority of users are following master because an uncountable number articles on medium.com tell them to do it this way. So in practical terms whatever you merge into master ends up being used by some 90+% of users right away.

@danielshahaf
Copy link
Member

danielshahaf commented Aug 17, 2020 via email

@phy1729
Copy link
Member

phy1729 commented Aug 18, 2020

Regarding the #758 conflict the changes are fairly small the diff is just huge. No need to block this on my part; I can rebase.

Looked over a bit of the changes and they look ok. It would be nice to have a style guide documented as well to document which idioms are known to be ideal/non-ideal to help reduce regressions.

@phy1729 phy1729 mentioned this pull request Aug 18, 2020
11 tasks
@romkatv
Copy link
Contributor Author

romkatv commented Aug 20, 2020

I've fixed all issues identified by travis ci build. It's green now.

@danielshahaf
Copy link
Member

Thanks! (But have no time for more than that right now)

@romkatv
Copy link
Contributor Author

romkatv commented Aug 22, 2020

This PR had a regression that caused newly installed commands not being recognized as commands until the user explicitly calls rehash. I've fixed this and added a test.

$1 == [^/]*/* && $zsyh_user_options[pathdirs] == on && -n ${^path}/$1(#q-N.*) ]]; then
REPLY=command
elif (( _zsh_highlight_main__rehash )); then
builtin rehash
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure it's our place to invoke rehash. A library shouldn't change global state unless explicitly asked to. That's just as valid for printing to stdout and setting envvars as it is for populating cmdnamtab.

The new-command.zsh test clarifies the failing use-case. Could you elaborate on what the failure mode was (why that use-case works in master, and what part of the PR broke it)? Also, why run rehash at all? If I do, say, apt install rsync and then type rsync in command position, and it gets highlighted in red, that'll be perfectly correct behaviour (as invoking accept-line at that point will result in an error).

master does call type -w, which does an implicit rehash, but that happens in a subshell.

And, yes, what I wrote above implies that even that subshell shouldn't be doing a rehash, if we can figure a way to implement that. That may be a bug, but if so, it only affects zsh/parameter-less builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new-command.zsh test clarifies the failing use-case. Could you elaborate on what the failure mode was (why that use-case works in master, and what part of the PR broke it)?

In master a newly installed command gets highlighted as "command" (green) but this comes at the cost of two forks. Here's how master figures out the type of zsyh-new-command:

  1. Check $+commands[zsyh-new-command]. It's zero, so no dice.
  2. Check whether builtin type -w -- zsyh-new-command fails in a subshell. It doesn't fail, so no dice.
  3. Run builtin type -w -- zsyh-new-command in a subshell a second time and parse the output to extract "command" from it.

Highlighting of zsyh-new-command gets fast (no forks) only after rehash is invoked by the user.

In the first version of this PR zsyh-new-command was highlighted as "unknown-token" (red) because the algorithm was giving up after checking $+commands[zsyh-new-command]. I mistakenly believed that builtin type -w calls in master were there only for forward compatibility and thus could be avoided when ZSH_VERSION <= 5.8 and zsh/parameter is present.

Thanks to _zsh_highlight_main__command_type_cache, highlighting of zsyh-new-command in master is slow only once per command. The reason I wanted to get rid of forks in my PR is not specfically to speed up highlighting of newly installed commands. My goal was faster highlighting of partially typed commands. For example, when I type systemctl status one character at a time, master forks twice on every character until I get to systemctl. Two forks (or even just one) on every keystroke is something I'd like to avoid.

If I do, say, apt install rsync and then type rsync in command position, and it gets highlighted in red, that'll be perfectly correct behaviour (as invoking accept-line at that point will result in an error).

Not in zsh 5.8 with the default options.

% sudo docker run -e TERM -e LC_ALL=C.UTF-8 -it --rm zshusers/zsh:5.8
# rsync
zsh: command not found: rsync
# apt-get update && apt-get install -y rsync
# rsync
rsync  version 3.1.2  protocol version 31
# print $+commands[rsync]
0

Are there options that make the second rsync command fail with "command not found"?

Tangent: Seems like the meaning of hash_cmds option is reversed. If the commands above are executed with no_hash_cmds, $+commands[rsync] ends up set to 1 at the end.

I'm not sure it's our place to invoke rehash. A library shouldn't change global state unless explicitly asked to.

This is a good point. rehash has the side effect of removing manually hashed commands -- a destructive action.

% hash say=/bin/echo
% say hello
hello
% rehash
% say hello
zsh: command not found: say

I've added a test that verifies that rehash is not called (no-rehash.zsh). It fails with the version of this PR that invokes rehash, and succeeds in master.

I've also added a test (removed-command.zsh) that verifies highlighting of a newly uninstalled command (e.g., after apt remove rsync). Since the command cannot be invoked, it should be highlighted as "unknown-token" (red). This test fails on master and the version of this PR that you've looked at.

I've added a couple more tests that cover edge cases related to hashed commands. My goal was to highlight commands in green if executing them would succeed and in red if executing them would fail. This doesn't always align with the output of type -w. For example:

% hash foo=/not-found
% type -w foo
foo: hashed
% foo
command not found: foo

I believe it's desirable for foo to be highlighted in red here. Let me know if I misunderstood the expected behavior of zsyh.

I've changed main-highlighter.zsh so that all tests pass. It's still as fast as in the very first version of the PR. No forks when using a recent-enough version of zsh. Note that this code now highlights some tokens as "hashed-command" that were previously (mistakenly, I think) highlighted as "command".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My goal was faster highlighting of partially typed commands. For example, when I type systemctl status one character at a time, master forks twice on every character until I get to systemctl. Two forks (or even just one) on every keystroke is something I'd like to avoid.

Makes sense.

Previous discussions about the "command word whilst it's being typed" case: #148, #244.

Are there options that make the second rsync command fail with "command not found"?

No, sorry, I must've been misremembering. I stand corrected.

% sudo docker run -e TERM -e LC_ALL=C.UTF-8 -it --rm zshusers/zsh:5.8
# rsync
zsh: command not found: rsync
# apt-get update && apt-get install -y rsync
# rsync
rsync  version 3.1.2  protocol version 31
# print $+commands[rsync]
0

Tangent: Seems like the meaning of hash_cmds option is reversed. If the commands above are executed with no_hash_cmds, $+commands[rsync] ends up set to 1 at the end.

I see that too:

$ sudo chmod -x /usr/local/bin/caste
$ zsh -f -o nohashcmds
% sudo chmod +x /usr/local/bin/caste
% caste &>/dev/null 
% echo $+commands[caste] 
1

But checking another way, I don't:

$ sudo chmod -x /usr/local/bin/caste
$ zsh -f -o nohashcmds
% sudo chmod +x /usr/local/bin/caste
% caste &>/dev/null
% hash | grep caste
%

I think the 1 output in the NO_HASH_CMDS case is because of the implicit cmdnamtab->filltable() call in the getter when the HASH_LIST_ALL option is set: https://github.com/zsh-users/zsh/blob/zsh-5.8/Src/Modules/parameter.c#L217-L221. However, I can't explain the 0 output in the "default option settings" case. Isn't it a bug?

I don't have a strong opinion regarding whether or not HASH_CMDS should or shouldn't affect ${commands}.

I've added a test that verifies that rehash is not called (no-rehash.zsh). It fails with the version of this PR that invokes rehash, and succeeds in master.

I've also added a test (removed-command.zsh) that verifies highlighting of a newly uninstalled command (e.g., after apt remove rsync). Since the command cannot be invoked, it should be highlighted as "unknown-token" (red). This test fails on master and the version of this PR that you've looked at.

Thanks for the additional tests and bugfixes!

I believe it's desirable for foo to be highlighted in red here. Let me know if I misunderstood the expected behavior of zsyh.

That's an edge case indeed, but I agree it should be highlighted in red.

I've changed main-highlighter.zsh so that all tests pass. It's still as fast as in the very first version of the PR. No forks when using a recent-enough version of zsh. Note that this code now highlights some tokens as "hashed-command" that were previously (mistakenly, I think) highlighted as "command".

Thanks. Looks good, but a few comments:

  1. The log message of the latest commit (6a210f1) isn't clear about which corner cases were fixed. Is it just "the cases that tests are being added for"?

  2. There is one case where the following logic incorrectly sets ← could you please either add an XFail test, or make this a ### TODO: comment, or both?

  3. Please explain the magic number 5.8 in that comment. (I guess it should just say that that's the latest zsh release at the time of writing.)

  4. -n $1(#q-.*N) seems a little roundabout: why not [[ -x $1 ]]? (The answer should be in a code comment, please, rather than here.)

  5. Should _zsh_highlight_main__command_type_cache be emptied when we notice $PATH or ${zsyh_user_options[pathdirs]} has changed? (Preëxisting issue, at least for PATH_DIRS)

  6. Now the PR adds _zsh_highlight_main__rehash and then removes it again. What's your preference — to keep it like that, or to rewrite history (after review finishes, before merging) to avoid the intermediate state? (Honest question.)

  7. What's the difference between the hashed-command test in master and the invalid-hashed-command test in the PR? Seems like they're identical, except that one is XFailing and one is passing? The latest commit seems to have essentially repurposed hashed-command to test a different scenario than it tests in master, relegating the scenario tested in master to another file, and I don't see why.

  8. no-rehash: Cherry-picked in 62c5575. Thanks!

  9. Changing hash sudo=false is correct, but isn't covered by the log message.

Thanks for all the work so far!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw your comments and I appreciate them. I should be able to address them early next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't explain the 0 output in the "default option settings" case. Isn't it a bug?

Looks like a bug.

I don't have a strong opinion regarding whether or not HASH_CMDS should or shouldn't affect ${commands}.

My expectation is that $commands contains hashed commands and only hashed commands. So if HASH_CMDS causes an invoked command to be hashed, $commands should reflect that. I don't have a strong support for this expectation, it's just how I thought it works.

  1. The log message of the latest commit (6a210f1) isn't clear about which corner cases were fixed.

See below.

Is it just "the cases that tests are being added for"?

Yes.

  1. There is one case where the following logic incorrectly sets ← could you please either add an XFail test, or make this a ### TODO: comment, or both?

Done. Added highlighters/main/test-data/ambiguous-hashed-command.zsh.

Note that this isn't a regression. The PR fixes most misclassifications of hashed commands as plain commands but it doesn't fix all of them.

  1. Please explain the magic number 5.8 in that comment. (I guess it should just say that that's the latest zsh release at the time of writing.)

Done.

  1. -n $1(#q-.*N) seems a little roundabout: why not [[ -x $1 ]]? (The answer should be in a code comment, please, rather than here.)

Added a comment.

# [[ -n $1(#q-.*N) ]] is a faster version of [[ -f $1 && -x $1 ]].

The difference in performance is greatest on filesystems with slow stat(). Another reason I've written it this way is consistency with the code a few lines below.

  1. Should _zsh_highlight_main__command_type_cache be emptied when we notice $PATH or ${zsyh_user_options[pathdirs]} has changed? (Preëxisting issue, at least for PATH_DIRS)

_zsh_highlight_main__command_type_cache is emptied on precmd. I think this is reasonable. (I think computing zsyh_user_options only once per command would also be reasonable but it makes no noticeable difference on performance.)

  1. Now the PR adds _zsh_highlight_main__rehash and then removes it again. What's your preference — to keep it like that, or to rewrite history (after review finishes, before merging) to avoid the intermediate state? (Honest question.)

I don't know the standard PR methodology or even if there is one. I usually do it like this:

  1. Work on code changes locally. Commit when I feel like.
  2. Once done, rebase all commits. Split changes into reasonable self-contained chunks so that commits can be reviewed one-by-one.
  3. Once the reviewer posts their first comments, avoid force pushes. Additional changes requested by the reviewer must go in separate commits.
  4. When the reviewer is happy with the code changes, ask them if they prefer the PR to be rebased and whether they have a preference for how it should be split into commits.

I think we are currently at 3 -- discussing the code changes and making changes to it. If you are OK with the current state of the code, let me know whether you want me to rebase the PR into one commit with a long description or a handful of commits with somewhat shorter descriptions. The former is obviously easier for me but I'm fine with the latter, too.

  1. What's the difference between the hashed-command test in master and the invalid-hashed-command test in the PR? Seems like they're identical, except that one is XFailing and one is passing? The latest commit seems to have essentially repurposed hashed-command to test a different scenario than it tests in master, relegating the scenario tested in master to another file, and I don't see why.

It's a trade-off between the simplicity of the final version of the code and the size of the code delta to reach it. I wanted to have two tests for hashed commands: one where the command is valid and another where it's invalid. hashed-command and invalid-hashed-command seemed like good names for these tests, so I grabbed them.

That said, I have a very weak preference here, so if you'd like me to change anything I'll be happy to oblige.

  1. Changing hash sudo=false is correct, but isn't covered by the log message.

Indeed. I realize that commit messages are not up to z-sy-h standards. I wanted to invest only as much time in them as would be necessary for you to review the code. I can write higher-quality commit messages once you are OK with the proposed code changes.

Thanks for all the work so far!

Thanks for the review and great feedback!

Please don't hesitate using more direct language to request changes from me. "Add a comment here", "rename this test", "merge these commits", etc. I'm happy to comply as soon as I understand what's required. By proposing these code changes I'm effectively asking you to maintain them going forward, so it's only reasonable that you request compliance with your own standards.

danielshahaf pushed a commit that referenced this pull request Aug 26, 2020
See comments within for the rationale.

This is a regression test for a regression that was only present in development
versions of PR #764 and was never present in master.
@danielshahaf
Copy link
Member

danielshahaf commented Sep 4, 2020 via email

@phy1729
Copy link
Member

phy1729 commented Sep 6, 2020

With my zsh upstream hat, I recall that Perl has a magic variable _ (without a sigil) exactly for this use-case: in Perl, -f $foo && -x _ is equivalent to -f $foo && -x $foo except that it doesn't call stat() a second time. I wonder if zsh upstream should consider implementing something along these lines.

Relevant zsh code is around https://github.com/zsh-users/zsh/blob/master/Src/cond.c#L341 . Memoizing the dostat call would be trivial; however, -x calls the access syscall with X_OK. The [[ -n $1(#q-.*N) ]] trick has a false positive on

-r-sr-x---  1 root  operator   270K Aug 23 11:28 /sbin/shutdown

whereas [[ -f $1 && -x $1 ]] correctly returns false. I suspect there are additional edge cases around ACLs.

@romkatv
Copy link
Contributor Author

romkatv commented Sep 12, 2020

I've made another performance improvement that affects highlighting of commands with an alias in command position. PTAL.


With my zsh upstream hat, I recall that Perl has a magic variable _ (without a sigil) exactly for this use-case: in Perl, -f $foo && -x _ is equivalent to -f $foo && -x $foo except that it doesn't call stat() a second time. I wonder if zsh upstream should consider implementing something along these lines.

Relevant zsh code is around https://github.com/zsh-users/zsh/blob/master/Src/cond.c#L341 . Memoizing the dostat call would be trivial; however, -x calls the access syscall with X_OK. The [[ -n $1(#q-.*N) ]] trick has a false positive on

-r-sr-x---  1 root  operator   270K Aug 23 11:28 /sbin/shutdown

whereas [[ -f $1 && -x $1 ]] correctly returns false. I suspect there are additional edge cases around ACLs.

Good point. Zsh is also affected by these edge cases.

% zsh -f
% mkdir /tmp/foo
% sudo touch /tmp/foo/bar
% sudo chmod 700 /tmp/foo/bar
% path+=(/tmp/foo)
% echo $commands[bar]
/tmp/foo/bar
% bar
zsh: permission denied: bar
% echo $commands[bar]
/tmp/foo/bar

I believe this is intended behavior. HASH_EXECUTABLES_ONLY fixes it at the cost of additional system calls when hashing directories.

I've fixed the code so that it doesn't get tripped over this corner case. It's now a bit slower but I can measure the difference in performance only on WSL + NTFS, and even then it's fairly small.

Would you report it upstream, please?

Done: workers/47366.

Well, yes, it's not exactly likely that a widget will change $PATH. Nevertheless, that's neither impossible nor forbidden, so could you please record it in a comment or an issue or fix it (whichever is easiest for you)?

Added a comment. Note that this isn't a regression. master also clears _zsh_highlight_main__command_type_cache only in precmd.

Please keep hashed-command's existing name and meaning and name the new test hashed-command-valid.

Done.

In general, I prefer small, self-contained commits. The extra time required to do the rebase before pushing is generally well offset by making reviews and future bisects easier. (As producingoss points out, in general it's advisable for one person to do a little more work if that means O(N) people will each hvae to do a little less work down the road.)

Please confirm whether the code looks good (after the latest changes) and I'll go ahead with rebasing.

@danielshahaf
Copy link
Member

danielshahaf commented Sep 14, 2020 via email

@danielshahaf
Copy link
Member

Sorry about the bad estimate in the last comment; life got ahead of me.

In other news, 62c5575, which I cherry-picked earlier, has been reverted due to breaking tests. @romkatv Could you re-add it to this PR please so it gets in master eventually? Thanks and sorry for the double work.

@romkatv
Copy link
Contributor Author

romkatv commented Oct 15, 2020

Sorry about the bad estimate in the last comment; life got ahead of me.

No worries. As long as there are no large changes in master (which I would have to merge), the delay has no negative impact on me. Take your time.

In other news, 62c5575, which I cherry-picked earlier, has been reverted due to breaking tests. @romkatv Could you re-add it to this PR please so it gets in master eventually?

Done.

The current status of this PR from my point of view: I'm waiting for you to review the code (the whole diff) and either request additional changes or let me know that the code looks good to go. In the latter case I'll split all changes into reasonable-sized commits and force-push.

@vladdoster
Copy link

vladdoster commented Dec 31, 2021

Any movement on this @romkatv, @danielshahaf?

@phy1729 phy1729 modified the milestones: 0.8.0, 0.8.1 Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants