close
Skip to content

fix: don't invoke asdf inside asdf commands#1208

Merged
Stratus3D merged 3 commits into
masterfrom
tb/fix-recursive-invocations
Apr 25, 2022
Merged

fix: don't invoke asdf inside asdf commands#1208
Stratus3D merged 3 commits into
masterfrom
tb/fix-recursive-invocations

Conversation

@Stratus3D

@Stratus3D Stratus3D commented Apr 20, 2022

Copy link
Copy Markdown
Member

Recursive calls have a number of disadvantages:

  • Poorer performance since each invocation spawns and new process and re-executes all the code in bin/asdf
  • Makes debugging more difficult
  • More likely to introduce subtle bugs and the possibility for infinite loops

Fixes: No known bugs this fixes, but hopefully these changes have flushed out a few obscure bugs.

Reviewing this code you'll see commands like asdf latest in the code have been replaced by function calls like latest_command. I'll break down how each of these work to show how important this fix is.

asdf latest

  • Locate the asdf executable on the user's $PATH
  • Spawn a new process that executes it
  • That new process executes all of the code in bin/asdf, parses the arguments, determines what asdf command_ script to run and then sources it.
  • Finally the output is printed and returned to the calling script

latest_command

  • latest_command is a function that is invoked in the current process
  • All the code in latest_command is executed, but everything it does is needed by the calling script, so there is no waste
  • The output is printed and the function returns

As you can see there is a lot of unnecessary overhead when executing asdf ... commands from inside other asdf command scripts. I did some rough benchmarking locally and saw a speedup of about 10% with this fix.

Recursive calls have a number of disadvantages:

* Poorer performance since each invocation spawns and new process and re-executes all the code in bin/asdf
* Makes debugging more difficult
* More likely to introduce subtle bugs and the possibility for infinite loops
@Stratus3D Stratus3D requested a review from a team as a code owner April 20, 2022 01:10
if [ $# -eq 0 ]; then
for plugin in $(asdf plugin list); do
# shellcheck disable=SC2119
for plugin in $(plugin_list_command); do

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

asdf is a binary on the user's path, whereas plugin_list_command is a Bash function already in memory. While the code looks similar invoking asdf results in a new process that re-executes all the code in bin/asdf.

@@ -1,4 +1,6 @@
# -*- sh -*-
# shellcheck source=lib/functions/versions.bash
. "$(dirname "$(dirname "$0")")/lib/functions/versions.bash"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since we are now invoking the functions instead of bin/asdf I moved functions that need to be invoked from multiple commands into a couple "function" files that store related functions. In this case, functions related to tool versions.

# shellcheck source=lib/commands/reshim.bash
. "$(dirname "$ASDF_CMD_FILE")/reshim.bash"
# shellcheck source=lib/functions/installs.bash
. "$(dirname "$(dirname "$0")")/lib/functions/installs.bash"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Code was re-arranged and calls to asdf <cmd> were replaced with the corresponding function calls, but no other code was changed. The diff is pretty big but most of it is moving code into the new function files.

@Stratus3D

Copy link
Copy Markdown
Member Author

Ping.

@Stratus3D

Copy link
Copy Markdown
Member Author

Merging now as the only code changes here are to replace internal asdf <cmd> calls with the corresponding Bash function. All other changes are code re-org.

@Stratus3D Stratus3D merged commit 27f7ef7 into master Apr 25, 2022
@Stratus3D Stratus3D deleted the tb/fix-recursive-invocations branch April 25, 2022 12:45
@jthegedus

Copy link
Copy Markdown
Contributor

Sorry I didn't get to reviewing this before merge. Really liking the lib/functions files as it gives the code some more structure. It would be good to apply this kind of separation to the utils file too at some point.

@Stratus3D

Copy link
Copy Markdown
Member Author

Sorry I didn't get to reviewing this before merge. Really liking the lib/functions files as it gives the code some more structure. It would be good to apply this kind of separation to the utils file too at some point.

Yes absolutely! utils has been a random collection of utility functions, but really all of them pertain to one thing or another. Ideally we shouldn't even have a generic utils file unless it's for truly generic helper functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants