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: pre-fill rest of detection values for Windows #12044

Closed
wants to merge 38 commits into from

Conversation

vszakats
Copy link
Member

@vszakats vszakats commented Oct 6, 2023

The goal of this patch is to avoid unnecessary feature detection work
when doing Windows builds with CMake. Do this by pre-filling well-known
detection results for Windows and specifically for mingw-w64 and MSVC
compilers. Also limit feature checks to platforms where the results are
actually used. Drop a few redundant ones. And some tidying up.

  • pre-fill remaining detection values in Windows CMake builds.

    Based on actual detection results observed in CI runs, preceding
    similar work over libssh2 and matching up values with
    lib/config-win32.h.

    This brings down CMake configuration time from 58 to 14 seconds on the
    same local machine.

    On AppVeyor CI this translates to:

    The formula is about 1-3 seconds delay for each detection. Almost all
    of these trigger a full compile-link cycle behind the scenes, slow
    even today, both cross and native, mingw-w64 and apparently MSVC too.
    Enabling .map files or other custom build features slows it down
    further. (Similar is expected for autotools configure.)

  • stop detecting idn2.h if idn2 was deselected.
    autotools does this.

  • stop detecting idn2.h if idn2 was not found.
    This deviates from autotools. Source code requires both header and
    lib, so this is still correct, but faster.

  • limit ADDRESS_FAMILY detection to Windows.

  • normalize HAVE_WIN32_WINNT value to lowercase 0x0a12 format.

  • pre-fill HAVE_WIN32_WINNT-dependent detection results.
    Saving 4 (slow) feature-detections in most builds: getaddrinfo,
    freeaddrinfo, inet_ntop, inet_pton

  • fix pre-filled HAVE_SYS_TIME_H, HAVE_SYS_PARAM_H,
    HAVE_GETTIMEOFDAY for mingw-w64.
    Luckily this do not change build results, as WIN32 took
    priority over HAVE_GETTIMEOFDAY with the current source
    code.

  • limit HAVE_CLOCK_GETTIME_MONOTONIC_RAW and
    HAVE_CLOCK_GETTIME_MONOTONIC detections to non-Windows.
    We're not using these in the source code for Windows.

  • reduce compiler warning noise in CMake internal logs:

    • fix to include winsock2.h before windows.h.
      Apply it to autotools test snippets too.
    • delete previous -D_WINSOCKAPI_= hack that aimed to fix the above.
    • cleanup CMake/CurlTests.c to emit less warnings.
  • delete redundant HAVE_MACRO_SIGSETJMP feature check.
    It was the same check as HAVE_SIGSETJMP.

  • delete "experimental" marking from CURL_USE_OPENSSL.

  • show CMake version via CMakeLists.txt.
    Credit to the zlib-ng project for the idea:
    https://github.com/zlib-ng/zlib-ng/blob/61e181c8ae93dbf56040336179c9954078bd1399/CMakeLists.txt#L7

  • make CMake/CurlTests.c pass checksrc.

  • CMake/WindowsCache.cmake tidy-ups.

  • replace WIN32 guard with _WIN32 in CMake/CurlTests.c.

Closes #12044


Clearer without whitespace changes: https://github.com/curl/curl/pull/12044/files?w=1

@vszakats vszakats added the cmake label Oct 6, 2023
@vszakats vszakats marked this pull request as draft October 6, 2023 00:48
@github-actions github-actions bot added the CI Continuous Integration label Oct 6, 2023
@vszakats vszakats changed the title cmake: tidy-up and Windows optimization cmake: pre-cache rest of detection values for Windows Oct 6, 2023
@vszakats
Copy link
Member Author

vszakats commented Oct 6, 2023

Also tested an experimental configure-vs-cmake feature-detection comparison
for macOS. Besides differently detected dependencies and some usual suspects,
they were identical, except #define CURL_CA_PATH "/etc/ssl/certs" which was
picked up by CMake, but missed by autotools:
https://github.com/curl/curl/actions/runs/6431509560/job/17464559513?pr=12044

macOS configure-vs-cmake patch:

diff --git a/.github/workflows/configure-vs-cmake.yml b/.github/workflows/configure-vs-cmake.yml
index 3131bc128..0f20801a3 100644
--- a/.github/workflows/configure-vs-cmake.yml
+++ b/.github/workflows/configure-vs-cmake.yml
@@ -27,7 +27,7 @@ on:
 permissions: {}
 
 jobs:
-  check:
+  check-linux:
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v4
@@ -39,7 +39,28 @@ jobs:
 
     - name: run cmake
       run: |
-         mkdir build && cd build && cmake ..
+         cmake -B build
+
+    - name: compare generated curl_config.h files
+      run: ./scripts/cmp-config.pl lib/curl_config.h build/lib/curl_config.h
+
+  check-macos:
+    runs-on: macos-latest
+    steps:
+    - uses: actions/checkout@v4
+
+    - name: install packages
+      run: |
+         while [[ $? == 0 ]]; do for i in 1 2 3; do brew update && brew install libtool autoconf automake && break 2 || { echo Error: wait to try again; sleep 10; } done; false Too many retries; done
+
+    - name: run configure --with-openssl
+      run: |
+         autoreconf -fi
+         ./configure --with-openssl
+
+    - name: run cmake
+      run: |
+         cmake -B build
 
     - name: compare generated curl_config.h files
       run: ./scripts/cmp-config.pl lib/curl_config.h build/lib/curl_config.h

@vszakats vszakats force-pushed the cmake-tidy-2 branch 2 times, most recently from d031a14 to c281b78 Compare October 6, 2023 15:30
@vszakats vszakats marked this pull request as ready for review October 6, 2023 15:34
@vszakats vszakats added the Windows Windows-specific label Oct 6, 2023
@vszakats vszakats force-pushed the cmake-tidy-2 branch 4 times, most recently from 282d4da to 1bf70e6 Compare October 7, 2023 14:57
@vszakats vszakats changed the title cmake: pre-cache rest of detection values for Windows cmake: pre-fill rest of detection values for Windows Oct 7, 2023
@vszakats vszakats force-pushed the cmake-tidy-2 branch 2 times, most recently from 4c9e89c to cb87638 Compare October 9, 2023 17:10
@vszakats vszakats added the feature-window A merge of this requires an open feature window label Oct 11, 2023
It's same check as `HAVE_SIGSETJMP`.
Comment suggested it is special, but it used the same method as all
the other symbol detections. It also suggested this was a secondary
detection attempt, but there was no first one.
Based on results found on AppVeyor CI with VS2008-VS2010-VS2022
and mingw-w64 6 to 9, origina mingw-w64 1.0 release and current
11.0 release.
```
        /usr/local/Cellar/mingw-w64/11.0.1/toolchain-x86_64/x86_64-w64-mingw32/include/winsock2.h:15:2: warning: #warning Please include winsock2.h before windows.h [-Wcpp]
           15 | #warning Please include winsock2.h before windows.h
              |  ^~~~~~~
```
This seems to actually cause the warnings, after fixing the header
include order to really include winsock2.h before windows.h:
```
        Building C object CMakeFiles/cmTC_0da7d.dir/HAVE_IDN2_H.c.obj
        /usr/local/bin/x86_64-w64-mingw32-gcc -D_WINSOCKAPI_="" -I/usr/local/include -W -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes
        In file included from .../curl/bld-cmake-win/CMakeFiles/CMakeScratch/TryCompile-7IYZyM/HAVE_IDN2_H.c:2:
        /usr/local/Cellar/mingw-w64/11.0.1/toolchain-x86_64/x86_64-w64-mingw32/include/winsock2.h:15:2: warning: #warning Please include winsock2.h before windows.h [-Wcpp]
           15 | #warning Please include winsock2.h before windows.h
              |  ^~~~~~~
        Linking C executable cmTC_0da7d.exe
```

After both fixes, the test is warning-free:
```
        Building C object CMakeFiles/cmTC_a2548.dir/HAVE_IDN2_H.c.obj
        /usr/local/bin/x86_64-w64-mingw32-gcc  -I/usr/local/include -W -Wall -pedantic -Wbad-function-cast -Wconversion -Winline -Wmissing-declarations -Wmissing-prototypes -Wnested-externs -Wno-long-long -Wno-multichar -Wpointer-arith -Wshadow -Wsign-compare -Wundef -Wunused -Wwrite-strings -Wcast-align -Wdeclaration-after-statement -Wempty-body -Wendif-labels -Wfloat-equal -Wignored-qualifiers -Wno-format-nonliteral -Wno-sign-conversion -Wno-system-headers -Wstrict-prototypes -Wtype-limits -Wvla -Wmissing-parameter-type -Wold-style-declaration -Wstrict-aliasing=3 -Wno-pedantic-ms-format -Wformat=2 -Warray-bounds=2 -ftree-vrp -Wduplicated-cond -Wnull-dereference -fdelete-null-pointer-checks -Wshift-negative-value -Wshift-overflow=2 -Walloc-zero -Wduplicated-branches -Wformat-overflow=2 -Wformat-truncation=1 -Wrestrict -Warith-conversion -Wdouble-promotion -Wenum-conversion -Wunused-const-variable  -o CMakeFiles/cmTC_a2548.dir/HAVE_IDN2_H.c.obj -c .../curl/bld-cmake-win/CMakeFiles/CMakeScratch/TryCompile-7JKzOa/HAVE_IDN2_H.c
        Linking C executable cmTC_a2548.exe
```
This breaks from autotools. autotools will need to be adapted
to avoid the unnecessary header check in case the lib is missing.

The curl source bits that use idn2 require BOTH to be detected.
Not ruling out a terrible custom mis-configuration that may end up
with both WIN32 and UNIX set, but this was the only file we did
consider this scenario, so it seems fairly unlikely.

When building for Cygwin or MSYS2, WIN32 is NOT set and UNIX is set.
`CMAKE_VERSION` supported since CMake 2.6.3
Move it before including Windows headers.
`NOT UNIX` / `WIN32` / `HAVE_WINDOWS_H` / `HAVE_WIN*_H` are used
interchangeably in the source. In practice these all mean the same
thing and logic expects them to be in sync.

We should end up using `WIN32` only in CMake sources to check for
Windows. And `_WIN32` in C sources.

There is Windows C compiler supported that seems to miss Windows
socket header: `__SALFORDC__`. This C compiler seems to be long
gone. If still around, it must support Windows sockets out of the
box.
i.e. use windows.h directly, instead via `lib/setup-win32.h`.

This makes it work without requiring `HAVE_WINDOWS_H` defined.
…ngw-w64

The pre-cached values for CMake and the `config-win32.h` ones were
wrongly set as unsupported.

mingw-w64 supported these since its 1.0 release.

autotools got them right.

I did not catch these when doing cross-build-tool reproducibility tests
for Windows. Reason being that the `gettimeofday` branch is overridden
by a `WIN32` one, so it's not used regardless of these settings.

It means that this fix doesn't change the build results. Two new headers
_will_ be included though and the detection results are correctly setup
now.
This is present in mingw-w64 on native Windows by default,
otherwise it's only detected when manually adding pthread lib.

But, the result of this feature check is never used on Windows
by the source code, because the `WIN32` branches override it.
It'd also make curl phtread dependent if it did use it.
Always overridden by `WIN32` branches in curl source code.
Otherwise it needs pthread and a constant that is not present
in mingw-w64 anyway.
Possibly clang-cl might support this, which might show up as MSVC
in CMake. I'm not sure, so revert.

This reverts commit 761361f.
@vszakats vszakats closed this in 2100d9f Oct 24, 2023
@vszakats vszakats deleted the cmake-tidy-2 branch October 24, 2023 21:08
@vszakats vszakats removed the feature-window A merge of this requires an open feature window label Oct 25, 2023
vszakats added a commit to curl/curl-for-win that referenced this pull request Oct 25, 2023
Switch from `curl-gnumake.sh` to `curl-cmake.sh` with upcoming curl
release v8.5.0.

cmake builds are now _faster_ for Windows builds than raw gnumake
(aka `Makefile.mk`). They also use 'unity' mode, which makes builds
run fast with the side-effect of also creating potentially more
efficient binaries by allowing better compiler optimizations.

This also makes curl-for-win use the same build system for all target
platforms (`Makefile.mk` is not available for *nix platforms).

Initially on 2022-07-04 cmake was 25% slower than gnumake. By
2022-09-26 this reduced to 20%, by 2023-07-29 to 18% and after the
latest round of updates gnumake came out 7% slower than cmake.
This is for Windows, for a triple x64, arm64 and x86 build. In
absolute times this is 27m22s for cmake and 29m11s for gnumake.

(FWIW autotools builds are 52% slower than cmake unity builds now.)

Making cmake builds fast was a multi-year effort with these major steps:

1. add support for cmake builds in curl-for-win.
   420e73c
2. bring it in-line with gnumake and autotools builds and tidy-up
   as much as possible. Scattered to a many commits.
3. delete a whole bunch of unused feature detections.
   curl/curl@4d73854
   curl/curl#9044
   (and a lot more smaller commits)
4. speed up picky warning option initialization by avoiding brute-force
   all options. (first in libssh2, then in curl, then applied also
   ngtcp2, nghttp3, nghttp2)
   curl/curl@9c543de
   curl/curl#10973
5. implement single build run for shared and static libcurl + tool
   (first in libssh2)
   curl/curl@1199308
   curl/curl#11505
   53dcd49
6. implement single build pass for shared and static libcurl
   (for Windows initially)
   curl/curl@2ebc74c
   curl/curl#11546
7. extend above to non-Windows (e.g. macOS)
   curl/curl@fc9bfb1
   curl/curl#11627
   bafa77d (mac)
   1b27304 (linux)
8. implement unity builds: single compiler invocation for libcurl + tool
   curl/curl@3f8fc25
   curl/curl#11095
   curl/curl@f42a279
   curl/curl#11928
   67d7fd3
9. speed up 4x the cmake initialization phase (for Windows)
   curl/curl@2100d9f
   curl/curl#12044
10. speed up cmake initialization even more by pre-filling
   detection results for our well-known mingw-w64 environments.
   74dd967
   5a43c61

+1: speed up autotools initialization by mitigating a brute-force
   feature detection on Windows. This reduced total build times
   by 5 minutes at the time, for the 3 Windows targets in total.
   curl/curl@6adcff6
   curl/curl#9591

Also update build times for cmake-unity and gnumake, based on runs:
cmake-unity: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48357875
gnumake: https://ci.appveyor.com/project/curlorg/curl-for-win/builds/48358005
zuoxiaofeng pushed a commit to zuoxiaofeng/curl that referenced this pull request Nov 28, 2023
The goal of this patch is to avoid unnecessary feature detection work
when doing Windows builds with CMake. Do this by pre-filling well-known
detection results for Windows and specifically for mingw-w64 and MSVC
compilers. Also limit feature checks to platforms where the results are
actually used. Drop a few redundant ones. And some tidying up.

- pre-fill remaining detection values in Windows CMake builds.

  Based on actual detection results observed in CI runs, preceding
  similar work over libssh2 and matching up values with
  `lib/config-win32.h`.

  This brings down CMake configuration time from 58 to 14 seconds on the
  same local machine.

  On AppVeyor CI this translates to:
  - 128 seconds -> 50 seconds VS2022 MSVC with OpenSSL (per CMake job):
    https://ci.appveyor.com/project/curlorg/curl/builds/48208419/job/4gw66ecrjpy7necb#L296
    https://ci.appveyor.com/project/curlorg/curl/builds/48217440/job/8m4fwrr2fe249uo8#L186
  - 62 seconds -> 16 seconds VS2017 MINGW (per CMake job):
    https://ci.appveyor.com/project/curlorg/curl/builds/48208419/job/s1y8q5ivlcs7ub29?fullLog=true#L290
    https://ci.appveyor.com/project/curlorg/curl/builds/48217440/job/pchpxyjsyc9kl13a?fullLog=true#L194

  The formula is about 1-3 seconds delay for each detection. Almost all
  of these trigger a full compile-link cycle behind the scenes, slow
  even today, both cross and native, mingw-w64 and apparently MSVC too.
  Enabling .map files or other custom build features slows it down
  further. (Similar is expected for autotools configure.)

- stop detecting `idn2.h` if idn2 was deselected.
  autotools does this.

- stop detecting `idn2.h` if idn2 was not found.
  This deviates from autotools. Source code requires both header and
  lib, so this is still correct, but faster.

- limit `ADDRESS_FAMILY` detection to Windows.

- normalize `HAVE_WIN32_WINNT` value to lowercase `0x0a12` format.

- pre-fill `HAVE_WIN32_WINNT`-dependent detection results.
  Saving 4 (slow) feature-detections in most builds: `getaddrinfo`,
  `freeaddrinfo`, `inet_ntop`, `inet_pton`

- fix pre-filled `HAVE_SYS_TIME_H`, `HAVE_SYS_PARAM_H`,
  `HAVE_GETTIMEOFDAY` for mingw-w64.
  Luckily this do not change build results, as `WIN32` took
  priority over `HAVE_GETTIMEOFDAY` with the current source
  code.

- limit `HAVE_CLOCK_GETTIME_MONOTONIC_RAW` and
  `HAVE_CLOCK_GETTIME_MONOTONIC` detections to non-Windows.
  We're not using these in the source code for Windows.

- reduce compiler warning noise in CMake internal logs:
  - fix to include `winsock2.h` before `windows.h`.
    Apply it to autotools test snippets too.
  - delete previous `-D_WINSOCKAPI_=` hack that aimed to fix the above.
  - cleanup `CMake/CurlTests.c` to emit less warnings.

- delete redundant `HAVE_MACRO_SIGSETJMP` feature check.
  It was the same check as `HAVE_SIGSETJMP`.

- delete 'experimental' marking from `CURL_USE_OPENSSL`.

- show CMake version via `CMakeLists.txt`.
  Credit to the `zlib-ng` project for the idea:
  https://github.com/zlib-ng/zlib-ng/blob/61e181c8ae93dbf56040336179c9954078bd1399/CMakeLists.txt#L7

- make `CMake/CurlTests.c` pass `checksrc`.

- `CMake/WindowsCache.cmake` tidy-ups.

- replace `WIN32` guard with `_WIN32` in `CMake/CurlTests.c`.

Closes curl#12044
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build CI Continuous Integration cmake performance Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

2 participants