GH-39517: [C++] Disable parallelism for jemalloc external project #39522

assignUser · 2024-01-09T00:18:49Z

Rationale for this change

With CMake > 3.28 the generated Makefile fails on the jemalloc_ep due to 'bad file descriptor'.

What changes are included in this PR?

For a sequential build for jemalloc by setting -j1.

Are these changes tested?

CI

Are there any user-facing changes?

No.

Closes: [R][C++] CMake 3.28.1 -GMake causes jemalloc subbuild to fail #39517

github-actions · 2024-01-09T00:19:19Z

⚠️ GitHub issue #39517 has been automatically assigned in GitHub to PR creator.

assignUser · 2024-01-09T00:19:55Z

@github-actions crossbow submit -g r

github-actions · 2024-01-09T00:22:24Z

Revision: 5f6051d

Submitted crossbow builds: ursacomputing/crossbow @ actions-e361533807

Task	Status
r-binary-packages
test-fedora-r-clang-sanitizer
test-r-arrow-backwards-compatibility
test-r-depsource-bundled
test-r-depsource-system
test-r-dev-duckdb
test-r-devdocs
test-r-gcc-11
test-r-gcc-12
test-r-install-local
test-r-install-local-minsizerel
test-r-library-r-base-latest
test-r-linux-as-cran
test-r-linux-rchk
test-r-linux-valgrind
test-r-minimal-build
test-r-offline-maximal
test-r-offline-minimal
test-r-rhub-debian-gcc-devel-lto-latest
test-r-rhub-debian-gcc-release-custom-ccache
test-r-rhub-ubuntu-gcc-release-latest
test-r-rstudio-r-base-4.1-opensuse153
test-r-rstudio-r-base-4.2-centos7-devtoolset-8
test-r-rstudio-r-base-4.2-focal
test-r-ubuntu-22.04
test-r-versions
test-ubuntu-r-sanitizer

assignUser · 2024-01-09T01:34:26Z

The r-binary-packages, devdocs, backwards comp and other builds where previously failing due to this issue and now pass.

kou · 2024-01-09T01:37:35Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

  set(JEMALLOC_BUILD_COMMAND ${MAKE} ${MAKE_BUILD_ARGS})
+  # Paralleism for Make fails with CMake > 3.28 see #39517
+  if(${CMAKE_GENERATOR} MATCHES "Makefiles")
+    list(APPEND JEMALLOC_BUILD_COMMAND "-j1")


Why do we want to use -j1 only for jemalloc?
It seems that this problem may be happen with other software. So how about setting MAKE_BUILD_ARGS?

The problem has not occurred with any other dependency neither on CI nor in my own testing (or after the fix). For the smaller dependencies it might not make a difference but with the number of larger dependencies we have forcing the all to use -j1 if there is no actual issue seems like it would slow build times with no gain?

It's not a strong opinion but I think that we should choose safer option (disable make's parallel build for all dependencies) here.
Because we have a workaround (use -GNinja) for the parallel build problem.

We only have 2 cores available on cran (policy), no ninja (afaik) and our build times are already being criticized so....

If we have only 2 cores, we should not use parallel build for dependencies.
If we use parallel build, it will use 2 more cores in total and slow down total build time...

FYI: We use make only for jemalloc, bzip2 and UCX.

kou · 2024-01-09T01:37:49Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  if(NOT ((_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_VERSION=")
+          OR (_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_CHECKSUM=")))


Why are they needed?

These were made by the cmake linter.

Wow. What CMake linter did you use? And what the error message from the CMake linter?
It seems that this diff changes matched pattern. (I think that it doesn't have any problem in our use case.)

archery lint --fix --cmake-format ^^ but I am happy to remove the change?

hm weird, it doesn't produce output if I leave the --fix flag off. I will revert the change to be safe.

kou

+1

kou · 2024-01-09T02:48:41Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

+  if(NOT ((_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_VERSION=")
+          OR (_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_CHECKSUM=")))


Wow. What CMake linter did you use? And what the error message from the CMake linter?
It seems that this diff changes matched pattern. (I think that it doesn't have any problem in our use case.)

kou · 2024-01-09T02:50:26Z

cpp/cmake_modules/ThirdpartyToolchain.cmake

  set(JEMALLOC_BUILD_COMMAND ${MAKE} ${MAKE_BUILD_ARGS})
+  # Paralleism for Make fails with CMake > 3.28 see #39517
+  if(${CMAKE_GENERATOR} MATCHES "Makefiles")
+    list(APPEND JEMALLOC_BUILD_COMMAND "-j1")


It's not a strong opinion but I think that we should choose safer option (disable make's parallel build for all dependencies) here.
Because we have a workaround (use -GNinja) for the parallel build problem.

This reverts commit 5f6051d.

…9522) ### Rationale for this change With CMake > 3.28 the generated Makefile fails on the jemalloc_ep due to 'bad file descriptor'. ### What changes are included in this PR? For a sequential build for jemalloc by setting -j1. ### Are these changes tested? CI ### Are there any user-facing changes? No. * Closes: #39517 Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

pitrou · 2024-01-09T09:44:54Z

Isn't this making builds slower? Can it be circumvented to only CMake 3.28?
Also, is an issue open with the CMake project?

…9522) ### Rationale for this change With CMake > 3.28 the generated Makefile fails on the jemalloc_ep due to 'bad file descriptor'. ### What changes are included in this PR? For a sequential build for jemalloc by setting -j1. ### Are these changes tested? CI ### Are there any user-facing changes? No. * Closes: #39517 Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

pitrou · 2024-01-10T13:15:02Z

Ping @raulcd @assignUser ^^

assignUser · 2024-01-10T13:34:42Z

Isn't this making builds slower?

Tiny bit maybe but unlikely as the jemalloc build in itself is pretty quick + we only set the one build to -j1 so the cmake can still build other objects/external prjects in parallel.

Can it be circumvented to only CMake 3.28?

That would be possible via https://cmake.org/cmake/help/latest/variable/CMAKE_VERSION.html but also it only applies when using the Make Generator and most dev/ci are likely using Ninja...

Also, is an issue open with the CMake project?

No but I have that as a todo. I haven't found a working reprex yet, I guess it needs the many parallel external projects we have...

pitrou · 2024-01-10T14:29:02Z

Tiny bit maybe but unlikely as the jemalloc build in itself is pretty quick + we only set the one build to -j1 so the cmake can still build other objects/external prjects in parallel.

My experience is that the jemalloc build blocks all other steps when building Arrow. I am not sure why that is (perhaps a bug in our dependency tree?); I tried outputting a dot file of the build graph using Ninja but I failed graphing the file afterwards...

conbench-apache-arrow · 2024-01-10T15:34:44Z

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 5acf67c.

There were 22 benchmark results indicating a performance regression:

Commit Run on ursa-i9-9960x at 2024-01-09 23:17:32Z
- file-read (R) with compression=uncompressed, dataset=fanniemae_2016Q4, file_type=feather, language=R, output_type=dataframe
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-20, scale_factor=1
and 20 more (see the report linked below)

The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.

assignUser · 2024-01-10T16:57:23Z

I build jemalloc with our release configure command and make vs make -j1:

./test.sh 56.47s user 7.29s system 102% cpu 1:02.35 total
./test.sh 57.22s user 7.05s system 103% cpu 1:02.16 total

So it seems that make defaults to a mostly single core build anyway, this just removes issues we had.

Explicitly passing -j16 to the jemalloc build interestingly also fixes the issue and does not show the warning mentioned in #2779. It might be enough to remove that change/conditional. I have tested this on ubuntu 20.04 with cmake 3.16 and it also works there. (I think cmake decoupled external projects from the main make by adding a cmake layer inbetween at some point prior to 3.16 thus getting rid of the sub-make issues).
I'll open a PR.

kou · 2024-01-10T20:11:11Z

My experience is that the jemalloc build blocks all other steps when building Arrow.

It seems that we solved this by GH-5281. (But only with Ninja.)

pitrou · 2024-01-10T20:35:37Z

Well, I use Ninja.

…ct (apache#39522) ### Rationale for this change With CMake > 3.28 the generated Makefile fails on the jemalloc_ep due to 'bad file descriptor'. ### What changes are included in this PR? For a sequential build for jemalloc by setting -j1. ### Are these changes tested? CI ### Are there any user-facing changes? No. * Closes: apache#39517 Authored-by: Jacob Wujciak-Jens <jacob@wujciak.de> Signed-off-by: Jacob Wujciak-Jens <jacob@wujciak.de>

assignUser added 2 commits January 9, 2024 01:16

add -j1 for -GMake

ef3b3b1

lint

5f6051d

assignUser added this to the 15.0.0 milestone Jan 9, 2024

github-actions bot added Component: C++ awaiting review Awaiting review labels Jan 9, 2024

assignUser requested a review from kou January 9, 2024 01:32

kou reviewed Jan 9, 2024

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Jan 9, 2024

assignUser requested a review from kou January 9, 2024 01:56

kou approved these changes Jan 9, 2024

View reviewed changes

github-actions bot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Jan 9, 2024

Revert "lint"

9be2dfe

This reverts commit 5f6051d.

github-actions bot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Jan 9, 2024

assignUser merged commit 5acf67c into apache:main Jan 9, 2024
30 of 31 checks passed

assignUser removed the awaiting changes Awaiting changes label Jan 9, 2024

assignUser mentioned this pull request Jan 10, 2024

[C++] Enable parallelism for Make based ExternalProjects #39549

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39517: [C++] Disable parallelism for jemalloc external project #39522

GH-39517: [C++] Disable parallelism for jemalloc external project #39522

assignUser commented Jan 9, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jan 9, 2024

assignUser commented Jan 9, 2024

github-actions bot commented Jan 9, 2024

assignUser commented Jan 9, 2024

kou Jan 9, 2024

assignUser Jan 9, 2024

kou Jan 9, 2024

assignUser Jan 9, 2024

kou Jan 9, 2024

kou Jan 9, 2024

kou Jan 9, 2024

assignUser Jan 9, 2024

kou Jan 9, 2024

assignUser Jan 9, 2024

assignUser Jan 9, 2024

kou left a comment

kou Jan 9, 2024

kou Jan 9, 2024

pitrou commented Jan 9, 2024

pitrou commented Jan 10, 2024

assignUser commented Jan 10, 2024

pitrou commented Jan 10, 2024

conbench-apache-arrow bot commented Jan 10, 2024

assignUser commented Jan 10, 2024 •

edited

Loading

kou commented Jan 10, 2024

pitrou commented Jan 10, 2024

		if(NOT ((_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_VERSION=")
		OR (_VERSION_ENTRY MATCHES "^[^ #][A-Za-z0-9-_]+_CHECKSUM=")))

GH-39517: [C++] Disable parallelism for jemalloc external project #39522

GH-39517: [C++] Disable parallelism for jemalloc external project #39522

Conversation

assignUser commented Jan 9, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 9, 2024

assignUser commented Jan 9, 2024

github-actions bot commented Jan 9, 2024

assignUser commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jan 9, 2024

pitrou commented Jan 10, 2024

assignUser commented Jan 10, 2024

pitrou commented Jan 10, 2024

conbench-apache-arrow bot commented Jan 10, 2024

assignUser commented Jan 10, 2024 • edited Loading

kou commented Jan 10, 2024

pitrou commented Jan 10, 2024

assignUser commented Jan 9, 2024 •

edited by github-actions bot

Loading

assignUser commented Jan 10, 2024 •

edited

Loading