-
-
Notifications
You must be signed in to change notification settings - Fork 643
Speed up initialization in popular shells #1470
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
Conversation
marc0der
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great, thank you! I've left some comments below that need to be addressed first. Also, please remove any comments unless they absolutely add value. We rely on clear git commit messages to describe changes in the code.
| # Generate home variable name using shell-specific methods | ||
| # to avoid heavy-weight fork/exec system calls. | ||
|
|
||
| if [ -n "${ZSH_VERSION-}" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have a global variable that you can use here.
| if [[ -d "${candidate_dir}/bin" ]]; then | ||
| echo "${candidate_dir}/bin" | ||
| # Generate home variable name using shell-specific methods | ||
| # to avoid heavy-weight fork/exec system calls. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer not to have comments unless it is absolutely necessary. Having all the details in the Git commit message is sufficient.
| if [ -n "${ZSH_VERSION-}" ]; then | ||
| # zsh: uppercase via ${value:u} | ||
| upcase_name="${candidate_name:u}" | ||
| elif [ -n "${BASH_VERSION-}" ] && [ "${BASH_VERSINFO[0]:-0}" -ge 4 ] 2>/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can also use the global definition here. Do we need to bolster our global definition with BASH_VERSIONINFO too?
| unset CANDIDATE_BIN_DIR | ||
| # replace the original candidate_dir with the 'bin' subfolder if it exists | ||
| if [ -d "${candidate_dir}/bin" ]; then | ||
| candidate_dir="${candidate_dir}/bin" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not correct, as candidate_dir describes the base directory of the candidate (the candidate's home directory), not its bin directory. For readability, I prefer a separate variable for the candidate_bin_dir as we had before.
|
Thanks for the review! My intent with this PR was to address the core issue using practices aligned with widely accepted Unix and software engineering conventions. That said, I realize this project may follow its own stylistic and structural expectations — and I fully respect that. If you feel the changes are useful overall and just need polish, feel free to adapt the PR as needed. Thanks again either way! |
|
Hey @azhuchkov, sad that you aren't willing to polish this a little. It is standard practice to review PRs and give feedback, and it is never personal. You've done great work here, and many people can benefit. If you are willing to make these minor tweaks, I can merge it right away so all can benefit. |
|
@marc0der I’ve thought through the changes you suggested, and I’m not entirely sure I can apply them in a consistent way — I mean mixing those project-level globals and Bash variables without introducing additional variables into the user’s session. Since the PR is open for edits, feel free to adjust it if you’d like. In any case, appreciate the review and consideration. |
|
I've now cleaned up the code and removed all the unnecessary comments. Thank you for contributing. |
|
Thanks. Please note that macOS ships bash 3.2 due to license restrictions, and hence doesn't support |
Motivation
SDKMAN! exhibits noticeable shell startup latency as installations grow. Profiling shows that two initialization functions are invoked repeatedly and account for a significant share of startup time. The call counts appear to match the number of installed candidates. The likely cause is this per-candidate loop in
sdkman-init.sh:sdkman-cli/src/main/bash/sdkman-init.sh
Lines 116 to 122 in dda90bc
The loop iterates over all available candidates and spawns external processes for each installed candidate under the hood.
For a back-of-the-envelope estimate: if one process spawn costs ~10 ms and you have 10 candidates, that’s ~200 ms added to shell startup (two spawns per candidate —
trandgrep). While ~200 ms isn’t large on its own, it scales linearly as more candidates are installed — and SDKMAN! isn’t the only thing initializing in an advanced shell configuration. See #977 for user feedback confirming the UX impact.Solution
This PR eliminates per-candidate external process overhead during initialization, relying on shell-native logic where possible.
zprofoutput forzshstartup without the patch (9 of 77 candidates installed):As shown above, SDKMAN! initialization accounts for ~50% of total shell startup time in this profile. The other half is taken by a relatively heavy
zshsetup: a custom theme and init of 6+ plugins (omitted from the output for brevity).Now, the same profile with the patch applied:
Both functions dropped to <1% of total time after the patch.
Fixes #977.
These changes have been manually verified on
zsh 5.8,bash 5.2, andbash 3.2.Almost all automated tests pass on the development machine; five fail with the same
Request was not matched error: