Skip to content
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

single_files_libs/combine.sh very slow, ~3 minutes. #2924

Closed
averms opened this issue Dec 13, 2021 · 4 comments
Closed

single_files_libs/combine.sh very slow, ~3 minutes. #2924

averms opened this issue Dec 13, 2021 · 4 comments

Comments

@averms
Copy link

averms commented Dec 13, 2021

Describe the bug
The amalgamation script is very slow, running it through Bash takes around 3 minutes for zstd-in.c.

To Reproduce
Steps to reproduce the behavior:

  1. cd build/single_file_libs && time create_single_file_library.sh

Expected behavior
Faster amalgamation.

Solution

I wrote an alternative in Python. It is 135x faster so it takes around 1 second. If you don't want to take an (optional) build dependency on Python, perhaps the shell script could be refactored somehow. There is a comment in the source suggesting that speed was sacrificed for POSIX compliance but the script isn't compliant with POSIX because it uses local.

@Cyan4973
Copy link
Contributor

Moving to Python is something we consider, especially for such a remarkable speed improvement.
The plan is to keep both versions (at least in short term), so that environments without python dependency can still run the shell script as backup.

@cwoffenden
Copy link
Contributor

cwoffenden commented Jan 15, 2022

I also rewrote this in Python3 (original author of the script here) but due to the required platform support (at the time) this shell script was the option that worked everywhere. The original was version in Bash, and was much quicker, but the CI for older platforms meant plain ol' sh or nothing.

@Cyan4973
Copy link
Contributor

I believe we should keep the sh version around, precisely because it can be used in more systems with less dependencies.
But it doesn't prevent the existence of faster variants alongside the sh one, that could even become default if it feels reasonable in term of dependencies (and python is a reasonable dependency).

@cwoffenden
Copy link
Contributor

Fixed in #3005

@averms averms closed this as completed Jan 23, 2022
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

No branches or pull requests

3 participants