Implementation of ^C prompt cancel handling is laggy

[mqudsi]

With the changes to how ^C is handled when editing the command line buffer in interactive mode, there is a "significant" delay in how long it takes for the prompt to be reset. I've noticed this off and on since 5f849d0 and #2904, but today I had a background build going using all my cores and the delay was really bad, you could see the ^C being painted and then there was a hang until the fish prompt was redrawn and the command line was ready to accept input on the new line.

Obviously my machine was dog-slow at this point, but I was able to use it normally except for the ctrl-c stuff, which means that this is exceptionally slow relative to the rest of fish's interactive mode handling.

The problem is that prior to 5f849d0, \cc was bound to a builtin call (commandline "") which runs pretty much instantaneously. But now, it's bound to a function, which runs, evaluates some things in interpreted code, running several command substitutions then shells out to tput to clear any potential autosuggestions on the line, then shells out to seq, and executes commandline about a dozen times.

Profiling the old behavior with \cc bound to commandline '' shows the following:

390	390	> commandline ""

but now, this is what ^C triggers, as profiled:

343	26220	> __fish_cancel_commandline
382	407	-> source /usr/local/share/fish/functions/__fish_cancel_commandline.fish
25	25	--> function __fish_cancel_commandline
    set -l cmd (commandline)
    if test -n "$cmd"
        commandline -C 1000000
        if set -q fish_color_cancel
            echo -ns (set_color $fish_color_cancel) "^C" (set_color normal)
        else
            echo -ns "^C"
        end
        if command -sq tput
            # Clear to EOL (to erase any autosuggestion).
            echo -n (tput el; or tput ce)
        end
        for i in (seq (commandline -L))
            echo ""
        end
        commandline ""
        commandline -f repaint
    end
...
99	209	-> set -l cmd (commandline)
110	110	--> commandline
303	25261	-> if test -n "$cmd"
        commandline -C 1000000
        if set -q fish_color_cancel
            echo -ns (set_color $fish_color_cancel) "^C" (set_color normal)
        else
            echo -ns "^C"
        end
        if command -sq tput
            # Clear to EOL (to erase any autosuggestion).
            echo -n (tput el; or tput ce)
        end
        for i in (seq (commandline -L))
            echo ""
        end
        commandline ""
        commandline -f repaint
    ...
19	19	--> test -n "$cmd"
249	249	--> commandline -C 1000000
22	81	--> if set -q fish_color_cancel
            echo -ns (set_color $fish_color_cancel) "^C" (set_color normal)
        else
            echo -ns "^C"
        ...
21	21	---> set -q fish_color_cancel
38	38	---> echo -ns "^C"
33	7214	--> if command -sq tput
            # Clear to EOL (to erase any autosuggestion).
            echo -n (tput el; or tput ce)
        ...
513	513	---> command -sq tput
286	6668	---> echo -n (tput el; or tput ce)
6382	6382	----> tput el
284	16910	--> for i in (seq (commandline -L))
            echo ""
        ...
15623	16537	---> seq (commandline -L)
431	854	----> source /usr/local/share/fish/functions/seq.fish
8	423	-----> if not command -sq seq
    # No seq command
    function seq --description "Print sequences of numbers"
        __fish_fallback_seq $argv
    end

    function __fish_fallback_seq --description "Fallback implementation of the seq command"

        set -l from 1
        set -l step 1
        set -l to 1

        switch (count $argv)
            case 1
                set to $argv[1]

            case 2
                set from $argv[1]
                set to $argv[2]

            case 3
                set from $argv[1]
                set step $argv[2]
                set to $argv[3]

            case '*'
                printf (_ "%s: Expected 1, 2 or 3 arguments, got %d\n") seq (count $argv)
                return 1

        end

        for i in $from $step $to
            if not string match -rq '^-?[0-9]*([0-9]*|\.[0-9]+)$' $i
                printf (_ "%s: '%s' is not a number\n") seq $i
                return 1
            end
        end

        if [ $step -ge 0 ]
            echo "for( i=$from; i<=$to ; i+=$step ) i;" | bc
        else
            echo "for( i=$from; i>=$to ; i+=$step ) i;" | bc
        end
    end
...
415	415	------> not command -sq seq
60	60	----> commandline -L
89	89	---> echo ""
454	454	--> commandline ""
31	31	--> commandline -f repaint
40	178	> fish_mode_prompt
57	138	-> fish_default_mode_prompt
10	81	--> if test "$fish_key_bindings" = "fish_vi_key_bindings"
        or test "$fish_key_bindings" = "fish_hybrid_key_bindings"
        switch $fish_bind_mode
            case default
                set_color --bold --background red white
                echo '[N]'
            case insert
                set_color --bold --background green white
                echo '[I]'
            case replace_one
                set_color --bold --background green white
                echo '[R]'
            case visual
                set_color --bold --background magenta white
                echo '[V]'
        end
        set_color normal
        echo -n ' '
    ...
