lephare-photoz / lephare

LePHARE is a code for calculating photometric redshifts.
MIT License
5 stars 1 forks source link

Fix memory leak #193

Closed hdante closed 2 months ago

hdante commented 2 months ago

Change Description

Hello, this fixes leaking SED instances if they still exist when calling the PhotoZ class destructor (this is the case when running lephare with pz-rail-lephare). The relevant leaks, shown by the address sanitizer:

Direct leak of 2358756400 byte(s) in 5360810 object(s) allocated from:
    #0 0x2b20a2157d77 in operator new(unsigned long) /opt/conda/conda-bld/gcc-compiler_1654084175708/work/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x2b20d6e0a224 in PhotoZ::read_lib(std::vector<SED*, std::allocator<SED*> >&, int&, int*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::vector<int, std::allocator<int> >, int&) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/src/lib/photoz_lib.cpp:561

Direct leak of 77133056 byte(s) in 219128 object(s) allocated from:
    #0 0x2b20a2157d77 in operator new(unsigned long) /opt/conda/conda-bld/gcc-compiler_1654084175708/work/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x2b20d6e09dd1 in PhotoZ::read_lib(std::vector<SED*, std::allocator<SED*> >&, int&, int*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::vector<int, std::allocator<int> >, int&) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/src/lib/photoz_lib.cpp:565

(...)
Direct leak of 1162304 byte(s) in 3302 object(s) allocated from:
    #0 0x2b20a2157d77 in operator new(unsigned long) /opt/conda/conda-bld/gcc-compiler_1654084175708/work/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x2b20d6e09472 in PhotoZ::read_lib(std::vector<SED*, std::allocator<SED*> >&, int&, int*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::vector<int, std::allocator<int> >, int&) /lustre/t1/cl/lsst/tmp/henrique.almeida/lephare/src/lib/photoz_lib.cpp:567

Solution Description

Completed the destructor with the remaining de-allocations.

Code Quality

Project-Specific Pull Request Checklists

Bug Fix Checklist

New Feature Checklist

Documentation Change Checklist

Build/CI Change Checklist

Other Change Checklist

johannct commented 2 months ago

thanks @hdante it seems very reasonable.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 66.28%. Comparing base (23bea25) to head (7f420cb). Report is 6 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #193 +/- ## ========================================== + Coverage 66.26% 66.28% +0.02% ========================================== Files 50 50 Lines 6162 6166 +4 Branches 935 938 +3 ========================================== + Hits 4083 4087 +4 Misses 2079 2079 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

johannct commented 2 months ago

You just need to avoid going to next line for pre-commit to be happy (strange that it chokes on that)

johannct commented 2 months ago

@hdante can you clear this, and perhaps add a test case that clear the 3 vector on destruction?

raphaelshirley commented 2 months ago

I made the changes to get precommit hooks passing. I was partly curious to see how that would work with a forked repository.

hdante commented 2 months ago

Thank you everyone,