Use contiguous Arrays more by KrisThielemans · Pull Request #1438 · UCL/STIR

KrisThielemans · 2024-05-28T02:29:01Z

add copy_to/fill_from specialisations for Array that check if they are contigous. This enhances the likelihood that the compiler will recognise that it can call memmove as opposed to doing a loop. (related to std::copy of VectorWithOffset<float> does not fall back to memmove #1259 )
use that in ProjDataInMemory

- remove some occurences if #ifdef STIR_TOF - use newer constructors taking a Bin

use them in ProjDataInMemory

KrisThielemans · 2024-05-28T02:29:42Z

@markus-jehl I'm still hunting for an elusive speed-up. Maybe this one does help a bit...

[ci skip]

markus-jehl · 2024-06-02T06:11:40Z

This merge broke something in the reconstruction. Projections still work, but when I do the initial reconstruction it comes out full of NaN‘s in the FoV. Haven‘t had time to look into the changes yet, but maybe this is also what impacts you on the psmr branch (since I also got a test failure on the MacOS build on my fork after pulling this merge).

KrisThielemans · 2024-06-02T13:22:22Z

ah. sorry.

This PR is quite innocent really. Essentially the only change is in ProjDataInMemory::copy_data_from_buffer (and the to version), where it uses a new specialisation of fill_from and copy_to for Array. That checks that if it's a contiguous array, we'll fall back to a std::copy using float * as opposed to full iterators, as the former should be faster (any good compiler will call memmove or similar).

There are some other formatting changes as I got rid of #ifdef STIR_TOF, which gives a lot of "noise" due to reformatting sadly.

I've added tests for all this, but I suppose none of our tests do a recon with ProjDataInMemory though.

KrisThielemans · 2024-06-02T22:48:23Z

@markus-jehl I cannot find a problem, nor reproduce it. Are you sure it is this PR?

Suggested debugging steps:

disable the CopyFill<Array<>> specialisation (just comment out this)
if step 1 works, then re-enable and replace the if (stir_array.is_contiguous()) statements in those lines with if(false).
if step 1 didn't work, revert the ProjDataInMemory::copy_data_from_buffer (and to) to use std::copy directly (really shouldn't do anything as CopyFill should then fall-back to the same std::copy anyway)
if step 3 didn't work, then revert ProjDataInMemory completely (it'd have to be something in the STIR_TOF edits I guess)

thanks for your help!

markus-jehl · 2024-06-03T06:58:45Z

I'm fairly certain it is this one: I went back to each PR in my fork and this is the one that first displayed the issue.

Will go through your suggested debug steps today!

markus-jehl · 2024-06-03T11:37:20Z

The problem is the first if(stir_array.is_contiguous()). I noticed that we're not actually checking if some other process has already obtained full access:

STIR/src/include/stir/Array.inl

Line 283 in 31e730e

this->_full_pointer_access = true;

What is the purpose of _full_pointer_access, actually? It seems to be never checked and since it's only used to read data, not modify them it shouldn't be required. Still, I don't understand why std::copy(beg, beg + stir_array.size_all(), iter); gives issues while std::copy(stir_array.begin_all(), stir_array.end_all(), iter); is fine...

KrisThielemans · 2024-06-03T13:46:53Z

The problem is the first if(stir_array.is_contiguous()).

which one is the first? L60?

I noticed that we're not actually checking if some other process has already obtained full access

that's true.

What is the purpose of _full_pointer_access, actually? It seems to be never checked and since it's only used to read data, not modify them it shouldn't be required.

For get_data_ptr, we do check if access is on/off

STIR/src/include/stir/VectorWithOffset.inl

Lines 611 to 613 in 31e730e

    
           VectorWithOffset<T>::get_data_ptr() 
        
           { 
        
             assert(!pointer_access);

I put those in as a safety check really. std::vector::data() doesn't do that. It just says that the "behaviour is undefined" if you modify the underlying vector (e.g. by a resize(), which would affect read access as well). I felt it was safer to check, at the expense of more elaborate code (so maybe it wasn't a great decision).

Still, I don't understand why std::copy(beg, beg + stir_array.size_all(), iter); gives issues while std::copy(stir_array.begin_all(), stir_array.end_all(), iter); is fine...

I have no clue.

