improve speed of scripts

[Subarctic2796]

• a GitHub Issue was opened for this feature / bug.
• test coverage was added (Cucumber or Spock as appropriate).

Fixes #XXX

Hi.
I know I did not link to any open issues. But I still think that this is an important change.
Basically I vastly improved the execution speed of the scripts. I do know that most of the functions in the scripts are being rewritten to the native versions, with that in mind I still think it is important as it also vastly improves the start up time of a shell that is using sdkman. It also uses many more native bash features. I also changed some of the formating of the scripts if that is okay.

Most of the changes I made are commented or are using the same technique as other changes. If you are not sure why I changed something I am more than happy to explain what I did.

[marc0der]

Hi @Subarctic2796,

Unfortunately, we won't be making broad sweeping changes to the entire CLI so close to its retirement. This piece of software has been tried and tested over many years and has very few issues due to small incremental fixes and improvements from many contributors. After all, in the words of Thomas Bertram, "If it ain't broke, don't fix it" 🙂

Basically I vastly improved the execution speed of the scripts.

Can you be more specific about your vast improvements? Can you provide your benchmarks? When I source the sdkman-init.sh on my machine, it responds instantly, so I'm not sure why you see such degradation in your environment.

Lastly, I noticed that all checks have failed on your build, which immediately decreases my confidence in the PR.

In the future, please reach out to us on our community Discord server as described in the contributing guidelines linked below. Having an upfront discussion about such big change will avoid disappointment when your PR is closed on submission.

[Subarctic2796]

Hi sorry about this PR. I realised to late that the changes I made were causing the tests to fail, and I ran out of time yesterday to fix them. So I completely understand why you closed the PR.

All I was trying to do was improve the speed of the init script as it causes my shell to start up slowly.
So I made changes to the local init script on my machine and it as a result my shell starts up way faster now so I wanted to contribute that to the community.
So while I was doing that I decided to also just change the other scripts and got carried away and only after did I see that the tests were failing. And as I said I didn't have time to fix them yesterday.

So I am sorry for this waste of time. So I have deleted that fork.

That being said eventhough the native rewrite is happening, it will still need an init script to set everything up. I am more than happy to help speed it up if you want. But I completely understand it you don't want my help, as I may be sounding arrogant, which isn't my intention. I love what you have done for the community and I just wanted to help.

You are welcome to stop reading now.
But, below are a few improvements that will speed up and (in my opinion also) clean up the init script.

Change the if [ -z "$var" ] checks to use parameter expansion instead.
So things like this

if [ -z "$SDKMAN_CANDIDATES_API" ]; then
	export SDKMAN_CANDIDATES_API="https://api.sdkman.io/2"
fi

Become:

export SDKMAN_CANDIDATES_API=${SDKMAN_CANDIDATES_API:-"https://api.sdkman.io/2"}

It does the same thing but it is faster for bash and zsh to perform, I also think it is clearer.

Remove the cat in

SDKMAN_PLATFORM="$(cat "${SDKMAN_DIR}/var/platform")"

And change it to

SDKMAN_PLATFORM="$(<${SDKMAN_DIR}/var/platform")"

I am not sure why you do it here as you do it the way bash recommends here

SDKMAN_CANDIDATES_CSV=$(<"$SDKMAN_CANDIDATES_CACHE")

So thats just a bit weird.

When you are looking for the scripts instead of using find and putting it into an array you can use globbing instead.
So this

scripts=($(find "${SDKMAN_DIR}/src" "${SDKMAN_DIR}/ext" -type f -name 'sdkman-*.sh'))
for f in "${scripts[@]}"; do
	source "$f"
done

becomes

for f in $SDKMAN_DIR/{src,ext}/sdkman-*.sh; do
	source "$f"
done

The only issue with this is that if there are no scripts in $SDKMAN_DIR/ext then it will fail so you have to enable nullglobing on bash and reset it back to the original user settings.
On zsh you can just do

for f in $SDKMAN_DIR/{src,ext}/sdkman-*.sh(N); do
	source "$f"
done

Instead of doing this

for candidate_name in "${SDKMAN_CANDIDATES[@]}"; do
       candidate_dir="${SDKMAN_CANDIDATES_DIR}/${candidate_name}/current"
	candidate_dir="$candidate_name/current"
	if [[ -h "$candidate_dir" || -d "${candidate_dir}" ]]; then
		__sdkman_export_candidate_home "$candidate_name" "$candidate_dir"
		__sdkman_prepend_candidate_to_path "$candidate_dir"
	fi
done

to add the candidates to they path, this does way to many unnessasary iterations and pointless function calls
You should do this instead

for candidate_name in $SDKMAN_CANDIDATES_DIR/*; do
	candidate_dir="$candidate_name/current"
	if [[ -h "$candidate_dir" || -d "${candidate_dir}" ]]; then
		candidate_name="${candidate_name##*/}"
		[[ -z "${candidate_name:u}_HOME}" ]] && export "${candidate_name:u}_HOME"="$candidate_dir"
		if [[ -d "${candidate_dir}/bin" ]]; then
			candidate_dir="$candidate_dir/bin"
		fi
		[[ "$PATH" == *"$candidate_dir"* ]] || PATH="${candidate_dir}:${PATH}"
	fi
done

Although this has the same issue with nullglobing but the same solution applies for zsh and for bash you can just fix the nullglob option after sourcing the scripts and adding the candidates to the path here.

If you want any explanations I would be happy to explain. And if you want me to make these changes I would be happy to make a new PR that only makes these changes and I would make sure that the tests pass.

[marc0der]

@Subarctic2796 I'm happy to take on board some of these changes you noted above in new PRs that only touch the affected areas in the init script. We will only consider merging if you open small PRs for the individual pieces of work and if all the checks pass.

[Subarctic2796]

Awesome thanks I will do that