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

cmake: speed up zstd detection #12200

Closed
wants to merge 2 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 25, 2023

Before this patch we detected the presence of a specific zstd API to
see if we can use the library. zstd published that API in its first
stable release: v1.0.0 (2016-08-31).

Replace that method by detecting the zstd library version instead and
accepting if it's v1.0.0 or newer. Also display this detected version
and display a warning if the zstd found is unfit for curl.

We use the same version detection method as zstd itself, via its public
C header.

This deviates from autotools which keeps using the slow method of
looking for the API by building a test program. The outcome is the same
as long as zstd keeps offering this API.

Ref: facebook/zstd@5a0c8e2 (2016-08-12, committed)
Ref: https://github.com/facebook/zstd/releases/tag/v0.8.1 (2016-08-18, first released)
Ref: https://github.com/facebook/zstd/releases/tag/v1.0.0

Closes #12200


-- Checking for one of the modules 'libzstd'
-- Found Zstd: .../zstd/lib/libzstd.a (found version "1.5.5") 

Before this patch we detected the presence of a specific zstd API to
see if we can use the library. zstd introduced that API in its first
stable release: v1.0.0.

Replace that method by detecting the zstd library version instead and
accepting if it's v1.0.0 or newer. Also display this detected version.

We use the same version detection method as zstd itself, using its
public C header.

This deviates from autotools which keeps using the slow method of
looking for the API by building a test program. The outcome is the same
as long as zstd keeps offering this API.

Closes #xxxxx
@vszakats vszakats added the cmake label Oct 25, 2023
@bagder
Copy link
Member

bagder commented Oct 26, 2023

This deviates from autotools which keeps using the slow method of
looking for the API by building a test program

Right, but that's how configure always detects symbols/functions. It makes sure we can link with them. I don't think that is problematic.

On my Linux dev machine, a configure at current master run that detects a lot of third party components including HTTP/3, zstd, brotli and more, configure completes in less than 7 seconds.

A bare "cmake" run at the same commit on the same machine takes 0.8 seconds longer while detecting fewer 3rd party libs.

@vszakats
Copy link
Member Author

vszakats commented Oct 26, 2023

On my machine and on most CI machines, configure takes a lot more than 7 seconds. It's 82 seconds for a Windows cross-build on macOS (*) for example and 52 seconds for pure macOS (without running autoreconf). It's so slow that it makes it in practice unusable for local (and CI) iterations involving the build system.

For curl-for-win specifically, this slow configuration stage needs to run twice. This performance has been traditionally the case with all Windows (and apparently macOS) machines and Windows cross-builds, as opposed to pure Linux where the necessary operations are fast. Surely with a bleeding edge computer this could also be made faster, but those are not even an option in free CI tiers.

I'm not attempting to make autotools fast, because I reckon its mojo is to do and redo all the detection work on every run. This also has a valid case on *nix systems that have wide and unpredicable variations on what's available (also true for CMake).

But, in many cases, these detections are repeating the exact same work with predictable results. Speaking of Windows, almost all of these results are well-known. Same case with zstd: the result is the function of its version (which in this case means ALL existing stable releases), and detecting the version is much faster than triaging a full build. I understand this might not catch issues where zstd cannot be linked for other reasons (say a broken/incompatible zstd build), or a broken toolchain setup, but these are rather rare. Also, toolchain issues are caught earlier anyway and even a broken zstd is caught at link stage latest.

I think this is safe. I can fast-track this manually for "my" builds, but why not make all builds benefit?

(*) To put into perspective, with CMake, with current master and unity mode, the complete curl build takes 30 seconds (11 seconds configure + 19 seconds build). Each detection (such as ZSTD_createDStream) adds 1 to 3 seconds to the configure stage.

@vszakats
Copy link
Member Author

vszakats commented Oct 26, 2023

The zstd feature check was added for Ubuntu 16.04 LTS Xenial, according to #5453 (comment). Though Xenial is still officially supported till 2026, Ubuntu's package search page no longer supports it. According to one source, it came with v0.5.1, according to another, with v1.3.1. Either way, version detection works with v0.5.1:

-- Checking for one of the modules 'libzstd'
-- Found Zstd: .../zstd-0.5.1/lib/pkg/usr/local/lib/libzstd.a (found version "0.5.1") 

(That said it might be a nice touch to display a warning or error if zstd is not fit for curl.) ✅

@vszakats vszakats closed this in c5d506e Oct 27, 2023
@vszakats vszakats deleted the cmake-zstd-faster-detection branch October 27, 2023 00:39
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 27, 2023
Stop pre-filling values that are no longer used by the next curl
release.

Ref:
curl/curl#12200 `HAVE_ZSTD_CREATEDSTREAM`
curl/curl#12202 `THREADS_HAVE_PTHREAD_ARG` `CMAKE_HAVE_LIBC_PTHREAD` `CMAKE_HAVE_PTHREADS_CREATE` `CMAKE_HAVE_PTHREAD_CREATE`
curl/curl#12210 `DHAVE_VARIADIC_MACROS_C99` `HAVE_VARIADIC_MACROS_GCC`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants