Skip to content

CI: unify the way to set parallel build #1090

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

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

illwieckz
Copy link
Member

Let's see if it works for AppVeyor/MSVC, and if the same builder-agnostic method also works on other systems.

@illwieckz
Copy link
Member Author

It doesn't seem to be faster on AppVeyor, maybe there is only 1 core?

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch 3 times, most recently from e92b887 to 61d5708 Compare April 21, 2024 13:31
@illwieckz
Copy link
Member Author

Appveyor allocates only 2 cores:

set CMAKE_BUILD_PARALLEL_LEVEL=%NUMBER_OF_PROCESSORS%
echo %CMAKE_BUILD_PARALLEL_LEVEL%
2

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch 2 times, most recently from 1b3d2a8 to aa204b0 Compare April 21, 2024 13:49
@illwieckz
Copy link
Member Author

It doesn't seem to be faster on AppVeyor.

On macOS I had to set the variable with export to get it working, but there is no export on Windows.

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch 2 times, most recently from 21622f1 to 03098ee Compare April 21, 2024 14:17
@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from 03098ee to 9ac667a Compare April 21, 2024 14:30
@illwieckz
Copy link
Member Author

All I save with Appveyor/MSVC is 30s… 😐️
We basically go from 8m40 to 8m10 for a single build.
It's always welcome, but I as hoping more.

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from 9ac667a to 81ecf06 Compare April 21, 2024 15:11
@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from 81ecf06 to ea18aea Compare April 21, 2024 15:25
@illwieckz
Copy link
Member Author

I moved out of the appveyor script the commenst, to avoid those errors:

The system cannot find the file specified.

Both the standard REM remark (comment) command and the label-hijacked-hack :: produce this error.

@illwieckz
Copy link
Member Author

The idea of overloading a bit the CPU cores is that it usually allows to save some time because both the compiler and the build system (CMake, make, etc.) may not use 100% of the CPU when doing something. This usually works with 1 or 2 more jobs than cpu cores (overloading too much may put the memory and the scheduler on pressure and start being counter-productive).

@slipher
Copy link
Member

slipher commented Apr 21, 2024

If the CMake parallelism turns out to do anything, it would be nicer to use the --parallel option of cmake --build rather than an environment.

We already insert /MP in MSVC builds because it is needed to make Visual Studio build in parallel so I wouldn't expect adding it again to change anything.

@slipher
Copy link
Member

slipher commented Apr 21, 2024

NMake apparently doesn't have a -j equivalent so that part will probably be a no-op from CMake as well. https://stackoverflow.com/questions/601970/how-do-i-utilise-all-the-cores-for-nmake

@illwieckz
Copy link
Member Author

We already insert /MP in MSVC builds because it is needed to make Visual Studio build in parallel so I wouldn't expect adding it again to change anything.

Ah right, that's why I see so fee differences.

If the CMake parallelism turns out to do anything

It looks like it helps at doing cmake things in parallel like generating the .h from glsl files while building other things.

@slipher
Copy link
Member

slipher commented Apr 21, 2024

If the CMake parallelism turns out to do anything

It looks like it helps at doing cmake things in parallel like generating the .h from glsl files while building other things.

I don't think so, AFAIK CMake is strictly single-threaded. The way this is expected to work is that the generated build system (Make/VS/etc.) shells out to CMake script mode each time it generates one of those files. So the generated build system controls the parallelism.

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from ea18aea to 7783044 Compare April 21, 2024 19:04
@illwieckz
Copy link
Member Author

Whatever what is parallelized, I got this here:

  dummyapp.vcxproj -> C:\projects\daemon\build\Release\dummyapp.exe
  files.cpp
  huffman.cpp
  msg.cpp
  net_chan.cpp
  Generating embed_data/vertexSkinning_vp.glsl.h
  net_ip.cpp
  gmock.vcxproj -> C:\projects\daemon\build\lib\Release\gmock.lib
  translation.cpp
  sv_bot.cpp
  Generating embed_data/vertexSprite_vp.glsl.h
  sv_ccmds.cpp
  sv_client.cpp
  sv_init.cpp
  sv_main.cpp
  sv_net_chan.cpp
  sv_sgame.cpp
  sv_snapshot.cpp
  CryptoChallenge.cpp
  Generating embed_data/blurX_fp.glsl.h
  NullKeyboard.cpp
  null_client.cpp
  null_input.cpp
  Building Custom Rule C:/projects/daemon/CMakeLists.txt

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from 7783044 to 5161deb Compare April 21, 2024 19:10
@slipher
Copy link
Member

slipher commented Apr 21, 2024

I was wrong to say the generator is NMake. Nmake is used for the NaCl secondary build when part of a Visual Studio build, but we don't test that on CI. So the generator is Visual Studio, the same as for the IDE project.

What I said about /MP is still true though, so I doubt there are any improvements to be had.

@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from 5161deb to e75ad61 Compare June 17, 2024 00:15
@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch from e75ad61 to c560ac9 Compare June 17, 2024 00:23
@illwieckz illwieckz force-pushed the illwieckz/ci-parallel-build branch 14 times, most recently from e85c670 to 3d225bb Compare June 17, 2024 07:26
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

Successfully merging this pull request may close these issues.

2 participants