Note that this whole stuff is not thread-safe: if multiple processes modify an Array at the same time, stuff is going to happen. That' s not documented, but should be rather obvious. So, I don't think we do that.

I'm not so sure how to proceed. Can you find out where the NaNs are created? Is the sensitivity image ok for instance?

Can you please create an issue, just pointing to this discussion here? (I suggest we continue here for clarity)

markus-jehl · 2024-06-03T14:17:21Z

Yes, L60 is the problematic one. I'm currently running with as much debug output as possible to narrow down where it happens.

Sure, will create an issue.

markus-jehl · 2024-06-03T14:56:33Z

This is the subsensitivity image, so the NaN's come from there:

KrisThielemans · 2024-06-03T15:27:54Z

ok. Narrowing it down. Relevant lines are

STIR/src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx

Lines 830 to 832 in 31e730e

    
           auto sens_proj_data_sptr 
        
               = std::make_shared<ProjDataInMemory>(this->proj_data_sptr->get_exam_info_sptr(), this->sens_proj_data_info_sptr); 
        
           sens_proj_data_sptr->fill(1.0F);

STIR/src/recon_buildblock/PoissonLogLikelihoodWithLinearModelForMeanAndProjData.cxx

Line 871 in 31e730e

distributable_sensitivity_computation(this->sens_backprojector_sptr,

By the way, do your ctest and recon_test_pack work?

markus-jehl · 2024-06-03T15:51:29Z

All tests with ctest pass, but recon_test_pack I first need to look up how to run. Will also look into the distributable sensitivity computation, since that already sounds like it might be a good candidate :-)

KrisThielemans · 2024-06-03T16:24:22Z

All tests with ctest pass

interesting, as that is using OSMAPOSL etc as well

recon_test_pack, see e.g.: https://github.com/conda-forge/stir-feedstock/blob/6d4fd1aaf32bfe034bbe7440988cedac3f2d12da/recipe/meta.yaml#L66-L79

the distributable_computation is the hardest part in STIR (sits in distributable.cxx). Essentially, it is a multi-threaded loop where it's going to call a callback. That makes me think you could try and set threads=1, or compile without openmp.

thanks for debugging!

markus-jehl · 2024-06-04T11:18:22Z

Interestingly it looks exactly the same in repeat runs and also suing just 1 thread. So my suspicion is that the is_contiguous function sometimes gives false positives. But I'm still looking for the flaw in the logic...

KrisThielemans · 2024-06-04T11:55:54Z

it certainly is easier if it's an ordinary bug as opposed to a race-condition! I guess I'd recommend compiling without OpenMP then and running valgrind.

Other (more desperate) thing to try: use a more conservative approach for is_contiguous:

add a bool Array::_is_contiguous member
set it to true in Array::init()
set it to false in the 2 constructors that don't call init, i.e. here and here, and in resize()
let is_contiguous return _is_contiguous.

Vital to run ctest after that!

If that works, we could try to check consistency between the old and new is_contiguous (although the new one is overly pessimistic at the moment).

markus-jehl · 2024-06-04T12:07:29Z

I was actually thinking about this pessimistic approach! Because is_contiguous is called many times, I was worried if it might actually be computationally heavy in its own right. The member variable would solve this problem. Will give it a try.

I agree, running valgrind or a sanitiser should highlight where the issue is. For this I first need to create a minimal working example of the issue (currently I just test it in python with an iterative reconstruction that would take forever in serial with a sanitiser attached to it...). The recon_test_pack passes as well, btw.

KrisThielemans · 2024-06-04T12:57:38Z

ok. From Python, things are always trickier, as there's more variations possible. That'll be why I didn't see it yet.

As it's the sensitivity image, you can stop after the recon.set_up() call of course.

markus-jehl · 2024-06-05T07:57:35Z

Just an update: I've now ported the test across to C++ and I get this weird looking sensitivity map even when disabling the new copy. So the more I dig the less I understand... Will update you when I have more clarity.

KrisThielemans · 2024-06-05T10:20:43Z

Ok. Any chance you can do this on vanilla master as opposed to your branch?

markus-jehl · 2024-06-05T11:01:59Z

Yes, I'm currently doing it on the official master. It looks like in C++ some additional problems are caused by reading ProjDataInMemory from file and using them - as if some of that memory is either not persistent for long enough or gives issues with copying.

KrisThielemans · 2024-06-05T16:52:59Z

By the way, if you have a small test example, potentially we could add it as a test, as it's escaping all other tests somehow. Can also do a git bisect I guess. thanks, and sorry for the trouble.

markus-jehl · 2024-06-06T06:49:00Z

The address sanitiser actually already throws an error in the first line of my test, where I'm reading in ProjDataInMemory as such:
auto inputProjData = std::make_shared<stir::ProjDataInMemory>(*(stir::ProjDataInMemory::read_from_file("filename.hs")));

This is the ASAN output:

=================================================================
==1174870==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6170000301d0 at pc 0x55eb98f589ad bp 0x7fff7c0a8730 sp 0x7fff7c0a7ef8
READ of size 92160 at 0x6170000301d0 thread T0
    #0 0x55eb98f589ac in __interceptor_memmove (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x1839ac) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #1 0x55eb9951fff8 in float* stir::CopyFill<stir::Array<3, float> >::copy_to<float*>(stir::Array<3, float> const&, float*) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x74aff8) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #2 0x55eb9951b93d in stir::ProjDataInMemory::set_segment(stir::SegmentBySinogram<float> const&) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x74693d) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #3 0x55eb9951b9d1 in stir::ProjDataInMemory::set_segment(stir::SegmentByView<float> const&) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x7469d1) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #4 0x55eb994e2093 in stir::ProjData::fill(stir::ProjData const&) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x70d093) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #5 0x55eb9951dfa3 in stir::ProjDataInMemory::ProjDataInMemory(stir::ProjData const&) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x748fa3) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #6 0x55eb991c7e94 in decltype(new ((void*)(0))stir::ProjDataInMemory(std::declval<stir::ProjData&>())) std::construct_at<stir::ProjDataInMemory, stir::ProjData&>(stir::ProjDataInMemory*, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_construct.h:97:39
    #7 0x55eb991c7e94 in void std::allocator_traits<std::allocator<stir::ProjDataInMemory> >::construct<stir::ProjDataInMemory, stir::ProjData&>(std::allocator<stir::ProjDataInMemory>&, stir::ProjDataInMemory*, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/alloc_traits.h:518:4
    #8 0x55eb991c7e94 in std::_Sp_counted_ptr_inplace<stir::ProjDataInMemory, std::allocator<stir::ProjDataInMemory>, (__gnu_cxx::_Lock_policy)2>::_Sp_counted_ptr_inplace<stir::ProjData&>(std::allocator<stir::ProjDataInMemory>, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:519:4
    #9 0x55eb991c7e94 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count<stir::ProjDataInMemory, std::allocator<stir::ProjDataInMemory>, stir::ProjData&>(stir::ProjDataInMemory*&, std::_Sp_alloc_shared_tag<std::allocator<stir::ProjDataInMemory> >, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:651:6
    #10 0x55eb991c7e94 in std::__shared_ptr<stir::ProjDataInMemory, (__gnu_cxx::_Lock_policy)2>::__shared_ptr<std::allocator<stir::ProjDataInMemory>, stir::ProjData&>(std::_Sp_alloc_shared_tag<std::allocator<stir::ProjDataInMemory> >, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr_base.h:1342:14
    #11 0x55eb991c7e94 in std::shared_ptr<stir::ProjDataInMemory>::shared_ptr<std::allocator<stir::ProjDataInMemory>, stir::ProjData&>(std::_Sp_alloc_shared_tag<std::allocator<stir::ProjDataInMemory> >, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:409:4
    #12 0x55eb991c7e94 in std::shared_ptr<stir::ProjDataInMemory> std::allocate_shared<stir::ProjDataInMemory, std::allocator<stir::ProjDataInMemory>, stir::ProjData&>(std::allocator<stir::ProjDataInMemory> const&, stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:862:14
    #13 0x55eb991c7e94 in std::shared_ptr<stir::ProjDataInMemory> std::make_shared<stir::ProjDataInMemory, stir::ProjData&>(stir::ProjData&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:878:14
    #14 0x55eb991c7e94 in TestClientReconComponent::TestArrayBug() /workspace/neurolf/reconstruction/test/unit/ReconstructionComponentTests.cpp:492:24
    #15 0x55eb9920907e in ____C_A_T_C_H____T_E_S_T____18() /workspace/neurolf/reconstruction/test/unit/ReconstructionComponentTests.cpp:987:24
    #16 0x55eb9902b759 in Catch::TestInvokerAsFunction::invoke() const /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:14317:9
    #17 0x55eb99096d36 in Catch::TestCase::invoke() const /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:14156:15
    #18 0x55eb99096d36 in Catch::RunContext::invokeActiveTestCase() /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:13016:27
    #19 0x55eb990943a9 in Catch::RunContext::runCurrentTest(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >&) /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:12989:17
    #20 0x55eb9909129f in Catch::RunContext::runTest(Catch::TestCase const&) /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:12750:13
    #21 0x55eb9909f32f in Catch::(anonymous namespace)::TestGroup::execute() /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:13343:45
    #22 0x55eb9909f32f in Catch::Session::runInternal() /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:13549:39
    #23 0x55eb9909c4eb in Catch::Session::run() /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:13505:24
    #24 0x55eb990d56d2 in int Catch::Session::run<char>(int, char const* const*) /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:13227:30
    #25 0x55eb990d56d2 in main /workspace/neurolf/reconstruction/libs/install/include/catch2/catch.hpp:17504:29
    #26 0x7f7c09d04d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
    #27 0x7f7c09d04e3f in __libc_start_main csu/../csu/libc-start.c:392:3
    #28 0x55eb98f3eb64 in _start (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x169b64) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)

