Avoid forks to improve performance (especially on Cygwin)

[m0vie]

This pull-request improves z-sy-h performance by avoiding all forks.

Especially on Cygwin the improvement is extreme.

The only drawback so far seems to be that "hashed" commands cannot be distinguished from normal "commands" anymore -- which I think is very minor.

Anyways, this is more of a proof-of-concept.

[danielshahaf]

By and large looks good.

_zsh_highlight_command_type_cache is populated in the main highlighter but cleared in the driver; I guess it should be cleared in the main highlighter, never used in the driver, and accordingly renamed _zsh_highlight_main_…?

I've merged the first commit. I have some specific review questions on the rest, but first:

Why do you say it's a WIP/PoC? Other than not distinguishing hashed commands from commands?

[danielshahaf]

I've merged the first commit.

It's now 38c8fbe.

[m0vie]

_zsh_highlight_command_type_cache is populated in the main highlighter but cleared in the driver; I guess it should be cleared in the main highlighter, never used in the driver, and accordingly renamed zsh_highlight_main…?

That would be nicer, yes. But right now there the there is no api function for the individual highlighters called from _zsh_highlight_preexec_hook.

Why do you say it's a WIP/PoC? Other than not distinguishing hashed commands from commands?

This was all done very quickly. But it seems to work a lot better and faster than I originally expected.
I've been using this for a few weeks now without any problems.
I guess the WIP prefix can be removed. ;-)

[danielshahaf]

I guess the WIP prefix can be removed. ;-)

Done. Review:

• First commit: typo in comment (parameters → parameter). Style nitpick: perhaps append a || true to be clear that an error is expected and ignored.
• Second commit: s/local local/local/. The incumbent code makes the assumption that the value of an alias is a command name, which isn't true in general; your patch extends that assumption to the helper's name and docstring. It would be nice to fix this, but this won't block merging the PR.
• Third commit: I propose to test ${+commands[…]}, since even taking PATH_DIRS into account, the check can false negative but can't false positive; and to fall back to REPLY=$(type -w …), which will cover both PATH_DIRS and possible future "new types". (There's no need for the none case, then; it will also handle hashed commands correctly.) I realize this will require a fork if the command word has a slash, would that be a problem?
• The use of (e) in associative array keys is harmless but not required since the keys are parameter expansions.

edited: Stroke out incorrect statement

[danielshahaf]

Shouldn't $+saliases[(e)${1#*.}] be $+saliases[(e)${1##*.}]?

% alias -s foo.bar=deep bar=shallow 
% type 1.foo.bar 
bar is a suffix alias for shallow

[danielshahaf]

I'd like to merge this and #301 next.

[danielshahaf]

This now conflicts due to 0893296 / 341a3ae.

[m0vie]

First commit: typo in comment (parameters → parameter). Style nitpick: perhaps append a || true to be clear that an error is expected and ignored.

Done.

Second commit: s/local local/local/. The incumbent code makes the assumption that the value of an alias is a command name, which isn't true in general; your patch extends that assumption to the helper's name and docstring. It would be nice to fix this, but this won't block merging the PR.

Done.

Third commit: I propose to test ${+commands[…]}, since even taking PATH_DIRS into account, the check can false negative but can't false positive; and to fall back to REPLY=$(type -w …), which will cover both PATH_DIRS and possible future "new types".

I'm not a 100% sure where we left off in the IRC discussion. Do you want to go forward with the future-proof approach of using if type -w; then REPLY=$(type -w …), or merge it like it is now? Actually, even then we wouldn't be future-proof since we still had to change the script anyways to actually support potential new types in different highlighting. I'd prefer it this way for the consequent "no forks. ever.".

The use of (e) in associative array keys is harmless but not required since the keys are parameter expansions.

I think it's best to leave them in, to be on the safe side.

Shouldn't $+saliases[(e)${1#.}] be $+saliases[(e)${1##.}]?

Right. Fixed.

[danielshahaf]

Third commit: I propose to test ${+commands[…]}, since even taking PATH_DIRS into account, the check can false negative but can't false positive; and to fall back to REPLY=$(type -w …), which will cover both PATH_DIRS and possible future "new types".

I'm not a 100% sure where we left off in the IRC discussion. Do you want to go forward with the future-proof approach of using if type -w; then REPLY=$(type -w …), or merge it like it is now? Actually, even then we wouldn't be future-proof since we still had to change the script anyways to actually support potential new types in different highlighting. I'd prefer it this way for the consequent "no forks. ever.".

First of all, let me post here the patch I devised during our IRC discussion:

--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -96,6 +96,7 @@ _zsh_highlight_main__type() {
   if (( $#options_to_set )); then
     setopt localoptions $options_to_set;
   fi
+  unset REPLY
   if zmodload -e zsh/parameter; then
     if (( $+aliases[(e)$1] )); then
       REPLY=alias
@@ -107,12 +108,11 @@ _zsh_highlight_main__type() {
       REPLY=function
     elif (( $+builtins[(e)$1] )); then
       REPLY=builtin
-    elif builtin type -w $1 >/dev/null 2>&1; then
+    elif (( $+commands[(e)$1] )); then
       REPLY=command
-    else
-      REPLY=none
     fi
-  else
+  fi
+  if ! (( $+REPLY )); then
     REPLY="${$(LC_ALL=C builtin type -w -- $1 2>/dev/null)#*: }"
   fi
   _zsh_highlight_command_type_cache[(e)$1]=$REPLY

Now, "no forks ever" seems to me like spilling the baby with the bathwater: on most platforms, forks aren't expensive, so a single fork in a rare codepath would be preferable to giving an actively wrong answer (not just "I don't know" instead of "{new-in-5.3 thing}", but "{command}" instead of "{new-in-5.3 thing}"). The point about future-proofness is a legitimate one but it can be handled by changing the last assignment to $REPLY in the above diff to REPLY=typedashwoutput-"${$(LC_ALL=C …)}", i.e., by namespacing the output of type -w to avoid style name clashes.

Keep in mind that there is an open feature request to make the command word as it is being typed a different color than green and red if it is the prefix of a valid command word; that can be implemented without forks (just by iterating once over the zsh/parameter hashes) and should make the fork a rare codepath.

So, while I would listen to an argument that the fork should be avoided on cygwin or made opt-outable, I think the default everywhere else should be to try $(type -w) before giving up, and even on cygwin I'd like to avoid confidently claiming "This word is a command" when that might not be the case.

The use of (e) in associative array keys is harmless but not required since the keys are parameter expansions.

I think it's best to leave them in, to be on the safe side.

+1

[danielshahaf]

@m0vie points out that current master doesn't map "type -w returns 0 but doesn't output any of the known strings" correctly. Something like this could handle it:

diff --git a/highlighters/main/main-highlighter.zsh b/highlighters/main/main-highlighter.zsh
index a0f8dba..9247f68 100644
--- a/highlighters/main/main-highlighter.zsh
+++ b/highlighters/main/main-highlighter.zsh
@@ -330,7 +330,8 @@ _zsh_highlight_main_highlighter()
         *': function')  style=function;;
         *': command')   style=command;;
         *': hashed')    style=hashed-command;;
-        *)              if _zsh_highlight_main_highlighter_check_assign; then
+        *)              if [[ $res != *': none' ]] ; then style=NEWTHING ; else 
+                        if _zsh_highlight_main_highlighter_check_assign; then
                           style=assign
                           if [[ $arg[-1] == '(' ]]; then
                             in_array_assignment=true
@@ -383,6 +384,7 @@ _zsh_highlight_main_highlighter()
                             style=unknown-token
                           fi
                         fi
+                        fi
                         ;;
       esac
      fi

But what should this case funnel to? What should NEWTHING be? [command], [unknown-token], [new-command-type-from-the-future]?

edit: This is now tracked separately as #316.

[danielshahaf]

Review of latest version (cb99b23):

• Fourth (ed003db):
  1. Log message typo zsh/paramter (twice)
  2. The test does not "issue a warning". The test used to PASS and is now an XFAIL (expected failure). Please use correct terminology.
  3. The precise fallback order of _zsh_highlight_main_add_region_highlight commandtypefromthefuture-$res commandtypefromthefuture command still needs to be decided. (This is a design issue wider than just this patch; it can be decided post-merging too, so long as it's decided before release.)
• Fifth (cb99b23):

  1. What is the purpose of the cache? What use-case does it optimize? I believe its design purpose is to fast-path the "Unrecognised command word" case, to avoid forking $(type -w) once per highlighting pass. (It would also help systems where command substitutions (fork()) are expensive and zsh/parameter is unavailable.)

     What use-cases will generate large numbers of cache hits? That is: How common is it to have an unrecognised command word passed to type -w more than once? When that happens, what is the performance cost of the second-and-beyond $(type -w) calls (which the cache seeks to avoid)?

     How common will it be for a unrecognised command word to be passed to type -w after [#148] command being typed as neither green nor red #244 is merged?

  2. Shouldn't the clear-screen widget clear the cache? (Suppose somebody types rsync foo and the command name shows in red, so they install rsync in a different terminal, and then press Ctrl+L with the expectation that rsync will be recolored green.)

I'm sorry for not realizing these command type cache questions earlier, but in any case, I think we can merge the first four commits (with the log message fixes), close this issue, and open two spin-off issues for (a) finalizing the fallback order, (b) adding the command cache.

So, let me know once you've fixup'd the fourth? Or I can do that if you prefer.

[danielshahaf]

I've merged the first two commits in f146651. I haven't rebased them but I did change the "area:" leader of the log messages to match convention.

[m0vie]

Log message typo zsh/paramter (twice)

Done.

The test does not "issue a warning"

Done.

What is the purpose of the cache?

The cache is actually quite an improvement. Consider long lines such as:

somecommandthatdoesnotexist foo bar; you keep typing here...

That would result in an execution of

_zsh_highlight_main__type somecommandthatdoesnotexist

every time you type a letter, each time calling type -w, which does a full PATH search. I don't have exact numbers right now, but the cache was a HUGE improvement on cygwin, and also very noticeable on native Linux when the BUFFER was large.

Shouldn't the clear-screen widget clear the cache? (Suppose somebody types rsync foo and the command name shows in red, so they install rsync in a different terminal, and then press Ctrl+L with the expectation that rsync will be recolored green.)

Even zsh so won't notice that there are new commands available instantly:
Shell 1: type foo
Shell 2: create foo (+x) in PATH
Shell 1: hit RETURN (also after ^L):

`zsh: correct 'foo' to 'fop' [nyae]?``

So IMHO that's not a big deal since it never worked before either.

... and open two spin-off issues for (a) finalizing the fallback order ... (b) adding the command cache

Yes, sounds good to me. But I'd really like the get both commits merged first. The details we have to think about are very minor and it's probably easier to do built on top of master.

[danielshahaf]

That would result in an execution of

_zsh_highlight_main__type somecommandthatdoesnotexist

every time you type a letter

Yes, I realized this was the use-case that would most benefit from the cache; I just wondered how often it occurred in practice.

Even zsh so won't notice that there are new commands available instantly:

Good point: there's no reason to clear the cache more often than every time rehash runs... which by default is never, so no need to hook to clear-screen.

Yes, sounds good to me. But I'd really like the get both commits merged. The details we have to think about are very minor and it's probably easier to do built on top of master.

Okay, so let's merge it without any fallback now (just ..._add_highlight commandfromthefuture-$res). This doesn't actually matter, but it'll probably make the future discussions about fallback order easier.

Thanks!

[m0vie]

Okay, so let's merge it without any fallback now (just ..._add_highlight commandfromthefuture-$res)

Okay. changed.

[danielshahaf]

Merged 12b879c and its parent. That left two outstanding questions:

1. Fallback order Forward compatibility: highlight unknown type -w outputs correctly #316 (comment)
2. The command type cache (currently 957a8b4) hasn't been merged yet

[m0vie]

command type cache is tracked in #320.

[psprint]

I've compared speeds:

./parse.zsh parse_bash.zsh > 2  2,10s user 0,58s system 92% cpu 2,905 total

vs

./parse.zsh parse_bash.zsh > 1  1,93s user 0,16s system 99% cpu 2,077 total

So almost 0.2 sec. Seems not much, but everything feels to be much faster now.

[danielshahaf]

So almost 0.2 sec.

Shouldn't you include system time in your count? The whole purpose of the PR was to eliminate syscalls, and users care about wallclock time, not about userland time (despite the name of the latter).

Seems not much, but everything feels to be much faster now.

:-)

[psprint]

Shouldn't you include system time in your count? The whole purpose of the PR was to eliminate syscalls, and users care about wallclock time, not about userland time (despite the name of the latter).

True, total time counts, so there is significant gain almost 0.9 sec

PS. As for the why did I look at user time: it corresponds closely to what zprof reports. I once did vast amount of tests with zprof, and also with time ( ), and user time was closely the same as total running time of main function in zprof.