install.sh: Perfomance: Use more shell builtins #106

mbargull · 2021-02-19T12:34:32Z

Replace echo/grep/cut/dirname/basename by variable substitutions and
case pattern matching to reduce the amount of subprocesses called for
every copied file.

The current echo | grep/echo | cut/dirname/basename invocations slow down the installation process, esp. when components with many files (rust-docs) are processed.
This pull request replaces them with shell builtin substitutions to avoid process calling overhead.
Locally for me, these small changes reduced the install time (for rust-docs only) from around 20 minutes to around 3 minutes.

Replace echo/grep/cut/dirname/basename by variable substitutions and case pattern matching to reduce the amount of subprocesses called for every copied file.

mbargull · 2021-02-22T13:33:40Z

install-template.sh

-                eval $v=$val
-            fi
+            case "${arg}" in
+                "--${op}="* )


FYI, this is technically a behavioral change because the original code grepped for "--$op" instead of "^--$op" -- testing for the prefix is probably intended, though. Same for the --$option case below.

Since the manifests have sorted file lists, consecutive invocations of abs_path (make_dir_recursive) likely do redundant work. Avoiding these redundancies reduces the amount of subprocessing per file further.

mbargull · 2021-02-22T19:06:14Z

install-template.sh

+    local file_path_dirname="${file_path%/*}"
+    local file_path_basename="${file_path##*/}"


NB: These dirname/basename substitutions assume more well-formed paths, e.g., no trailing slashes etc.
Let me know if you aren't comfortable with adding these constraints and if I should revert to dirname/basename.

pietroalbini · 2021-03-02T18:45:08Z

Thanks for the PR! It might take a bit for me to review the PR, but I'll eventually get it done :)

fweimer · 2021-03-21T09:31:46Z

Does this address the apparent hang after install: installing component 'rust-docs' with the 1.50.0 installer? strace shows many grep invocations with ^etc/, ^bin/, ^lib/ etc.

mbargull · 2021-03-21T09:42:23Z

@fweimer, these changes replace all those grep calls with a case construct. So yes, it greatly reduces the observed "apparent hang".

jyn514 · 2021-04-07T15:04:33Z

install-template.sh

+    # Skip if the last invocation of make_dir_recursive had the same argument
+    if ! [ "$_dir" = "${_make_dir_recursive_cached_key:-}" ]
+    then
+        _make_dir_recursive_cached_key="$_dir"


This seems like kind of an odd optimization, it only works if the very last call was the same directory. Have you compared the times with and without it?

Yes, see commit message 53d557b : input lists are sorted by path, the directories usually contain more than one file and as such this optimization is hit frequently.
I did compare times and the difference was significant enough to warrant the added complexity. However, I did not record any numbers.

This also significantly reduced the log file size.

Went AFK for a bit to let this run:

# pwd /tmp/rust-1.51.0-x86_64-unknown-linux-gnu # stat -fc%T /tmp tmpfs # dst=/tmp/destdir # for c in 53d557b 3d7ed69 5254dbf ; do curl -sLO "https://raw.githubusercontent.com/mbargull/rust-installer/${c}/install-template.sh" ; rm -rf "${dst}" ; printf %s\\n "${c}" ; time bash install-template.sh --destdir="${dst}" >/dev/null ; find "${dst}" -name \*.log -exec stat -c'%n %s' {} \; ; done 53d557b install: WARNING: failed to run ldconfig. this may happen when not installing as root. run with --verbose to see the error real 44.19 user 35.54 sys 11.26 /tmp/destdir/usr/local/lib/%%TEMPLATE_REL_MANIFEST_DIR%%/install.log 11361126 3d7ed69 install: WARNING: failed to run ldconfig. this may happen when not installing as root. run with --verbose to see the error real 105.68 user 82.46 sys 30.43 /tmp/destdir/usr/local/lib/%%TEMPLATE_REL_MANIFEST_DIR%%/install.log 12935429 5254dbf install: WARNING: failed to run ldconfig. this may happen when not installing as root. run with --verbose to see the error real 688.47 user 570.21 sys 207.40 /tmp/destdir/usr/local/lib/%%TEMPLATE_REL_MANIFEST_DIR%%/install.log 12935429

So for me that commit cuts about 60 % of the time on a tmpfs.
(install.log size difference is not that big in this case due to short prefix/destdir. At https://github.com/conda-forge/rust-feedstock we have longer (~ 255 characters) install prefixes, resulting in a much larger log file.)

Hmm, I wonder if you could get almost as much of a speedup by removing the logging instead. It sounds like it takes a while and isn't super useful.

No, the logging is just uses the echo builtin which should be able to offer high throughput.
One thing to try is whether we can just use a test -d DIR for make_dir_recursive instead of the memorized logic. (For the abs path function I don't see a similarly performing alternative.)

Yep, test -d gives the same run time. I've added two cleanup commits.

(Never mind the 2nd commit which I force-pushed away. I just forgot why (abs path from file vs dir) I split those functions.)

andersk · 2021-04-18T09:17:17Z

install-template.sh

@@ -625,7 +626,7 @@ install_components() {

 		    maybe_backup_path "$_file_install_path"

-		    if echo "$_file" | grep "^bin/" > /dev/null || test -x "$_src_dir/$_component/$_file"
+		    if test -z "${_file##bin/*}" || test -x "$_src_dir/$_component/$_file"


test -z "${_file##bin/*}" unintentionally accepts the empty string; [ "${_file#bin/}" != "$_file" ] would be a slightly more faithful translation. (Presumably this doesn’t really matter.)

Or there’s [[ "$_file" = bin/* ]] since this is #!/bin/bash, although the script seems to otherwise avoid bashisms so I guess that avoidance should be preserved.

${_file} is never empty here because of https://github.com/rust-lang/rust-installer/pull/106/files#diff-ec68db39ae4ea5bfe559a47a8a880d5d383dfa81ba067652d2adfcc3c0cd2a17R571 .
So,

if test -z "${_file##bin/*}" || ...

if [ "${_file#bin/*}" != "${_file}" ] || ...

if case "${_file}" in bin/*) ;; *) false ; esac || ... (POSIX-y [[ "${_file}" = bin/*)
should all be equivalent here.
If someone has a strong preference for any of those options (I don't), then I'm happy to change it accordingly!

install.sh: Perfomance: Use more shell builtins

3d7ed69

Replace echo/grep/cut/dirname/basename by variable substitutions and case pattern matching to reduce the amount of subprocesses called for every copied file.

mbargull commented Feb 22, 2021

View reviewed changes

install.sh: Cache abs_path and make_dir_recursive

53d557b

Since the manifests have sorted file lists, consecutive invocations of abs_path (make_dir_recursive) likely do redundant work. Avoiding these redundancies reduces the amount of subprocessing per file further.

mbargull commented Feb 22, 2021

View reviewed changes

mbargull mentioned this pull request Apr 7, 2021

Installing docs with x.py install is very slow. rust-lang/rust#80684

Closed

jyn514 reviewed Apr 7, 2021

View reviewed changes

install.sh: Use test -d for make_dir_recursive

2ccfecc

mbargull force-pushed the faster-install.sh branch from 7294f01 to 2ccfecc Compare April 7, 2021 20:00

andersk approved these changes Apr 18, 2021

View reviewed changes

jyn514 approved these changes Apr 18, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install.sh: Perfomance: Use more shell builtins #106

install.sh: Perfomance: Use more shell builtins #106

mbargull commented Feb 19, 2021

mbargull Feb 22, 2021

mbargull Feb 22, 2021

pietroalbini commented Mar 2, 2021

fweimer commented Mar 21, 2021

mbargull commented Mar 21, 2021

jyn514 Apr 7, 2021

mbargull Apr 7, 2021

mbargull Apr 7, 2021

mbargull Apr 7, 2021

jyn514 Apr 7, 2021 •

edited

Loading

mbargull Apr 7, 2021

mbargull Apr 7, 2021

mbargull Apr 7, 2021

andersk Apr 18, 2021

mbargull Apr 18, 2021

		local file_path_dirname="${file_path%/*}"
		local file_path_basename="${file_path##*/}"

install.sh: Perfomance: Use more shell builtins #106

Are you sure you want to change the base?

install.sh: Perfomance: Use more shell builtins #106

Conversation

mbargull commented Feb 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroalbini commented Mar 2, 2021

fweimer commented Mar 21, 2021

mbargull commented Mar 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyn514 Apr 7, 2021 •

edited

Loading