45	45	---> test "$fish_key_bindings" = "fish_vi_key_bindings"
26	26	---> test "$fish_key_bindings" = "fish_hybrid_key_bindings"
37	1174	> fish_prompt
20	20	-> set -l color_cwd
16	16	-> set -l suffix
26	62	-> switch "$USER"
        case root toor
            if set -q fish_color_cwd_root
                set color_cwd $fish_color_cwd_root
            else
                set color_cwd $fish_color_cwd
            end
            set suffix '#'
        case '*'
            set color_cwd $fish_color_cwd
            set suffix '>'
    ...
21	21	--> set color_cwd $fish_color_cwd
15	15	--> set suffix '>'
228	1039	-> echo -n -s "$USER" @ (prompt_hostname) ' ' (set_color $color_cwd) (prompt_pwd) (set_color normal) "$suffix "
24	83	--> prompt_hostname
59	59	---> string replace -r "\..*" "" $hostname
41	41	--> set_color $color_cwd
91	641	--> prompt_pwd
20	20	---> set -l options 'h/help'
26	26	---> argparse -n prompt_pwd --max-args=0 $options -- $argv
6	20	---> if set -q _flag_help
        __fish_print_help prompt_pwd
        return 0
    ...
14	14	----> set -q _flag_help
13	13	---> set -q fish_prompt_pwd_dir_length
15	15	---> set -l fish_prompt_pwd_dir_length 1
53	53	---> set realhome ~
193	273	---> set -l tmp (string replace -r '^'"$realhome"'($|/)' '~$1' $PWD)
80	80	----> string replace -r '^'"$realhome"'($|/)' '~$1' $PWD
25	130	---> if [ $fish_prompt_pwd_dir_length -eq 0 ]
        echo $tmp
    else
        # Shorten to at most $fish_prompt_pwd_dir_length characters per directory
        string replace -ar '(\.?[^/]{'"$fish_prompt_pwd_dir_length"'})[^/]*/' '$1/' $tmp
    ...
42	42	----> [ $fish_prompt_pwd_dir_length -eq 0 ]
63	63	----> string replace -ar '(\.?[^/]{'"$fish_prompt_pwd_dir_length"'})[^/]*/' '$1/' $tmp
46	46	--> set_color normal

The current implementation is 67 times slower and that's without my PC being bogged down with any background tasks that would slow down kernel calls and external executions, followed up with a repeat evaluation of the prompt (and mine is the default, fast prompt) and with all events firing (and this is how it performs with an external seq available).

I know @ridiculousfish wanted to revert this behavior in #4713 for subjective complaints (his and others), but I'm not trying to dig that up again - all I'm saying is that if we keep the current behavior, it needs to be implemented in reader.cpp or as an execution mode for the commandline builtin because the current implementation gives a rather poor experience in an otherwise snappy UI.

[faho]

So...

26220

That's 26ms, which honestly isn't that bad? I mean on my system it's quite a bit faster at 6ms, but this makes me feel like we're missing something. 30fps would be 33ms.

Could you upload the full profile?

---

all I'm saying is that if we keep the current behavior, it needs to be implemented in reader.cpp or as an execution mode for the commandline builtin

I feel like we don't want to rely on scripted implementations as much as we currently do. "Basic" behavior should be c++, so it works faster and better if, for some reason, the functions aren't available.

That includes not just ctrl-c, but also the up/down arrows (type up-or-search).

[faho]

