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.

Sign up for GitHub

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

Jump to bottom

perf: speed improvements #1441

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

perf: speed improvements #1441

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
417c122
feat: first pass at improving speed, mostly for list and current comm…
stephanGarland Jan 15, 2023
009ab6e
feat: added a fast_tr command, did some more grouping of commands whe…
stephanGarland Jan 20, 2023
f410204
fix: using native bash versinfo array to check versions; fixed a bash…
stephanGarland Jan 25, 2023
f585713
feat: changed fast_basename to use REPLY
stephanGarland Jan 25, 2023
0084332
feat: changed fast_tr to use REPLY; eliminated an unnecessary tr call
stephanGarland Jan 25, 2023
3d86e29
feat: minor changes to two functions; added comment for
stephanGarland Jan 25, 2023
1a6d8b9
fix: identified and fixed bug with parse_asdf_version_file that was a…
stephanGarland Feb 10, 2023
08beda0
chore: merge branch 'master' into develop
stephanGarland Feb 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 60 additions & 17 deletions lib/utils.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,45 @@ GREP_COLORS=

ASDF_DIR=${ASDF_DIR:-''}
ASDF_DATA_DIR=${ASDF_DATA_DIR:-''}
BASH_MAJOR_VERSION=${BASH_VERSION%%.*}

fast_basename() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be changed to:?

local path="$1"
path=${path%/}
path=${path##*/}
printf '%s\n' "$REPLY"

The extra tests seem redundant as Bash should perform them internally.

I think it's very worth pursuing having a REPLY-like pattern as shown here to completely eliminate the subshell (I strongly prefer the variable being called REPLY, though). In my experience this gives gigantic perf wins, something similar as what it seems you have alluded to in your other comment. I'm not sure how on-board the core-team is with this, though, which is why I think more discussion is needed.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed that the additional subshells can be removed, ty for this recommendation.

Re: $REPLY, I thought that was specifically for read? Using your code sample as-is resulted in an empty output for me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah it is for read and also select, I'm borrowing the convention (more info here). It will work if the function looks something like this:

fast_basename() {
  unset -v REPLY; REPLY=

  local path="$1"
  path=${path%/}
  path=${path##*/}
  REPLY=$path
}

And is called like this:

fast_basename "$executable_path"
local cmd="$REPLY"

The code sample I provided was for existing subshell code, not the REPLY-thing.

Copy link
Author

Choose a reason for hiding this comment

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

EDIT: Ah, there are times like this that would likely be sped up with the elimination of the subshell.

I ran the two fast_basename types (REPLY and printf) with hyperfine on my M1 Air. I also tried this on a Debian server, but generally was unable to get hyperfine to not issue warnings about interference, no matter the combinations tried. Regardless, the timings were always incredibly close. In this example, printf had a slightly higher max, but was otherwise identical.

❯ cat fast_base*.sh
#!/usr/bin/env bash

fast_basename() {
  unset dirpath
  local dirpath="$1"
  dirpath=${dirpath%/}
  dirpath=${dirpath##*/}
  printf "%s\n" $dirpath
}

fast_basename "$1"
#!/usr/bin/env bash

fast_basename() {
  unset -v REPLY; REPLY=
  local path="$1"
  path=${path%/}
  path=${path##*/}
  REPLY=$path
}

fast_basename "$1"

❯ for f in fast_*.sh; do hyperfine -N --warmup 1000 --runs 1000 "./$f $PWD"; done
Benchmark 1: ./fast_basename_printf.sh /Users/sgarland
  Time (mean ± σ):       2.7 ms ±   0.4 ms    [User: 1.3 ms, System: 0.9 ms]
  Range (min … max):     2.5 ms …   5.2 ms    1000 runs

  Warning: The first benchmarking run for this command was significantly slower than the rest (3.4 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.

Benchmark 1: ./fast_basename_reply.sh /Users/sgarland
  Time (mean ± σ):       2.7 ms ±   0.4 ms    [User: 1.3 ms, System: 0.9 ms]
  Range (min … max):     2.5 ms …   5.1 ms    1000 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Re: the best practices linked, I don't understand the reasoning for some of them.

unset REPLY; REPLY=

If you unset a var (or function), it's gone. In C you may need to then initialize the empty var to a known value, but this isn't C.

unset i

If it's function-scoped, why bother unsetting it? This is always going to print a null for the first echo, and 10 for the second:

function foo() {
  echo $i
  local i=
  for _ in {0..9}; do 
    ((i=i+1))
  done
  echo $i
}

In any case, much of this is quibbling over minute differences, and I'm not that much of a stickler. I care about improving the core performance, and it's clear that you do as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

For perf test, you have to include the capture of the variable as a part of the test. So I'm not sure what you're trying to measure when you use printf without a subshell. These were my results:

script.sh 
#!/usr/bin/env bash

fast_basename_work() {
	local path="$1"
	path=${path%/}
	printf '%s\n' "{path##*/}"
}

slow_basename() {
	unset -v REPLY
	REPLY=$(basename "$1")
}

fast_basename() {
	unset -v REPLY
	REPLY=$(fast_basename_work "$1")
}

faster_basename() {
	unset -v REPLY; REPLY=
	local path="$1"
	path=${path%/}
	path=${path##*/}
	REPLY=$path
}

main() {
	"$1" "$PWD"
	local _result="$REPLY"
}

main "$@"
> for fn in slow_basename fast_basename faster_basename; do hyperfine -N --warmup 1000 --runs 1000 "./script.sh $fn"; done
Benchmark 1: ./script.sh slow_basename
  Time (mean ± σ):       3.6 ms ±   0.2 ms    [User: 2.0 ms, System: 1.4 ms]
  Range (min … max):     3.4 ms …   8.8 ms    1000 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 1: ./script.sh fast_basename
  Time (mean ± σ):       3.2 ms ±   0.4 ms    [User: 1.6 ms, System: 1.4 ms]
  Range (min … max):     2.9 ms …  15.1 ms    1000 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 1: ./script.sh faster_basename
  Time (mean ± σ):       2.7 ms ±   0.3 ms    [User: 1.3 ms, System: 1.2 ms]
  Range (min … max):     2.5 ms …   8.9 ms    1000 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

For your questions about my self-defined "best practices"

If you unset a var (or function), it's gone. In C you may need to then initialize the empty var to a known value, but this isn't C.

The REPLY= is to be absolutely certain that REPLY is what you think it is. For a single function, this doesn't do anything, but if you have hundreds of functions that set REPLY, this ensures that artifacts of running those other functions don't accidentally interfere with the current function.

The unset -v is necessary because we want to be absolutely sure that the variable is of the correct type - it's fundamentally more correct. See below:

script.sh 
# shellcheck shell=bash

fn1() {
	unset -v REPLY
	REPLY=

	printf 'fn1: %s\n' "$(declare -p REPLY)"
	REPLY=(a b)
}

fn2() {
	# unset -v REPLY
	REPLY=

	printf 'fn2: %s\n' "$(declare -p REPLY)"
	REPLY=c
}

main() {
	fn1
	local -a result1="${REPLY[@]}"

	fn2
	local result2="$REPLY"
}

main "$@"

And I can't just do unset -v REPLY because for some reason, people like to enable errunset, and since I'm usually writing Bash libraries, I need to be compatible for the options that they wish to set.

If it's function-scoped, why bother unsetting it? This is always going to print a null for the first echo, and 10 for the second:

Yeah, it's function scoped, and I still don't want that variable to leak into the callers of functions that are called later in that function like so:

otherfn() {
  local i=
  for i in {0..1}; do :; done

  foo
}

So by doing unset -v i, the goal is to have a behavior closer to traditionally scoped variables. Bash variables (with local) are function scoped, but more importantly, they are also dynamically scoped, and I wish to account for that.

But yeah, these are minor differences, and I don't wish to prescribe all of these conventions on the ASDF codebase - just the one that actually significantly improves performance.

Copy link
Author

Choose a reason for hiding this comment

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

So I'm not sure what you're trying to measure when you use printf without a subshell.

In the light of day, I also have no idea what I was doing there. Good catch.

And I can't just do unset -v REPLY because for some reason, people like to enable errunset, and since I'm usually writing Bash libraries, I need to be compatible for the options that they wish to set.

An excellent point I hadn't thought of. People love to set -euo pipefail because someone said it was better.

Yeah, it's function scoped, and I still don't want that variable to leak into the callers of functions that are called later in that function like so

Again, an excellent point I hadn't thought of.

I think I'm just going to defer to your recommendations. I thought I knew shell reasonably well, but clearly I've a lot more to learn. Thank you for taking the time to demonstrate your answers with scripts.

# if the path ends in a slash, remove it, then use parameter substitution
# if it doesn't, use parameter substitution on the path as-is
dirpath="$1"
[[ "$dirpath" == */ ]] && dirpath="${dirpath%/}" && dirpath="${dirpath##*/}" || dirpath="${dirpath##*/}"
printf "$dirpath"
Copy link
Contributor

Choose a reason for hiding this comment

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

This, and other invocations to printf should have %s\n as a first parameter to prevent unintentional formatting.

EDIT: I should have said %s\n since that makes debugging/developing these functions easier - and subshells, process substitution, herestrings automatically append newlines anywyas.

}

fast_tr() {
inputArr=("$@")
inputStr="${inputArr[0]}"
local outputStr
if [ ! ${#inputArr[@]} -eq 3 ] && [ ! ${#inputArr[@]} -eq 5 ]; then
printf "%s\n" "ERROR: fast_tr expects an array of 3 or 5 elements, got ${#inputArr[@]}"
return 1
fi
# if bash >= v4, use parameter modification to upper/lower the string
if [ ${BASH_MAJOR_VERSION} -gt 3 ]; then
case ${inputArr[1]} in
"[:lower:]")
outputStr="${inputStr@U}"
;;
"[:upper:]")
outputStr="${inputStr@L}"
Copy link
Contributor

Choose a reason for hiding this comment

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

This breaks since @U and @L were added in bash 5.0 while ,, and ^^ were added in bash 4.0. But the check is still good in principle because we support 3.2 at a minimum.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, good catch. I originally had the ^^ syntax but then found out about @U and used it since it seemed more intuitive. I didn't check the version requirements for it, though.

;;
"*")
printf "ERROR: fast_tr() expects [:lower:] and [:upper:] as arguments"
esac
# and then use parameter substitution to replace characters in the string
# if the 4th or 5th elements are not set, the substitution has no effect
stephanGarland marked this conversation as resolved.
Show resolved Hide resolved
outputStr="${outputStr//${inputArr[3]-''}/${inputArr[4]-''}}"
printf "${outputStr}"
else
# else use tr with the input array elements as args
printf "${inputStr}" | tr "${inputArr[1]}${inputArr[3]-''}" "${inputArr[2]}${inputArr[4]-''}"
fi
}

asdf_version() {
local version git_rev
Expand Down Expand Up @@ -99,7 +138,7 @@ list_installed_versions() {
if [ -d "$plugin_installs_path" ]; then
for install in "${plugin_installs_path}"/*/; do
[[ -e "$install" ]] || break
basename "$install" | sed 's/^ref-/ref:/'
fast_basename "$install"
done
fi
}
Expand Down Expand Up @@ -190,7 +229,8 @@ find_versions() {
version=$(get_version_from_env "$plugin_name")
if [ -n "$version" ]; then
local upcase_name
upcase_name=$(printf "%s\n" "$plugin_name" | tr '[:lower:]-' '[:upper:]_')
tr_args=("$plugin_name" "[:lower:]" "[:upper:]" "-" "_")
upcase_name=$(fast_tr ${tr_args[@]})
Copy link
Author

Choose a reason for hiding this comment

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

Due to the subshell this is at best equally as fast as before. Probably a way to do this without a subshell.

local version_env_var="ASDF_${upcase_name}_VERSION"

printf "%s\n" "$version|$version_env_var environment variable"
Expand Down Expand Up @@ -280,7 +320,7 @@ get_custom_executable_path() {

# custom plugin hook for executable path
if [ -x "${plugin_path}/bin/exec-path" ]; then
cmd=$(basename "$executable_path")
cmd=$(fast_basename "$executable_path")
local relative_path
# shellcheck disable=SC2001
relative_path=$(printf "%s\n" "$executable_path" | sed -e "s|${install_path}/||")
Expand All @@ -300,7 +340,7 @@ get_executable_path() {

if [ "$version" = "system" ]; then
path=$(remove_path_from_path "$PATH" "$(asdf_data_dir)/shims")
cmd=$(basename "$executable_path")
cmd=$(fast_basename "$executable_path")
cmd_path=$(PATH=$path command -v "$cmd" 2>&1)
# shellcheck disable=SC2181
if [ $? -ne 0 ]; then
Expand All @@ -320,8 +360,8 @@ parse_asdf_version_file() {

if [ -f "$file_path" ]; then
local version
version=$(strip_tool_version_comments "$file_path" | grep "^${plugin_name} " | sed -e "s/^${plugin_name} //")

# this is actually only about 5% faster than the original, despite losing a function call and two pipes
version=$(awk -v plugin_name="$plugin_name" '!/^#/ { gsub(/#.*/,"",$0); gsub(/ +$/,"",$0)} $1 == plugin_name {print $NF}' "$file_path")
if [ -n "$version" ]; then
if [[ "$version" == path:* ]]; then
util_resolve_user_path "${version#path:}"
Expand Down Expand Up @@ -634,11 +674,15 @@ plugin_shims() {

shim_plugin_versions() {
local executable_name
executable_name=$(basename "$1")
# not sure why this needs a basename
executable_name=$(fast_basename "$1")
local shim_path
shim_path="$(asdf_data_dir)/shims/${executable_name}"
if [ -x "$shim_path" ]; then
grep "# asdf-plugin: " "$shim_path" 2>/dev/null | sed -e "s/# asdf-plugin: //" | uniq
# not sure why uniq was needed here since asdf doesn't let you install the same plugin version twice - maybe for legacy?
# nevertheless, the awk command handles de-duping; if it isn't needed, the sed command is ~3% faster
#sed -n "/# asdf-plugin: /s/# asdf-plugin: //p 2>/dev/null" "$shim_path"
awk '/# asdf-plugin: / { l=$(NF-1) " " $NF; !seen[$0]++; if (seen[$0] < 2) { print l } }' "$shim_path" 2>/dev/null
else
printf "asdf: unknown shim %s\n" "$executable_name"
return 1
Expand All @@ -647,11 +691,11 @@ shim_plugin_versions() {

shim_plugins() {
local executable_name
executable_name=$(basename "$1")
executable_name=$(fast_basename "$1")
local shim_path
shim_path="$(asdf_data_dir)/shims/${executable_name}"
if [ -x "$shim_path" ]; then
grep "# asdf-plugin: " "$shim_path" 2>/dev/null | sed -e "s/# asdf-plugin: //" | cut -d' ' -f 1 | uniq
awk '/# asdf-plugin: / { l=$(NF-1) } END { print l} ' "$shim_path" 2>/dev/null
stephanGarland marked this conversation as resolved.
Show resolved Hide resolved
else
printf "asdf: unknown shim %s\n" "$executable_name"
return 1
Expand All @@ -667,7 +711,9 @@ strip_tool_version_comments() {
# 2. Find a # and delete it and everything after the #.
# 3. Remove any whitespace from the end of the line.
# Finally, the command will print the lines that are not empty.
sed '/^[[:blank:]]*#/d;s/#.*//;s/[[:blank:]]*$//' "$tool_version_path"
# this is ~2% faster than the original implementation
# by combining steps 2 and 3
sed '/^[[:blank:]]*#/d; s/\s*#.*$//;' "$tool_version_path"
}

asdf_run_hook() {
Expand All @@ -686,19 +732,18 @@ asdf_run_hook() {
get_shim_versions() {
shim_name=$1
shim_plugin_versions "${shim_name}"
shim_plugin_versions "${shim_name}" | cut -d' ' -f 1 | awk '{print$1" system"}'
shim_plugin_versions "${shim_name%% [0-9]*} system}"
}

preset_versions() {
shim_name=$1
shim_plugin_versions "${shim_name}" | cut -d' ' -f 1 | uniq | xargs -IPLUGIN bash -c ". $(asdf_dir)/lib/utils.bash; printf \"%s %s\n\" PLUGIN \$(get_preset_version_for PLUGIN)"
shim_plugin_versions "${shim_name%% [0-9]*}" | xargs -IPLUGIN bash -c ". $(asdf_dir)/lib/utils.bash; printf \"%s %s\n\" PLUGIN \$(get_preset_version_for PLUGIN)"
}

select_from_preset_version() {
local shim_name=$1
local shim_versions
local preset_versions

shim_versions=$(get_shim_versions "$shim_name")
if [ -n "$shim_versions" ]; then
preset_versions=$(preset_versions "$shim_name")
Expand All @@ -721,7 +766,6 @@ select_version() {

local plugins
IFS=$'\n' read -rd '' -a plugins <<<"$(shim_plugins "$shim_name")"

for plugin_name in "${plugins[@]}"; do
local version_and_path
local version_string
Expand Down Expand Up @@ -751,7 +795,7 @@ select_version() {

with_shim_executable() {
local shim_name
shim_name=$(basename "$1")
shim_name=$(fast_basename "$1")
local shim_exec="${2}"

if [ ! -f "$(asdf_data_dir)/shims/${shim_name}" ]; then
Expand All @@ -761,7 +805,6 @@ with_shim_executable() {

local selected_version
selected_version="$(select_version "$shim_name")"

if [ -z "$selected_version" ]; then
selected_version="$(select_from_preset_version "$shim_name")"
fi
Expand Down