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

restore combine.sh bash performance while still sticking to POSIX #3241

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

wahern
Copy link
Contributor

@wahern wahern commented Aug 11, 2022

POSIX sh case statement is just as fast Bash's =~ extended test operator. It takes a glob pattern, though, so this patch uses it to short-circuit before falling back to grep. This patch effectively restores the original Bash performance, at least as compared with macOS /bin/bash and using the three older Bash regex tests. (It's negligibly slower on account of the #include and #pragma tests being called through a function rather than inline within the if statement.)

I experimented with POSIX expr(1), which takes BREs, but it's not a built-in on FreeBSD, macOS, or OpenBSD /bin/sh and wasn't any faster than invoking grep.

fnmatch function signature comes from Rich Felker's http://www.etalabs.net/sh_tricks.html

@facebook-github-bot
Copy link

Hi @wahern!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

1 similar comment
@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

…ating search patterns in combine.sh list_has_item
@Cyan4973
Copy link
Contributor

Cyan4973 commented Aug 11, 2022

Looks great !

cc @cwoffenden

@cwoffenden
Copy link
Contributor

This looks good!

(I'll give it a quick test on some old Unixes at the weekend)

@Cyan4973 Cyan4973 self-assigned this Aug 16, 2022
@Cyan4973 Cyan4973 merged commit 155d6a5 into facebook:dev Aug 16, 2022
@cwoffenden
Copy link
Contributor

cwoffenden commented Dec 18, 2022

Resurrecting this... I eventually got round to booting up an old Mac (Leopard on PPC) and out-of-the-box the shell script fails, but it's always failed on there because the grep implementation is broken (and I put a test for said old Mac to abort).

Anyway, replacing the system grep with the one from MacPorts (ggrep) runs and amalgamating the entire library takes 2m20, which is much better than the previous version (on the same machine the Python implementation runs in 600ms).

Part of the reason it still takes so long, at least on this machine, is because the resulting zstd.c is 5MB vs the approx 2MB of the Python version, due to paths not being canonically resolved (so lots of .. means files are multiply added).

I'm not sure it's worth the time to test for ggrep, since anyone installing MacPorts will have probably added Python, but I think it worth verifying on other systems that the amalgamated library isn't larger than needs be.

I think it breaks because of changes to how the headers are included. The Python version runs resolve() on every include, to make sure there's only one of each:

def resolve_include(file: str, parent: Optional[Path] = None) -> Optional[Path]:

(After writing all of this I feel a bug report would've been better...)

@Cyan4973
Copy link
Contributor

Indeed, a python dependency is acceptable for this task.
It's general enough to be present on most systems.
Plus, amalgamation doesn't have to happen on "target" systems, it's enough that one development system can do it.

@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants