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

[R-package] parallelize compilation in CMake-based builds #4525

Merged
merged 12 commits into from
Nov 10, 2021

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented Aug 15, 2021

Inspired by #4462, this proposes passing -j4 when building the R package with CMake, to parallelize compilation and make building faster.

Ran the following on my Mac to benchmark and found that this did make compilation noticeably faster.

# test this PR
time Rscript build_r.R
rm -r ./lightgbm_r

# test master
git checkout master
time Rscript build_r.R

this PR

real	1m45.165s
user	4m36.421s
sys	0m13.545s

master

real	3m17.079s
user	3m3.012s
sys	0m9.107s

Notes for Reviewers

CI should pass here once #4524 is merged.

@Laurae2
Copy link
Contributor

Laurae2 commented Aug 15, 2021

What happens on computers with fewer than 4 CPU thread? (cloud VMs, VPS, embedded devices)

In R I remember we didn't allow to use more than 1 thread to compile LightGBM by default (the user had to manually add a parameter to perform multithreaded compilation), as to not overload those machines (still common in businesses).

Previous solution 1, in a R configuration file:

MAKEFLAGS = -j4

Previous solution 2, in a prompt:

export MAKE="make -j8"

Previous solution 3, directly from R:

Sys.setenv(MAKE = "make -j4")

Is there a way to have something similar instead of an hardcoded 4 thread?

@jameslamb
Copy link
Collaborator Author

Is there a way to have something similar instead of an hardcoded 4 thread?

Sure, we could expose a command-line argument that you'd pass like this:

Rscript build_r.R \
    --parallel-build-jobs 4

Similar to the way that other customizations have been exposed as command-line flags.

LightGBM/build_r.R

Lines 44 to 51 in 86ead20

ARGS_TO_DEFINES <- c(
"--boost-root" = "-DBOOST_ROOT"
, "--boost-dir" = "-DBoost_DIR"
, "--boost-include-dir" = "-DBoost_INCLUDE_DIR"
, "--boost-librarydir" = "-DBOOST_LIBRARYDIR"
, "--opencl-include-dir" = "-DOpenCL_INCLUDE_DIR"
, "--opencl-library" = "-DOpenCL_LIBRARY"
)

Do you think that added complexity is worth it?

I remember we didn't allow to use more than 1 thread to compile LightGBM by default, as to not overload those machines

Is it your opinion that the changes in #4462 should be reverted and that such customization should be exposed in a command-line argument in the Python package too?

@Laurae2
Copy link
Contributor

Laurae2 commented Aug 23, 2021

Do you think that added complexity is worth it?

If this just requires adding -j parameter in the list you mentioned, yes (users wanting to compile explicitly in parallel). Otherwise, it would need to longer rework, and instead -j4 would be a temporary harmonious solution with what we have on the Python side.

In my opinion we should expose that parameter in the command line as make -j parameter was intended for the user to set it explicitly (because it linearly consumes more RAM to compile with more threads).

Copy link
Contributor

@Laurae2 Laurae2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be merged to temporarily get Pythion and R compilation behaving similarly for the number of threads used.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe passing args as a vector will be more correct, especially on Windows with processx

args Character vector, arguments to the command.
https://processx.r-lib.org/reference/run.html#arguments

R-package/src/install.libs.R Outdated Show resolved Hide resolved
R-package/src/install.libs.R Outdated Show resolved Hide resolved
R-package/src/install.libs.R Outdated Show resolved Hide resolved
@jameslamb
Copy link
Collaborator Author

Sorry for taking so long to get back to this! I'd like to not merge this, and take @Laurae2 's suggestion of allowing for Rscript build_r.R -j4.

@jameslamb jameslamb changed the title [R-package] parallelize compilation in CMake-based builds WIP: [R-package] parallelize compilation in CMake-based builds Nov 6, 2021
@jameslamb jameslamb force-pushed the feat/faster-r-cmake branch from 42f9a04 to d4a8854 Compare November 7, 2021 02:06
@jameslamb jameslamb changed the title WIP: [R-package] parallelize compilation in CMake-based builds [R-package] parallelize compilation in CMake-based builds Nov 7, 2021
@jameslamb
Copy link
Collaborator Author

jameslamb commented Nov 7, 2021

Ok I just updated this. build_r.R will now accept -j, and still defaults to single-threaded compilation.

Tested on my Mac, which has two physical CPUs and 4 logical CPUs (processor = "2.3 GHz Intel Core i5"). The results were what @Laurae2 said we should expect...going from single-threaded to double-threaded compilation gives a noticeable speedup, but beyond that, compilation doesn't get any faster.

time Rscript build_r.R -j1

# real    3m2.032s
# user    2m53.028s
# sys 0m6.873s

time Rscript build_r.R -j2

# real    1m54.311s
# user    3m14.167s
# sys 0m7.428s

time Rscript build_r.R -j4

# real    1m40.045s
# user    4m22.629s
# sys 0m9.230s

time Rscript build_r.R -j8

# real    1m38.459s
# user    4m16.870s
# sys 0m9.590s

@Laurae2 what do you think?

@@ -112,7 +112,7 @@ cd ${BUILD_DIRECTORY}
PKG_TARBALL="lightgbm_*.tar.gz"
LOG_FILE_NAME="lightgbm.Rcheck/00check.log"
if [[ $R_BUILD_TYPE == "cmake" ]]; then
Rscript build_r.R --skip-install || exit -1
Rscript build_r.R -j2 --skip-install || exit -1
Copy link
Collaborator

@StrikerRUS StrikerRUS Nov 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why j2? We use j4 in all our CI jobs, e.g.

make -j4 || exit -1

make _lightgbm -j4 || exit -1

Refer to #1745.

Also, from the PR description:

Inspired by #4462, this proposes passing -j4 when building the R package with CMake, to parallelize compilation and make building faster.
#4525 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on #4525 (comment) and the investigation I did last night (#4525 (comment)), I found that using more threads than you have available CPUs doesn't provide much speedup compared to using exactly as many threads as CPUs, but does lead to higher memory consumption.

According to https://docs.github.com/en/actions/using-github-hosted-runners/about-github-hosted-runners#supported-runners-and-hardware-resources, the VMs we get from GitHub Actions come with a single 2-core CPU. This is why I chose -j2.

All that said though, I think most of the benefit for CI comes from moving from single-threaded to multi-threaded, and the difference between -j2 and -j4 shouldn't be large in that environment. And I don't think the memory consumption for compilation is going to be a problem, given your point about all other jobs in this project's CI using -j4 already. I'll change these to -j4 for consistency with the other CI jobs.

.ci/test_r_package_windows.ps1 Show resolved Hide resolved
R-package/README.md Outdated Show resolved Hide resolved
Co-authored-by: Nikita Titov <[email protected]>
Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making -j flag publicly accessible.

@shiyu1994 shiyu1994 merged commit 7152c49 into master Nov 10, 2021
@StrikerRUS StrikerRUS deleted the feat/faster-r-cmake branch November 10, 2021 12:24
@StrikerRUS StrikerRUS mentioned this pull request Jan 6, 2022
13 tasks
@jameslamb jameslamb mentioned this pull request Oct 7, 2022
40 tasks
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants