[fix] change the destructor of ScoreUpdater to virtual (fixes #5400)

[shiyu1994]

This is to fix #5400.

[jameslamb]

@shiyu1994 please add one of the labels from

LightGBM/.github/release-drafter.yml

Line 3 in c7102e5

categories:

to every pull request you open, so it will be correctly categorized in the release notes

[jameslamb]

/gha run r-valgrind

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

Status: failure ❌.

[jameslamb]

also @shiyu1994 I noticed you created this PR from your fork...can you please just create branches here in microsoft/LightGBM in the future?

That makes it easier for other maintainers to help and collaborate, e.g. avoids issues like #5371.

[shiyu1994]

@jameslamb Thanks. Will always create a branch on microsoft/LightGBM in the future.

[shiyu1994]

/gha run r-valgrind

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

Status: failure ❌.

[jameslamb]

Sorry @shiyu1994 , it looks like valgrind job is timing out. I'm not sure why, but it has been a few weeks since we last ran that job and I know there have been some significant changes on master in that time.

I opened #5414 increasing the timeout, hopefully that will help.

[jameslamb]

/gha run r-valgrind

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

Status: failure ❌.

[jameslamb]

@shiyu1994 the valgrind test is now failing for what looks like a real reason, not a timeout.

==4762== 352 bytes in 1 blocks are possibly lost in loss record 152 of 2,904
==4762==    at 0x484DA83: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==4762==    by 0x40147D9: calloc (rtld-malloc.h:44)
==4762==    by 0x40147D9: allocate_dtv (dl-tls.c:375)
==4762==    by 0x40147D9: _dl_allocate_tls (dl-tls.c:634)
==4762==    by 0x4DF4834: allocate_stack (allocatestack.c:430)
==4762==    by 0x4DF4834: pthread_create@@GLIBC_2.34 (pthread_create.c:647)
==4762==    by 0x575B1EF: ??? (in /usr/lib/x86_64-linux-gnu/libgomp.so.1.0.0)
==4762==    by 0x5751A10: GOMP_parallel (in /usr/lib/x86_64-linux-gnu/libgomp.so.1.0.0)
==4762==    by 0x16C99F47: OMP_NUM_THREADS() (openmp_wrapper.h:22)
==4762==    by 0x16FE6E64: OMP_SET_NUM_THREADS(int) (openmp_wrapper.h:29)
==4762==    by 0x16FDB3AC: LGBM_DatasetCreateFromCSC (c_api.cpp:1462)
==4762==    by 0x1700C83C: LGBM_DatasetCreateFromCSC_R (lightgbm_R.cpp:183)
==4762==    by 0x4953D8A: R_doDotCall (dotcode.c:627)
==4762==    by 0x495ED3E: do_dotcall (dotcode.c:1284)
==4762==    by 0x499E245: Rf_eval (eval.c:851)

(build link)

That might not be from this PR (it has been a while since we have run r-valgrind), but it should be fixed. Given that it's coming from LGBM_DatasetCreateFromCSC, I suspect it might be related to #5344, but not sure.

Can you please look into it?

[shiyu1994]

@jameslamb I did not find any potential memory linkage introduced by the changes of LGBM_DatasetCreateFromCSC from #5344. I noticed that there's one false positive case documented about LGBM_DatasetCreateFromCSC

LightGBM/.ci/test_r_package_valgrind.sh

Lines 48 to 63 in 9489f87

	# one error caused by a false positive between valgrind and openmp is allowed
	# ==2063== 336 bytes in 1 blocks are possibly lost in loss record 153 of 2,709
	# ==2063== at 0x483DD99: calloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
	# ==2063== by 0x40149CA: allocate_dtv (dl-tls.c:286)
	# ==2063== by 0x40149CA: _dl_allocate_tls (dl-tls.c:532)
	# ==2063== by 0x5702322: allocate_stack (allocatestack.c:622)
	# ==2063== by 0x5702322: pthread_create@@GLIBC_2.2.5 (pthread_create.c:660)
	# ==2063== by 0x56D0DDA: ??? (in /usr/lib/x86_64-linux-gnu/libgomp.so.1.0.0)
	# ==2063== by 0x56C88E0: GOMP_parallel (in /usr/lib/x86_64-linux-gnu/libgomp.so.1.0.0)
	# ==2063== by 0x1544D29C: LGBM_DatasetCreateFromCSC (c_api.cpp:1286)
	# ==2063== by 0x1546F980: LGBM_DatasetCreateFromCSC_R (lightgbm_R.cpp:91)
	# ==2063== by 0x4941E2F: R_doDotCall (dotcode.c:634)
	# ==2063== by 0x494CCC6: do_dotcall (dotcode.c:1281)
	# ==2063== by 0x499FB01: bcEval (eval.c:7078)
	# ==2063== by 0x498B67F: Rf_eval (eval.c:727)
	# ==2063== by 0x498E414: R_execClosure (eval.c:1895)

.
Is it related to the new error message? They seem quite similar.

[jameslamb]

oh wow @shiyu1994 you are right! Very nice attention to detail!!

This looks like EXACTLY that same issue, even the same stack trace. I guess the test is failing because the "possibly lost" went from 336 bytes.

LightGBM/.ci/test_r_package_valgrind.sh

Line 49 in 9489f87

# ==2063== 336 bytes in 1 blocks are possibly lost in loss record 153 of 2,709

to 352 (#5403 (comment))

The fact that it's exactly 16 bytes is interesting. I wonder if maybe, for example, some change from int16 to int32 or something somewhere is causing this? That could be coming from a change in libomp version, compiler version, or something else...I think it's ok just to increase the limit in the test. I'll put up a PR to do that.

[shiyu1994]

/gha run r-valgrind

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

Status: success ✔️.

[jameslamb]

🎉 glad valgrind is passing now!

whenever @guolinke is able to review and approve this, let's merge it.

[jameslamb]

I wanted to check that this fixed the warning from #5400, but unfortunately the build logs are already gone from Azure.

So I was going to merge master into this and push a commit, but I can't because this is from your fork.

Closing and re-opening to re-trigger CI.

[jameslamb]

Checked the logs for several different CI jobs and I don't see any warnings after:

[ 11%] Building CXX object CMakeFiles/lightgbm_objs.dir/src/boosting/gbdt_model_text.cpp.o

Thanks for the fix!

[github-actions]

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.