0x6170000301d0 is located 0 bytes to the right of 720-byte region [0x61700002ff00,0x6170000301d0)
allocated by thread T0 here:
    #0 0x55eb98ffc88d in operator new[](unsigned long) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x22788d) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)
    #1 0x55eb99212e3a in stir::VectorWithOffset<float>::reserve(int, int) /workspace/neurolf/reconstruction/libs/install/include/STIR-6.2/stir/VectorWithOffset.inl:395:45
    #2 0x55eb99337963 in stir::detail::VectorWithOffset_iter<stir::Array<1, float> > std::__copy_move<false, false, std::random_access_iterator_tag>::__copy_m<stir::detail::VectorWithOffset_iter<stir::Array<1, float> const>, stir::detail::VectorWithOffset_iter<stir::Array<1, float> > >(stir::detail::VectorWithOffset_iter<stir::Array<1, float> const>, stir::detail::VectorWithOffset_iter<stir::Array<1, float> const>, stir::detail::VectorWithOffset_iter<stir::Array<1, float> >) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x562963) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x1839ac) (BuildId: 8775ca2bc7777fd8de9cf6411d87eded039071d8) in __interceptor_memmove
Shadow bytes around the buggy address:
  0x0c2e7fffdfe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffdff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffe000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffe010: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffe020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c2e7fffe030: 00 00 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa
  0x0c2e7fffe040: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2e7fffe050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffe060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffe070: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2e7fffe080: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1174870==ABORTING

When I replace the first if statement with if (false), then it runs through the whole test and gives a good sensitivity map, but in the very end also reports a small memory leak:

=================================================================
==1176468==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1584 byte(s) in 3 object(s) allocated from:
    #0 0x562c30a4e77d in operator new(unsigned long) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x22777d) (BuildId: 5778f5e9d8b110bd49f3ae4e3fea7131c16efff7)
    #1 0x562c30f364c9 in stir::ProjData::read_from_file(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::_Ios_Openmode) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x70f4c9) (BuildId: 5778f5e9d8b110bd49f3ae4e3fea7131c16efff7)

Indirect leak of 24576 byte(s) in 3 object(s) allocated from:
    #0 0x562c30a4e88d in operator new[](unsigned long) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x22788d) (BuildId: 5778f5e9d8b110bd49f3ae4e3fea7131c16efff7)
    #1 0x7f91d9670023 in std::basic_filebuf<char, std::char_traits<char> >::_M_allocate_internal_buffer() (/lib/x86_64-linux-gnu/libstdc++.so.6+0x115023) (BuildId: e37fe1a879783838de78cbc8c80621fa685d58a2)

SUMMARY: AddressSanitizer: 26160 byte(s) leaked in 6 allocation(s).

markus-jehl · 2024-06-06T09:50:27Z

Ok, progress! Managed to debug into STIR to see where it's failing. All that is needed is to handle the case where an array has only one index. Currently it is not entering the for loop and therefore not checking whether the sub-array is contiguous (plus the last sub-array was never checked). Changing the for loop from i < this->get_max_index() to i <= this->get_max_index() and handling the one case where we index i+1 fixes this:

template <int num_dimensions, typename elemT>
bool
Array<num_dimensions, elemT>::is_contiguous() const
{
  auto mem = &(*this->begin_all());
  for (auto i = this->get_min_index(); i <= this->get_max_index(); ++i)
    {
      if (!(*this)[i].is_contiguous())
        return false;
      mem += (*this)[i].size_all();
      if (i < this->get_max_index() && mem != &(*(*this)[i + 1].begin_all()))
        return false;
    }
  return true;
}

KrisThielemans · 2024-06-06T11:08:35Z

great! That was a bug. Can you submit a PR?

KrisThielemans · 2024-06-06T11:11:13Z

I suggest

     if (i == this->get_max_index() 
       return true;
     mem += (*this)[i].size_all();
     if (mem != &(*(*this)[i + 1].begin_all()))
        return false;

and remove the final return false. Might be slightly easier to understand the logic.

KrisThielemans · 2024-06-06T11:14:28Z

By the way, while

auto inputProjData = std::make_shared<stir::ProjDataInMemory>(*(stir::ProjDataInMemory::read_from_file("filename.hs")));

is fine, it might be clearer to use

std::shared<stir::ProjDataInMemory> inputProjData(stir::ProjDataInMemory::read_from_file("filename.hs"));

markus-jehl · 2024-06-06T11:32:25Z

Yes, I'll issue a PR. But first I also want to understand the memory leak that still exists.

KrisThielemans · 2024-06-06T11:37:47Z

Ok. Doesn't TSAN tell you where it came from? Valgrind could.

markus-jehl · 2024-06-06T11:57:18Z

This is all it says:

==1342194==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 1584 byte(s) in 3 object(s) allocated from:
    #0 0x55da4942f77d in operator new(unsigned long) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x22777d) (BuildId: ee84e9178c2086875508efe40ec877644497b58a)
    #1 0x55da49917699 in stir::ProjData::read_from_file(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::_Ios_Openmode) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x70f699) (BuildId: ee84e9178c2086875508efe40ec877644497b58a)

Indirect leak of 24576 byte(s) in 3 object(s) allocated from:
    #0 0x55da4942f88d in operator new[](unsigned long) (/workspace/neurolf/reconstruction/build/test/nlf.reconstruction.tests.asan+0x22788d) (BuildId: ee84e9178c2086875508efe40ec877644497b58a)
    #1 0x7f8958273023 in std::basic_filebuf<char, std::char_traits<char> >::_M_allocate_internal_buffer() (/lib/x86_64-linux-gnu/libstdc++.so.6+0x115023) (BuildId: e37fe1a879783838de78cbc8c80621fa685d58a2)

SUMMARY: AddressSanitizer: 26160 byte(s) leaked in 6 allocation(s).

I suspect it is when the object goes out of scope at the end of the test that the memory is not correctly freed. Has something changed there recently?

KrisThielemans · 2024-06-06T12:50:02Z

Seems unrelated to the rest. Nothing changed there.

It's not so useful that it doesn't say what object. Clearly, it cannot be the ProjDataFromMemory data itself, as the leak would then be a lot larger.

Just for sanity, please try the above constructor instead.

markus-jehl · 2024-06-06T13:19:27Z

I actually did (must have also tried before and then given up), but it gives me the following error:

/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:150:7: note: candidate constructor not viable: no known conversion from 'shared_ptr<stir::ProjData>' to 'const shared_ptr<stir::ProjDataInMemory>' for 1st argument
      shared_ptr(const shared_ptr&) noexcept = default; ///< Copy constructor
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:304:7: note: candidate constructor not viable: no known conversion from 'shared_ptr<stir::ProjData>' to 'shared_ptr<stir::ProjDataInMemory>' for 1st argument
      shared_ptr(shared_ptr&& __r) noexcept
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:357:17: note: candidate constructor not viable: no known conversion from 'shared_ptr<stir::ProjData>' to 'std::nullptr_t' for 1st argument
      constexpr shared_ptr(nullptr_t) noexcept : shared_ptr() { }
                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:160:2: note: candidate template ignored: could not match '_Yp *' against 'shared_ptr<stir::ProjData>'
        shared_ptr(_Yp* __p) : __shared_ptr<_Tp>(__p) { }
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:296:2: note: candidate template ignored: requirement 'is_constructible<std::__shared_ptr<stir::ProjDataInMemory, __gnu_cxx::_S_atomic>, const std::shared_ptr<stir::ProjData> &>::value' was not satisfied [with _Yp = stir::ProjData]
        shared_ptr(const shared_ptr<_Yp>& __r) noexcept
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:313:2: note: candidate template ignored: requirement 'is_constructible<std::__shared_ptr<stir::ProjDataInMemory, __gnu_cxx::_S_atomic>, std::shared_ptr<stir::ProjData>>::value' was not satisfied [with _Yp = stir::ProjData]
        shared_ptr(shared_ptr<_Yp>&& __r) noexcept
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:325:11: note: candidate template ignored: could not match 'weak_ptr' against 'shared_ptr'
        explicit shared_ptr(const weak_ptr<_Yp>& __r)
                 ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:332:2: note: candidate template ignored: could not match 'auto_ptr' against 'shared_ptr'
        shared_ptr(auto_ptr<_Yp>&& __r);
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:340:2: note: candidate template ignored: could not match 'unique_ptr' against 'shared_ptr'
        shared_ptr(unique_ptr<_Yp, _Del>&& __r)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:408:2: note: candidate template ignored: could not match '_Sp_alloc_shared_tag' against 'shared_ptr'
        shared_ptr(_Sp_alloc_shared_tag<_Alloc> __tag, _Args&&... __args)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:148:17: note: candidate constructor not viable: requires 0 arguments, but 1 was provided
      constexpr shared_ptr() noexcept : __shared_ptr<_Tp>() { }
                ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:177:2: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided
        shared_ptr(_Yp* __p, _Deleter __d)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:194:2: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided
        shared_ptr(nullptr_t __p, _Deleter __d)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:257:2: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided
        shared_ptr(const shared_ptr<_Yp>& __r, element_type* __p) noexcept
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:284:2: note: candidate constructor template not viable: requires 2 arguments, but 1 was provided
        shared_ptr(shared_ptr<_Yp>&& __r, element_type* __p) noexcept
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:417:7: note: candidate constructor not viable: requires 2 arguments, but 1 was provided
      shared_ptr(const weak_ptr<_Tp>& __r, std::nothrow_t) noexcept
      ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:214:2: note: candidate constructor template not viable: requires 3 arguments, but 1 was provided
        shared_ptr(_Yp* __p, _Deleter __d, _Alloc __a)
        ^
/usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/shared_ptr.h:233:2: note: candidate constructor template not viable: requires 3 arguments, but 1 was provided
        shared_ptr(nullptr_t __p, _Deleter __d, _Alloc __a)

It's as if it can't convert pointers to convertable objects, only the objects themselves. With my old approach I initialise a new pointer with the converted object, so it worked.

My suspicion is that the leak might be in the interfile parsing somewhere, but I haven't yet found an obvious allocation somewhere that might cause this.

markus-jehl · 2024-06-06T13:35:38Z

Update:
When I use std::shared_ptr<stir::ProjDataInMemory> inputProjData(std::dynamic_pointer_cast<stir::ProjDataInMemory>(stir::ProjDataInMemory::read_from_file("filename.hs"))); then it compiles, but gives a segmentatoin fault later when I call sapyb on inputProjData to subtract randoms from it. Is this because the underlying object is still some file stream that is closed prematurely?

KrisThielemans · 2024-06-06T14:33:37Z

It's as if it can't convert pointers to convertable objects, only the objects themselves. With my old approach I initialise a new pointer with the converted object, so it worked.

oops, sorry. it needs a std::move

std::shared_ptr<stir::ProjDataInMemory> inputProjData(std::move(stir::ProjDataInMemory::read_from_file("filename.hs"));

The cast will fail, as the object disappears. Not "prematurely" in a C++ sense, as the unique_ptr gets destructed immediately, so the object is gone.

KrisThielemans · 2024-06-06T14:34:16Z

My suspicion is that the leak might be in the interfile parsing somewhere, but I haven't yet found an obvious allocation somewhere that might cause this.

oh right. There might even be an issue on that. I'll keep ignoring that for a bit longer. sorry...

markus-jehl · 2024-06-06T14:48:33Z

It's as if it can't convert pointers to convertable objects, only the objects themselves. With my old approach I initialise a new pointer with the converted object, so it worked.

oops, sorry. it needs a std::move
std::shared_ptr<stir::ProjDataInMemory> inputProjData(std::move(stir::ProjDataInMemory::read_from_file("filename.hs"));
The cast will fail, as the object disappears. Not "prematurely" in a C++ sense, as the unique_ptr gets destructed immediately, so the object is gone.

Hmm... The move still gives me the same error: note: candidate constructor not viable: no known conversion from 'shared_ptr<stir::ProjData>' to 'const shared_ptr<stir::ProjDataInMemory>' for 1st argument

KrisThielemans · 2024-06-06T15:29:28Z

ok. Of course, read_from_file doesn't return a ProjDataInMemory object. ignore me!

markus-jehl · 2024-06-06T15:34:09Z

My suspicion is that the leak might be in the interfile parsing somewhere, but I haven't yet found an obvious allocation somewhere that might cause this.

oh right. There might even be an issue on that. I'll keep ignoring that for a bit longer. sorry...

I couldn't find an issue related to this. Should I create one?

KrisThielemans · 2024-06-06T15:52:09Z

Neither can I. Please do so. Thanks a lot!

KrisThielemans added 3 commits May 28, 2024 03:25

minor clean-up of ProjDataInMemory

a9099c2

- remove some occurences if #ifdef STIR_TOF - use newer constructors taking a Bin

small fixes in doxygen for Array

2cd8a95

copy_to/fill_from specialisations for contiguous Arrays

ce41282

use them in ProjDataInMemory

KrisThielemans added the enhancement label May 28, 2024

KrisThielemans added this to the v6.2 milestone May 28, 2024

update release notes

91d718c

[ci skip]

KrisThielemans merged commit 631da0f into UCL:master May 31, 2024

KrisThielemans deleted the useContiguousArraysMore branch May 31, 2024 00:11

KrisThielemans mentioned this pull request Jun 3, 2024

Remove boost::iterator_adaptor from VectorWithOffset #1442

Merged

markus-jehl mentioned this pull request Jun 3, 2024

Array PR introduced issue in osmaposl reconstruction using ProjDataInMemory #1443

Closed

Conversation

KrisThielemans commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented May 28, 2024

Uh oh!

markus-jehl commented Jun 2, 2024

Uh oh!

KrisThielemans commented Jun 2, 2024

Uh oh!

KrisThielemans commented Jun 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markus-jehl commented Jun 3, 2024

Uh oh!

markus-jehl commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markus-jehl commented Jun 3, 2024

Uh oh!

markus-jehl commented Jun 3, 2024

Uh oh!

KrisThielemans commented Jun 3, 2024

Uh oh!

markus-jehl commented Jun 3, 2024

Uh oh!

KrisThielemans commented Jun 3, 2024

Uh oh!

markus-jehl commented Jun 4, 2024

Uh oh!

KrisThielemans commented Jun 4, 2024

Uh oh!

markus-jehl commented Jun 4, 2024

Uh oh!

KrisThielemans commented Jun 4, 2024

Uh oh!

markus-jehl commented Jun 5, 2024

Uh oh!

KrisThielemans commented Jun 5, 2024

Uh oh!

markus-jehl commented Jun 5, 2024

Uh oh!

KrisThielemans commented Jun 5, 2024

Uh oh!

markus-jehl commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markus-jehl commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

markus-jehl commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

markus-jehl commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

markus-jehl commented Jun 6, 2024

Uh oh!

markus-jehl commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

markus-jehl commented Jun 6, 2024

Uh oh!

KrisThielemans commented Jun 6, 2024

Uh oh!

markus-jehl commented Jun 6, 2024

KrisThielemans commented May 28, 2024 •

edited

Loading

KrisThielemans commented Jun 2, 2024 •

edited

Loading

markus-jehl commented Jun 3, 2024 •

edited

Loading

KrisThielemans commented Jun 3, 2024 •

edited

Loading

markus-jehl commented Jun 6, 2024 •

edited

Loading

KrisThielemans commented Jun 6, 2024 •

edited

Loading