(Also regarding labelling, I'm never really sure what "regression" is supposed to be for. I like it for things that have gotten worse since the last release, where fixing them is higher priority because it brings us back to the status quo. This has been released already.)

[mqudsi]

(That's fine by me; I've always worked at places where regression meant "this was the correct behavior previously, so we have a baseline for what correct looks like.")

I agree with you, but fwiw up-or-search is 100% builtins and shouldn't involve any forking.

[mqudsi]

@faho this is with the system under full load:

84	243267	> __fish_cancel_commandline
99	137	-> set -l cmd (commandline)
38	38	--> commandline
123	243046	-> if test -n "$cmd"
        commandline -C 1000000
        if set -q fish_color_cancel
            echo -ns (set_color $fish_color_cancel) "^C" (set_color normal)
        else
            echo -ns "^C"
        end
        if command -sq tput
            # Clear to EOL (to erase any autosuggestion).
            echo -n (tput el; or tput ce)
        end
        for i in (seq (commandline -L))
            echo ""
        end
        commandline ""
        commandline -f repaint
    ...
28	28	--> test -n "$cmd"
304	304	--> commandline -C 1000000
26	159	--> if set -q fish_color_cancel
            echo -ns (set_color $fish_color_cancel) "^C" (set_color normal)
        else
            echo -ns "^C"
        ...
23	23	---> set -q fish_color_cancel
110	110	---> echo -ns "^C"
45	73549	--> if command -sq tput
            # Clear to EOL (to erase any autosuggestion).
            echo -n (tput el; or tput ce)
        ...
367	367	---> command -sq tput
21285	73137	---> echo -n (tput el; or tput ce)
51852	51852	----> tput el
437	168477	--> for i in (seq (commandline -L))
            echo ""
        ...
150365	150445	---> seq (commandline -L)
80	80	----> commandline -L
17595	17595	---> echo ""
372	372	--> commandline ""
34	34	--> commandline -f repaint
78	243	> fish_mode_prompt
60	165	-> fish_default_mode_prompt
14	105	--> if test "$fish_key_bindings" = "fish_vi_key_bindings"
        or test "$fish_key_bindings" = "fish_hybrid_key_bindings"
        switch $fish_bind_mode
            case default
                set_color --bold --background red white
                echo '[N]'
            case insert
                set_color --bold --background green white
                echo '[I]'
            case replace_one
                set_color --bold --background green white
                echo '[R]'
            case visual
                set_color --bold --background magenta white
                echo '[V]'
        end
        set_color normal
        echo -n ' '
    ...
56	56	---> test "$fish_key_bindings" = "fish_vi_key_bindings"
35	35	---> test "$fish_key_bindings" = "fish_hybrid_key_bindings"
76	1275	> fish_prompt
30	30	-> set -l color_cwd
24	24	-> set -l suffix
60	119	-> switch "$USER"
        case root toor
            if set -q fish_color_cwd_root
                set color_cwd $fish_color_cwd_root
            else
                set color_cwd $fish_color_cwd
            end
            set suffix '#'
        case '*'
            set color_cwd $fish_color_cwd
            set suffix '>'
    ...
31	31	--> set color_cwd $fish_color_cwd
28	28	--> set suffix '>'
284	1026	-> echo -n -s "$USER" @ (prompt_hostname) ' ' (set_color $color_cwd) (prompt_pwd) (set_color normal) "$suffix "
48	137	--> prompt_hostname
89	89	---> string replace -r "\..*" "" $hostname
42	42	--> set_color $color_cwd
76	525	--> prompt_pwd
31	31	---> set -l options 'h/help'
52	52	---> argparse -n prompt_pwd --max-args=0 $options -- $argv
7	29	---> if set -q _flag_help
        __fish_print_help prompt_pwd
        return 0
    ...
22	22	----> set -q _flag_help
22	22	---> set -q fish_prompt_pwd_dir_length
26	26	---> set -l fish_prompt_pwd_dir_length 1
38	38	---> set realhome ~
83	152	---> set -l tmp (string replace -r '^'"$realhome"'($|/)' '~$1' $PWD)
69	69	----> string replace -r '^'"$realhome"'($|/)' '~$1' $PWD
16	99	---> if [ $fish_prompt_pwd_dir_length -eq 0 ]
        echo $tmp
    else
        # Shorten to at most $fish_prompt_pwd_dir_length characters per directory
        string replace -ar '(\.?[^/]{'"$fish_prompt_pwd_dir_length"'})[^/]*/' '$1/' $tmp
    ...
31	31	----> [ $fish_prompt_pwd_dir_length -eq 0 ]
52	52	----> string replace -ar '(\.?[^/]{'"$fish_prompt_pwd_dir_length"'})[^/]*/' '$1/' $tmp
38	38	--> set_color normal
37	277	> fish_title
132	240	-> echo (status current-command) " " (__fish_pwd)
24	24	--> status current-command
44	84	--> __fish_pwd
40	40	---> pwd
46	242	> __fish_disable_bracketed_paste exit
196	196	-> printf "\e[?2004l"
26	331	> fish_title exit
141	305	-> echo (status current-command) " " (__fish_pwd)
19	19	--> status current-command
83	145	--> __fish_pwd
62	62	---> pwd
77	77	> exit
28	88	> __fish_disable_bracketed_paste
60	60	-> printf "\e[?2004l"

Obviously it's not a fair test scenario; everything was slow at that point. Just showing you how obvious it becomes when the system is under load. (Under the same full load test, commandline "" takes 304 microseconds.)

[floam]

Yeah, this simply should have been done in C++. I actually subjectively like it, the way it looks, but all the tput and whatnot to make this work is pretty terrible.

[zanchey]

Should be fixed with #7145 / eb35975