-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] move more finalizer logic into C++ side to address memory leaks #4353
Conversation
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: failure ❌. |
Alright I still think this change is worth making, but it does not allow those skipped tests to pass
full logs (click me)
|
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: failure ❌. |
/gha run r-valgrind Workflow R valgrind tests has been triggered! 🚀 Status: success ✔️. |
I just ran another test in #4352 (comment). That looks at the
full logs (click me)
commit: 2714777 As of the change in this PR, Namely, the two findings below say "possibly lost" on
I know this is kind of complicated and confusing. My conclusion from this is:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Glad to see R-package is getting more "standard" and rich in terms of working with underlying cpp code 👍 .
Co-authored-by: Nikita Titov <[email protected]>
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. |
Contributes to #4282 (fixing memory leaks in the R package). Specifically, this addresses the following comment from that issue:
From https://cran.r-project.org/doc/manuals/r-release/R-exts.html#External-pointers-and-weak-references:
Some examples:
Changes in this PR
Booster
andDataset
objects at the C++ size, usingR_RegisterCFinalizerEx()
!lgb.is.null.handle()
in R6 objects'finalize()
calls, and moved that logic into the corresponding C++ functionsremoves uses oftestthat::skip()
on tests where valgrind detects memory leaks