scripts(termux_download_deb_pac): optimization and refactoring #24621

twaik · 2025-05-04T11:46:39Z

Moved duplicated code to a separate function to make code more readable and editable.
Removed invocations of external commands like sed and seq to avoid binary execution overhead, using bash builtins instead.
Replaced all test and [ with [[ to improve speed.
Grouped some variable declarations under single expressions to avoid parsing overhead.
Replaced ${var} with $var in some places to reduce parsing overhead.

TomJo2000 · 2025-05-04T11:55:16Z

Moved duplicated code to a separate function to make code more readable and editable. Removed invocations of external commands like sed and seq to avoid binary execution overhead, using bash builtins instead. Replaced all test and [ with [[ to improve speed.

Makes sense.

Grouped some variable declarations under single expressions to avoid parsing overhead.

That's probably not worth worrying about but go ahead.

Replaced ${var} with $var in some places to reduce parsing overhead.

Since when does that cause additional overhead?
I'm gonna need to see some benchmarks for that one in particular.

twaik · 2025-05-04T12:04:14Z

Since when does that cause additional overhead?
I'm gonna need to see some benchmarks for that one in particular.

$ time for ((i=0; i<10000000; i++)); do [[ "$PKG_HASH" != "null" ]]; done
real    0m13.239s
user    0m13.238s
sys     0m0.001s

$ time for ((i=0; i<10000000; i++)); do [[ "${PKG_HASH}" != "null" ]]; done
real    0m14.320s
user    0m14.318s
sys     0m0.001s

scripts/build/termux_download_deb_pac.sh

TomJo2000 · 2025-05-04T12:38:35Z

Since when does that cause additional overhead?
I'm gonna need to see some benchmarks for that one in particular.
$ time for ((i=0; i<10000000; i++)); do [[ "$PKG_HASH" != "null" ]]; done
real    0m13.239s
user    0m13.238s
sys     0m0.001s

$ time for ((i=0; i<10000000; i++)); do [[ "${PKG_HASH}" != "null" ]]; done
real    0m14.320s
user    0m14.318s
sys     0m0.001s

10 million iterations.
So we are literally talking about microseconds here.

twaik · 2025-05-04T12:42:24Z

Yes. Currently it does not matter but I am planning to parralelize packages downloading and after that this optimization will be more effective.

TomJo2000 · 2025-05-04T12:46:31Z

Yes. Currently it does not matter but I am planning to parralelize packages downloading and after that this optimization will be more effective.

My local testing puts the per-instance difference of $var vs ${var} at about 6 microseconds.
Or 0.000006 seconds, or 0.006 milliseconds.

This codepath is nowhere near hot enough to apply that level of pedantry.
Even after you parallelize it.

twaik · 2025-05-04T12:47:48Z

So we are literally talking about microseconds here.

I am planning to write a cron script for package building validation.
We have ~>2500 packages, which are being built for 4 archs.
So if I optimize our buildsystem scripts to save at least 5 seconds I will save at least one calendar day during each build validation attempt. From this point of view it becomes more valuable, right?

TomJo2000 · 2025-05-04T12:50:11Z

I am planning to write a cron script for package building validation. We have ~>2500 packages, which are being built for 4 archs. So if I optimize our buildsystem scripts to save at least 5 seconds I will save at least one calendar day during each build validation attempt. From this point of view it becomes more valuable, right?

4 arches. Those run in parallel.
2500-ish iterations each.
0.006ms saved on each.

We're saving 15ms (Milliseconds, not seconds), for each ${var} you turn into a $var.
There's not enough ${var}'s in the repo to make that worth while.

twaik · 2025-05-04T12:56:01Z

Ok, I'll revert this one after getting home.

Maxython · 2025-05-04T12:56:11Z

I did a quick test of these changes on pacman package loading, and I can say that the current changes break pacman package loading completely.

twaik · 2025-05-04T13:02:25Z

I tried to build pacman packages in termux-packages repo and everything was fine, but it seems like it downloaded packages from debian repos. Does pacman-packages-related building require pacman-packages's version of repo.json?

Maxython · 2025-05-04T13:05:14Z

I tried to build pacman packages in termux-packages repo and everything was fine, but it seems like it downloaded packages from debian repos. Does pacman-packages-related building require pacman-packages's version of repo.json?

For testing you need to reconfigure repo.json to the termux-pacman repositories (service) - https://github.com/termux-pacman/termux-packages/blob/master/repo.json

agnostic-apollo · 2025-05-04T16:43:42Z

Note that this has been rewritten in my repo.json changes pull including separate functions and any changes will be replaced in coming weeks. This also applies to most of the other dependencies/repo code. So you might want to hold off till after.

twaik · 2025-05-04T16:45:22Z

When exactly do you want to merge your PRs? And maybe you have links to your PRs?

agnostic-apollo · 2025-05-04T16:59:28Z

Those changes are not final yet, so haven't pushed. I will working on them after current Termux app and keychain releases, when I work on the new bootstraps. There would be one for docker, another for repo.json (pull is at #21181 but changes there are outdated and different from my local changes) and another for other refactoring of the build system (some changes are at agnostic-apollo@8ab564a).

Instead of doing major refactoring for structuring, if there are any high latency parts of the code taking significant seconds (not micro or mill seconds), feel free to make changes for that, I will handle any conflicts against those.

Moved duplicated code to a separate function to make code more readable and editable. Removed invocations of external commands like `sed` and `seq` to avoid binary execution overhead, using bash builtins instead. Replaced all `test` and `[` with `[[` to improve speed. Grouped some variable declarations under single expressions to avoid parsing overhead.

twaik · 2025-05-04T17:21:59Z

Instead of doing major refactoring for structuring, if there are any high latency parts of the code taking significant seconds (not micro or mill seconds), feel free to make changes for that, I will handle any conflicts against those.

These changes are preparation for making parallel downloading of dependencies, but refactoring old code at least to get rid of duplicates is good too.

taking significant seconds (not micro or mill seconds)

Parallel downloads can save minutes of CI time during building big packages or packages with big amount of dependencies, i.e. firefox, thunderbird or kdenlive.

BTW do our servers have any kind of ratelimiting, preventing parallel downloads of packages or anything similar?

twaik · 2025-05-04T17:22:31Z

@Maxython now pacman-packages should be fine too.

robertkirkman · 2025-05-04T22:22:20Z

scripts/build/termux_download_deb_pac.sh

 		return 1
 	fi

-	termux_download "${TERMUX_REPO_URL[${idx}-1]}/${PKG_PATH}" \
+	termux_download "${TERMUX_REPO_URL[${idx}]}/${PKG_PATH}" \


I've always wondered about this, but, there is a set of very similar code in the file generate-bootstraps.sh. Do you think it would be appropriate or possible to use some of these optimizations to also optimize that, by making that script import and use the same code that this script is using?

termux-packages/scripts/generate-bootstraps.sh

Line 149 in ee6cd92

curl --fail --location --output "$package_tmpdir/package.deb" "$package_url"

That script already sources properties.sh, so as I understand it, that means that it can already only work inside the Docker container, so there would be no meaningful downside to making it also source other files, especially if it would result in optimization and DRY.

The new bootstrap will use new repo.json format and will share code with build-package.sh. I also also removed the duplicate code changed above. There are lot of other issues to handle in current code.

Oh ok, I didn't know that you already changed generate-bootstraps.sh in the future version you mentioned.

I have been using code that I heavily copied from generate-bootstraps.sh somewhere else as well, with a comment about where it came from, so I'll want to pause that and wait for the new generate-bootstraps.sh to come so I can keep my copied implementation mostly up-to-date with the reference implementation.

${TERMUX_REPO_URL[${idx}-1]}

It is related to using seq instead of iterating over array keys.

Maxython · 2025-05-04T18:40:59Z

scripts/build/termux_download_deb_pac.sh

+		read -d "\n" PKG_PATH PKG_HASH <<< "$(jq -r '.["'"$PACKAGE"'"] |
+			select((.VERSION == "'"$VERSION_PACMAN"'" or "'"$TERMUX_WITHOUT_DEPVERSION_BINDING"'" == "true") and .SHA256SUM and .FILENAME) |
+			"'"$arch"'/\(.FILENAME) \(.SHA256SUM)"' "$pkg_file_path")" || :


I suggest the following changes to improve the readability of jq algorithms:

Suggested change

read -d "\n" PKG_PATH PKG_HASH <<< "$(jq -r '.["'"$PACKAGE"'"] |

select((.VERSION == "'"$VERSION_PACMAN"'" or "'"$TERMUX_WITHOUT_DEPVERSION_BINDING"'" == "true") and .SHA256SUM and .FILENAME) |

"'"$arch"'/\(.FILENAME) \(.SHA256SUM)"' "$pkg_file_path")" || :

read -d "\n" PKG_PATH PKG_HASH <<< "$(jq -r ".[\"$PACKAGE\"] |

select((.VERSION == \"$VERSION_PACMAN\" or \"$TERMUX_WITHOUT_DEPVERSION_BINDING\" == \"true\") and .SHA256SUM and .FILENAME) |

\"${arch}/\"+.FILENAME, .SHA256SUM" "$pkg_file_path")" || :

Maxython · 2025-05-04T18:47:51Z

scripts/build/termux_download_deb_pac.sh

+}
+
+termux_download_deb_pac_get_pkg_hash_and_version() {
+	local arch="$1" pkg_file_path="$TERMUX_COMMON_CACHEDIR-$1/$PACKAGE_FILE_PATH" PKG_PATH="" PKG_HASH=""


The arch variable in the termux_download_deb_pac_get_pkg_hash_and_version() function is used only once, although it could be used in the pkg_file_path variable, but because they are in the same line, it is not possible to do so. Perhaps you should move the arch variable before the pkg_file_path variable or remove the arch variable.

twaik requested review from robertkirkman, TomJo2000 and thunder-coding May 4, 2025 11:46

twaik requested a review from Grimler91 as a code owner May 4, 2025 11:46

twaik requested a review from Maxython May 4, 2025 11:50

twaik force-pushed the optimize-termux_download_deb_pac branch from 5068f94 to eb6f7dc Compare May 4, 2025 11:53

TomJo2000 requested changes May 4, 2025

View reviewed changes

twaik force-pushed the optimize-termux_download_deb_pac branch 4 times, most recently from 02da454 to 1d62bb0 Compare May 4, 2025 16:33

twaik force-pushed the optimize-termux_download_deb_pac branch from 1d62bb0 to 752d705 Compare May 4, 2025 17:14

robertkirkman reviewed May 4, 2025

View reviewed changes

Maxython reviewed May 5, 2025

View reviewed changes

Uh oh!

scripts(termux_download_deb_pac): optimization and refactoring #24621

Are you sure you want to change the base?

scripts(termux_download_deb_pac): optimization and refactoring #24621

Conversation

twaik commented May 4, 2025

Uh oh!

TomJo2000 commented May 4, 2025

Uh oh!

twaik commented May 4, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomJo2000 commented May 4, 2025

Uh oh!

twaik commented May 4, 2025

Uh oh!

TomJo2000 commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twaik commented May 4, 2025

Uh oh!

TomJo2000 commented May 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twaik commented May 4, 2025

Uh oh!

Maxython commented May 4, 2025

Uh oh!

twaik commented May 4, 2025

Uh oh!

Maxython commented May 4, 2025

Uh oh!

agnostic-apollo commented May 4, 2025

Uh oh!

twaik commented May 4, 2025

Uh oh!

agnostic-apollo commented May 4, 2025

Uh oh!

twaik commented May 4, 2025

Uh oh!

twaik commented May 4, 2025

Uh oh!

robertkirkman May 4, 2025

Choose a reason for hiding this comment

Uh oh!

agnostic-apollo May 4, 2025

Choose a reason for hiding this comment

Uh oh!

robertkirkman May 5, 2025

Choose a reason for hiding this comment

Uh oh!

twaik May 5, 2025

Choose a reason for hiding this comment

Uh oh!

Maxython May 4, 2025

Choose a reason for hiding this comment

Uh oh!

Maxython May 4, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TomJo2000 commented May 4, 2025 •

edited

Loading

TomJo2000 commented May 4, 2025 •

edited

Loading