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] remove pre-allocated call_state in C++ calls #4244

Merged
merged 1 commit into from
May 1, 2021

Conversation

jameslamb
Copy link
Collaborator

@jameslamb jameslamb commented May 1, 2021

This is another step towards #3016.

As of #4155 and #4163, the R package use .Call() to call routines from the C++ library. Currently, an integer called call_state is initialized on the R side before each such call, its value is manipulated during the call, and its value is returned by each call. Initializing this integer and passing it around is unnecessary, because nothing is done with it on the R side after the call completes. All uses of .Call() in the R package (except in calls to LGBM_GetLastError_R) modify their inputs by reference and do not return anything.

This PR proposes simplifying the R package and removing unnecessary memory usage and computation by removing the call_state concept. Instead, the functions defined in https://github.com/microsoft/LightGBM/blob/b27dcfa48f76e77c9e5033dcdefc1f3a76d99c8b/R-package/src/lightgbm_R.h are converted to return a SEXP (R's built-in type for passing data between R and C). This is initialized on the C++ side.

Notes for Reviewers

If you want to learn more about the R SEXP object, see https://cran.r-project.org/doc/manuals/r-release/R-ints.html#SEXPs.

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 1, 2021

/gha run r-solaris

Workflow Solaris CRAN check has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/801388473

solaris-x86-patched: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-2df821005e34470eba4e33f7daef7e47
solaris-x86-patched-ods: https://builder.r-hub.io/status/lightgbm_3.2.1.99.tar.gz-a59dff6f8a754b21a78f5ad209e13c5b
Reports also have been sent to LightGBM public e-mail: http://www.yopmail.com/lightgbm_rhub_checks
Status: success ✔️.

@jameslamb
Copy link
Collaborator Author

jameslamb commented May 1, 2021

/gha run r-valgrind

Workflow R valgrind tests has been triggered! 🚀
https://github.com/microsoft/LightGBM/actions/runs/801390521

Status: success ✔️.

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.

Great!

@jameslamb jameslamb merged commit 26cde5f into microsoft:master May 1, 2021
@jameslamb jameslamb deleted the r/remove-call-state branch May 1, 2021 19:49
@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.

2 participants