[ci] [R-package] Fix memory leaks found by valgrind #3443

jameslamb · 2020-10-08T03:55:41Z

In #3338 , {lightgbm} was rejected from CRAN for multiple failed submission attempts. As of that submission, the package was failing tests with a version of R instrumented to use valgrind.

https://cran-archive.r-project.org/web/checks/2020/2020-10-02_check_results_lightgbm.html

This PR attempts to add a CI job that replicates these checks.

Notes

This is a work in progress. I'll add reviewers when I think it's ready.
Once I confirm that the job fails as it should, I'll try merging in the changes from https://github.com/microsoft/LightGBM/tree/fix-memory-leakage, which might fix the issues! Then we can develop on this branch until those issues are fixed
If the job takes too long, I'll switch it to an optional job triggered by a comment (like CI CUDA job #3402 )

jameslamb · 2020-10-08T12:52:00Z

Ok, I can see a few things from the first run (https://github.com/microsoft/LightGBM/pull/3443/checks?check_run_id=1224028806)

the issues with valgrind are successfully reproduced by this job!

==1312== Memcheck, a memory error detector
==1312== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==1312== Using Valgrind-3.15.0 and LibVEX; rerun with -h for copyright info
==1312== Command: /usr/local/RDvalgrind/lib/R/bin/exec/R --no-readline --vanilla -f testthat.R
==1312== 
Loading required package: R6
==1312== Invalid write of size 8
==1312==    at 0x154571E0: LGBM_BoosterGetNumPredict_R (lightgbm_R.cpp:509)
==1312==    by 0x4941B8C: R_doDotCall (dotcode.c:607)
==1312==    by 0x494CCC6: do_dotcall (dotcode.c:1281)
==1312==    by 0x499FB01: bcEval (eval.c:7078)
==1312==    by 0x498B67F: Rf_eval (eval.c:727)
==1312==    by 0x498E414: R_execClosure (eval.c:1895)
==1312==    by 0x498E0C7: Rf_applyClosure (eval.c:1821)
==1312==    by 0x498BE7A: Rf_eval (eval.c:850)
==1312==    by 0x49920BF: do_set (eval.c:2967)
==1312==    by 0x498BAD6: Rf_eval (eval.c:802)
==1312==    by 0x4990B42: do_begin (eval.c:2515)
==1312==    by 0x498BAD6: Rf_eval (eval.c:802)
==1312==  Address 0xf8a70d0 is 6,832 bytes inside a block of size 7,960 alloc'd
==1312==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==1312==    by 0x49E1CDB: GetNewPage (memory.c:946)
==1312==    by 0x49F102E: Rf_allocVector3 (memory.c:2784)
==1312==    by 0x49D4EE1: Rf_allocVector (Rinlinedfuns.h:593)
==1312==    by 0x49631FC: duplicate1 (duplicate.c:336)
==1312==    by 0x4962871: duplicate_child (duplicate.c:206)
==1312==    by 0x4962AEA: duplicate_list (duplicate.c:268)
==1312==    by 0x4962F0D: duplicate1 (duplicate.c:312)
==1312==    by 0x4962DAD: duplicate1 (duplicate.c:305)
==1312==    by 0x49627A1: Rf_duplicate (duplicate.c:139)
==1312==    by 0x49D32FB: R_compute_identical (identical.c:107)
==1312==    by 0x49D31D5: do_identical (identical.c:76)
==1312== 
==1312== Conditional jump or move depends on uninitialised value(s)
==1312==    at 0x49CF138: gregexpr_Regexc (grep.c:2439)
==1312==    by 0x49D1F13: do_regexpr (grep.c:3100)
==1312==    by 0x49A0058: bcEval (eval.c:7121)
==1312==    by 0x498B67F: Rf_eval (eval.c:727)
==1312==    by 0x498E414: R_execClosure (eval.c:1895)
==1312==    by 0x498E0C7: Rf_applyClosure (eval.c:1821)
==1312==    by 0x499FC8C: bcEval (eval.c:7089)
==1312==    by 0x498B67F: Rf_eval (eval.c:727)
==1312==    by 0x498B1CB: forcePromise (eval.c:555)
==1312==    by 0x49963AB: FORCE_PROMISE (eval.c:5142)
==1312==    by 0x4996566: getvar (eval.c:5183)
==1312==    by 0x499D1A5: bcEval (eval.c:6873)
==1312== 
==1312== 
==1312== HEAP SUMMARY:
==1312==     in use at exit: 274,972,869 bytes in 54,848 blocks
==1312==   total heap usage: 2,454,254 allocs, 2,399,406 frees, 5,660,846,940 bytes allocated
==1312== 
==1312== LEAK SUMMARY:
==1312==    definitely lost: 1,308,789 bytes in 7,847 blocks
==1312==    indirectly lost: 1,102 bytes in 2 blocks
==1312==      possibly lost: 9,768 bytes in 90 blocks
==1312==    still reachable: 273,653,210 bytes in 46,909 blocks
==1312==                       of which reachable via heuristic:
==1312==                         newarray           : 4,264 bytes in 1 blocks
==1312==         suppressed: 0 bytes in 0 blocks
==1312== Rerun with --leak-check=full to see details of leaked memory
==1312== 
==1312== Use --track-origins=yes to see where uninitialised values come from
==1312== For lists of detected and suppressed errors, rerun with: -s
==1312== ERROR SUMMARY: 6 errors from 2 contexts (suppressed: 0 from 0)

the job took 40+ minutes to run
despite these failures, the script I introduced did not actually cause an ❌ in CI

I'll try some fixes tonight

guolinke · 2020-10-09T08:03:58Z

@jameslamb you can simply try this:

  // R_INT64_PTR(out)[0] = len;
  R_INT_PTR(out)[0] = static_cast<int>(len);

jameslamb · 2020-10-10T23:20:51Z

@jameslamb you can simply try this:

  // R_INT64_PTR(out)[0] = len;
  R_INT_PTR(out)[0] = static_cast<int>(len);

thanks! to be clear, I know how to get valgrind to report errors, I just must have made some mistakes in getting the CI job to actually fail. I'll figure it out, I'm sure it was something simple

UPDATE: nevermind, I read too fast. Now I see you meant "as a way to fix the issue in LGBM_BoosterGetNumPredict_R". Thanks!

jameslamb · 2020-10-11T00:18:53Z

ok as of e80b68d, I got the valgrind test to fail!!!

Notice in https://github.com/microsoft/LightGBM/pull/3443/checks?check_run_id=1236732464 that all other R tests are passing.

Now that I know this test catches what we want, next I'm going to merge in the fix-memory-leakage branch. We can now work on fixing all valgrind errors here.

Once the test is passing, I'll move it to one that is triggered by a comment (not run on every build), since it takes about 30m to run.

guolinke · 2020-10-11T01:18:11Z

it seems the error is not related to lightgbm,

==1312== Conditional jump or move depends on uninitialised value(s)
==1312==    at 0x49CF138: gregexpr_Regexc (grep.c:2439)
==1312==    by 0x49D1F13: do_regexpr (grep.c:3100)
==1312==    by 0x49A0058: bcEval (eval.c:7121)
==1312==    by 0x498B67F: Rf_eval (eval.c:727)
==1312==    by 0x498E414: R_execClosure (eval.c:1895)
==1312==    by 0x498E0C7: Rf_applyClosure (eval.c:1821)
==1312==    by 0x499FC8C: bcEval (eval.c:7089)
==1312==    by 0x498B67F: Rf_eval (eval.c:727)
==1312==    by 0x498B1CB: forcePromise (eval.c:555)
==1312==    by 0x49963AB: FORCE_PROMISE (eval.c:5142)
==1312==    by 0x4996566: getvar (eval.c:5183)
==1312==    by 0x499D1A5: bcEval (eval.c:6873)

can we fix them?

guolinke

these shrinkage_to_fit is not efficiency, let us try to remove them.

R-package/src/lightgbm_R.cpp

src/boosting/gbdt_model_text.cpp

…to ci/r-valgrind-test

jameslamb · 2020-10-17T05:36:19Z

@jameslamb

two valgrind errors were caused by tests that used testthat::expect_error(). I've added testthat::skip() statements to these so they will not be a problem on CRAN. Both errors were small memory leaks, not more serious problems like illegal writes, so I think it is ok to skip them for now to get to CRAN, and come back to them later when we have time

Could you please create new issue for this task to not lose it?

added #3462

jameslamb · 2020-10-17T05:47:43Z

@jameslamb Looks like we have a "chocolate" issue again:

...
-- Check for working C compiler: C:/Rtools/mingw_64/bin/gcc.exe - broken
...
Run Build Command(s):C:/ProgramData/chocolatey/bin/mingw32-make.exe cmTC_48deb/fast && C:/ProgramData/chocolatey/lib/mingw/tools/install/mingw64/bin/mingw32-make  -f CMakeFiles\cmTC_48deb.dir\build.make CMakeFiles/cmTC_48deb.dir/build

I guess make should come from Rtools not from chocolatey...

Can you please point me to a log where you see this? I looked through all the Windows jobs and couldn't find that, maybe I'm missing something.

StrikerRUS · 2020-10-17T16:20:43Z

@jameslamb

Can you please point me to a log where you see this? I looked through all the Windows jobs and couldn't find that, maybe I'm missing something.

Sure, I can! Here it is: https://github.com/microsoft/LightGBM/runs/1264985038?check_suite_focus=true.

Also, please take a look at master builds. Looks like we have a false positive for WARNINGS:

Warning: package 'R6' was built under R version 4.0.3

StrikerRUS · 2020-10-17T16:26:56Z

Looks like we have a false positive for WARNINGS:

I believe we need the same quick fix: #3225. Can we add that warning message (Warning: package * was built under R version *) into whitelist to not block our development process with each R patch version release?

StrikerRUS

@jameslamb Thanks a lot for addressing review comments! LGTM, except one lint error about line length and some other nits.Feel free to merge after green CI.

.github/workflows/r_valgrind.yml

R-package/tests/testthat/test_utils.R

jameslamb · 2020-10-17T22:11:27Z

Looks like we have a false positive for WARNINGS:

I believe we need the same quick fix: #3225. Can we add that warning message (Warning: package * was built under R version *) into whitelist to not block our development process with each R patch version release?

yep I just noticed that too. Will do it in a separate PR right now

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

…to ci/r-valgrind-test

.github/workflows/r_valgrind.yml

jameslamb · 2020-10-18T03:59:17Z

@jameslamb Looks like we have a "chocolate" issue again:

...
-- Check for working C compiler: C:/Rtools/mingw_64/bin/gcc.exe - broken
...
Run Build Command(s):C:/ProgramData/chocolatey/bin/mingw32-make.exe cmTC_48deb/fast && C:/ProgramData/chocolatey/lib/mingw/tools/install/mingw64/bin/mingw32-make  -f CMakeFiles\cmTC_48deb.dir\build.make CMakeFiles/cmTC_48deb.dir/build

I guess make should come from Rtools not from chocolatey...

I opened #3469 , let's move this discussion there. Thanks for letting me know! I would have missed it.

jameslamb · 2020-10-18T04:03:20Z

thanks for the help and reviews! I hope that with these changes, we'll be able to get {lightgbm} onto CRAN in November

StrikerRUS · 2020-10-19T13:04:42Z

src/boosting/gbdt_model_text.cpp

@@ -393,7 +393,7 @@ std::string GBDT::SaveModelToString(int start_iteration, int num_iteration, int
    ss << loaded_parameter_ << "\n";
    ss << "end of parameters" << '\n';
  }
-  return ss.str();
+  return std::move(ss.str());


@guolinke @jameslamb I just noticed that std::move here produces the following compilation warnings:

-- The C compiler identification is AppleClang 9.1.0.9020039 -- The CXX compiler identification is AppleClang 9.1.0.9020039 -- Detecting C compiler ABI info -- Detecting C compiler ABI info - done -- Check for working C compiler: /Applications/Xcode_9.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/cc - skipped -- Detecting C compile features -- Detecting C compile features - done -- Detecting CXX compiler ABI info -- Detecting CXX compiler ABI info - done -- Check for working CXX compiler: /Applications/Xcode_9.4.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ - skipped -- Detecting CXX compile features -- Detecting CXX compile features - done -- Found OpenMP_C: -Xclang -fopenmp (found version "3.1") -- Found OpenMP_CXX: -Xclang -fopenmp (found version "3.1") -- Found OpenMP: TRUE (found version "3.1") -- Performing Test MM_PREFETCH -- Performing Test MM_PREFETCH - Success -- Using _mm_prefetch -- Performing Test MM_MALLOC -- Performing Test MM_MALLOC - Success -- Using _mm_malloc -- Configuring done -- Generating done -- Build files have been written to: /Users/runner/work/1/s/build Scanning dependencies of target _lightgbm [ 3%] Building CXX object CMakeFiles/_lightgbm.dir/src/application/application.cpp.o [ 6%] Building CXX object CMakeFiles/_lightgbm.dir/src/boosting/boosting.cpp.o [ 9%] Building CXX object CMakeFiles/_lightgbm.dir/src/boosting/gbdt_model_text.cpp.o [ 12%] Building CXX object CMakeFiles/_lightgbm.dir/src/boosting/gbdt.cpp.o /Users/runner/work/1/s/src/boosting/gbdt_model_text.cpp:396:10: warning: moving a temporary object prevents copy elision [-Wpessimizing-move] return std::move(ss.str()); ^ /Users/runner/work/1/s/src/boosting/gbdt_model_text.cpp:396:10: note: remove std::move call here return std::move(ss.str()); ^~~~~~~~~~ ~ /Users/runner/work/1/s/src/boosting/gbdt_model_text.cpp:621:10: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move] return std::move(feature_importances); ^ /Users/runner/work/1/s/src/boosting/gbdt_model_text.cpp:621:10: note: remove std::move call here return std::move(feature_importances); ^~~~~~~~~~ ~ [ 15%] Building CXX object CMakeFiles/_lightgbm.dir/src/boosting/gbdt_prediction.cpp.o 2 warnings generated. [ 18%] Building CXX object CMakeFiles/_lightgbm.dir/src/boosting/prediction_early_stop.cpp.o [ 21%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/bin.cpp.o [ 24%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/config.cpp.o [ 27%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/config_auto.cpp.o [ 30%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/dataset.cpp.o [ 33%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/dataset_loader.cpp.o [ 36%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/file_io.cpp.o [ 39%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/json11.cpp.o [ 42%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/metadata.cpp.o [ 45%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/parser.cpp.o [ 48%] Building CXX object CMakeFiles/_lightgbm.dir/src/io/tree.cpp.o [ 51%] Building CXX object CMakeFiles/_lightgbm.dir/src/metric/dcg_calculator.cpp.o [ 54%] Building CXX object CMakeFiles/_lightgbm.dir/src/metric/metric.cpp.o [ 57%] Building CXX object CMakeFiles/_lightgbm.dir/src/network/ifaddrs_patch.cpp.o [ 60%] Building CXX object CMakeFiles/_lightgbm.dir/src/network/linker_topo.cpp.o [ 63%] Building CXX object CMakeFiles/_lightgbm.dir/src/network/linkers_mpi.cpp.o [ 66%] Building CXX object CMakeFiles/_lightgbm.dir/src/network/linkers_socket.cpp.o [ 69%] Building CXX object CMakeFiles/_lightgbm.dir/src/network/network.cpp.o [ 72%] Building CXX object CMakeFiles/_lightgbm.dir/src/objective/objective_function.cpp.o [ 75%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/cuda_tree_learner.cpp.o [ 78%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/data_parallel_tree_learner.cpp.o [ 81%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/feature_parallel_tree_learner.cpp.o [ 84%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/gpu_tree_learner.cpp.o [ 87%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/serial_tree_learner.cpp.o [ 90%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/tree_learner.cpp.o [ 93%] Building CXX object CMakeFiles/_lightgbm.dir/src/treelearner/voting_parallel_tree_learner.cpp.o [ 96%] Building CXX object CMakeFiles/_lightgbm.dir/src/c_api.cpp.o [100%] Linking CXX shared library ../lib_lightgbm.so [100%] Built target _lightgbm

I wonder is there any way to avoid both valgrind error and compilation warning?

huh I'm not sure! Think @guolinke will have to answer this one

In c++11, the compliers should be able to auto move the "local" object by return call, so it is safe to remove std::move.
However, it seems apple clang doesn't have this optimization...
To fix the warning, we can remove these std::move

However, it seems apple clang doesn't have this optimization...

I also saw these warnings with ordinary Clang on Ubuntu.

github-actions · 2023-08-24T04:40:15Z

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.

guolinke and others added 4 commits September 29, 2020 18:10

fix int64 write error

52a981e

attempt

d8dff36

Merge branch 'master' into fix-memory-leakage

6af267d

[WIP] [ci] [R-package] Add CI job that runs valgrind tests

7718339

jameslamb added in progress fix labels Oct 8, 2020

jameslamb requested review from Laurae2 and StrikerRUS as code owners October 8, 2020 03:55

jameslamb removed request for Laurae2 and StrikerRUS October 8, 2020 03:55

jameslamb added 3 commits October 7, 2020 22:56

update all-successful

5fcd2fe

install

1d3762e

executable

3f217a3

jameslamb mentioned this pull request Oct 8, 2020

[R-Package] CRAN issues #629

Closed

12 tasks

jameslamb added 2 commits October 10, 2020 18:28

merge master

f5729ed

fix redirect stuff

e80b68d

jameslamb added 2 commits October 10, 2020 19:19

Merge branch 'master' into fix-memory-leakage

d8b9a84

Merge branch 'fix-memory-leakage' into ci/r-valgrind-test

4f20e36

jameslamb requested review from btrotta, chivee and guolinke as code owners October 11, 2020 00:23

jameslamb removed request for btrotta, chivee and guolinke October 11, 2020 00:23

guolinke reviewed Oct 11, 2020

View reviewed changes

jameslamb added 2 commits October 17, 2020 00:08

changes from code review

b03d863

Merge branch 'ci/r-valgrind-test' of github.com:microsoft/LightGBM in…

974ab17

…to ci/r-valgrind-test

jameslamb mentioned this pull request Oct 17, 2020

[R-package] fix unit tests that cause valgrind issues #3462

Closed

StrikerRUS approved these changes Oct 17, 2020

View reviewed changes

.github/workflows/r_valgrind.yml Outdated Show resolved Hide resolved

.github/workflows/r_valgrind.yml Outdated Show resolved Hide resolved

.github/workflows/r_valgrind.yml Outdated Show resolved Hide resolved

R-package/tests/testthat/test_utils.R Outdated Show resolved Hide resolved

jameslamb mentioned this pull request Oct 17, 2020

[ci] ignore R CMD CHECK warnings on new R version #3468

Merged

jameslamb and others added 5 commits October 17, 2020 17:27

Merge branch 'master' into ci/r-valgrind-test

44afb1d

Apply suggestions from code review

cefb3dd

Co-authored-by: Nikita Titov <nekit94-08@mail.ru>

Merge branch 'ci/r-valgrind-test' of github.com:microsoft/LightGBM in…

d8856bd

…to ci/r-valgrind-test

linting

0374e87

issue comments too

e9c46aa

jameslamb commented Oct 17, 2020

View reviewed changes

.github/workflows/r_valgrind.yml Outdated Show resolved Hide resolved

jameslamb added 2 commits October 17, 2020 18:16

remove issue_comment

5d435f6

Merge branch 'master' into ci/r-valgrind-test

265400c

jameslamb mentioned this pull request Oct 18, 2020

[R-package] [ci] mingw gcc often fails on CMake builds #3469

Closed

jameslamb merged commit 81d7611 into master Oct 18, 2020

jameslamb deleted the ci/r-valgrind-test branch October 18, 2020 04:04

StrikerRUS reviewed Oct 19, 2020

View reviewed changes

This was referenced Oct 19, 2020

SEGFAULT on GPU version compiled with Clang #3475

Open

remove std::move #3478

Merged

This was referenced Nov 2, 2020

Fix model locale issue and improve model R/W performance. #3405

Merged

fix invalid read detected by valgrind #3526

Merged

github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ci] [R-package] Fix memory leaks found by valgrind #3443

[ci] [R-package] Fix memory leaks found by valgrind #3443

jameslamb commented Oct 8, 2020

jameslamb commented Oct 8, 2020

guolinke commented Oct 9, 2020

jameslamb commented Oct 10, 2020 •

edited

Loading

jameslamb commented Oct 11, 2020

guolinke commented Oct 11, 2020

guolinke left a comment

jameslamb commented Oct 17, 2020

jameslamb commented Oct 17, 2020

StrikerRUS commented Oct 17, 2020

StrikerRUS commented Oct 17, 2020

StrikerRUS left a comment

jameslamb commented Oct 17, 2020

jameslamb commented Oct 18, 2020

jameslamb commented Oct 18, 2020

StrikerRUS Oct 19, 2020

jameslamb Oct 20, 2020

guolinke Oct 20, 2020 •

edited

Loading

StrikerRUS Oct 20, 2020

github-actions bot commented Aug 24, 2023

[ci] [R-package] Fix memory leaks found by valgrind #3443

[ci] [R-package] Fix memory leaks found by valgrind #3443

Conversation

jameslamb commented Oct 8, 2020

Notes

jameslamb commented Oct 8, 2020

guolinke commented Oct 9, 2020

jameslamb commented Oct 10, 2020 • edited Loading

jameslamb commented Oct 11, 2020

guolinke commented Oct 11, 2020

guolinke left a comment

Choose a reason for hiding this comment

jameslamb commented Oct 17, 2020

jameslamb commented Oct 17, 2020

StrikerRUS commented Oct 17, 2020

StrikerRUS commented Oct 17, 2020

StrikerRUS left a comment

Choose a reason for hiding this comment

jameslamb commented Oct 17, 2020

jameslamb commented Oct 18, 2020

jameslamb commented Oct 18, 2020

StrikerRUS Oct 19, 2020

Choose a reason for hiding this comment

jameslamb Oct 20, 2020

Choose a reason for hiding this comment

guolinke Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

StrikerRUS Oct 20, 2020

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 2023

jameslamb commented Oct 10, 2020 •

edited

Loading

guolinke Oct 20, 2020 •

edited

Loading