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, fix fish_status_to_signal #8530

Merged
merged 5 commits into from
Dec 11, 2021

Conversation

IlanCosman
Copy link
Contributor

@IlanCosman IlanCosman commented Dec 5, 2021

Description

Some super small changes to fish_status_to_signal that grant a massive speed boost.
Running (count $signals) is slow, and silly considering that we know the number already.
And this list of signals is never, ever going to change, so it won't waste any developer time in the future.

# In config.fish
set -g list (seq 255)

function new_status_to_signal
    for arg in $argv
        if test $arg -gt 128
            set -l sigix (math $arg - 128)
            if test $sigix -le 31
                set -l signals SIGHUP SIGINT SIGQUIT SIGILL SIGTRAP SIGABRT SIGBUS \
                    SIGFPE SIGKILL SIGUSR1 SIGSEGV SIGUSR2 SIGPIPE SIGALRM \
                    SIGTERM SIGSTKFLT SIGCHLD SIGCONT SIGSTOP SIGTSTP \
                    SIGTTIN SIGTTOU SIGURG SIGXCPU SIGXFSZ SIGVTALRM \
                    SIGPROF SIGWINCH SIGIO SIGPWR SIGSYS
                echo $signals[$sigix]
            else
                echo $arg
            end
        else
            echo $arg
        end
    end
    return 0
end
ilan@arch ~> hyperfine --shell=fish "fish_status_to_signal \$list" "new_status_to_signal \$list"
Benchmark 1: fish_status_to_signal $list
  Time (mean ± σ):      23.4 ms ±   3.0 ms    [User: 15.6 ms, System: 10.0 ms]
  Range (min … max):    12.9 ms …  37.4 ms    103 runs
 
Benchmark 2: new_status_to_signal $list
  Time (mean ± σ):      11.7 ms ±   0.9 ms    [User: 7.3 ms, System: 6.0 ms]
  Range (min … max):     6.7 ms …  14.5 ms    177 runs
 
Summary
  'new_status_to_signal $list' ran
    1.99 ± 0.30 times faster than 'fish_status_to_signal $list'

@floam
Copy link
Member

floam commented Dec 5, 2021

Yikes! This speedup is nice but this function is no bueno! For starters, SIGBUS (10, not 7) comes after SIGKILL and before SIGSEGV on my system.

signal 7 is SIGEMT here. This hardcoded list idea is trouble.

@floam
Copy link
Member

floam commented Dec 5, 2021

Would you like to also fix that while you are improving this function? We actually cache these - see the completions for kill/killall. What you want to do is check for $__kill_signals otherwise run __fish_make_completion_signals.

@IlanCosman
Copy link
Contributor Author

Can do 👍

@IlanCosman
Copy link
Contributor Author

Should be good now, hope I did things right 😄

@floam floam changed the title Speed up fish_status_to_signal Speed up, fix fish_status_to_signal Dec 6, 2021
@floam floam merged commit 521eebe into fish-shell:master Dec 11, 2021
@krobelus krobelus added this to the fish 3.4.0 milestone Dec 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants