update Array hierarchy and allocate nD arrays in a contiguous block by default #1236

KrisThielemans · 2023-08-24T11:26:45Z

addresses the following

Array and VectorWithOffset did not have move constructors, so we were using the default generated ones which might not be optimal
some copy constructors were the default generates ones, but they now needed to be added explicitly
some assignment operators were the default generates ones, which could imply unnecessary reallocation (in 1D)
nD Arrays used many different separately allocated 1D Arrays. This had the consequence that memory is not contiguous (which prevents some optimisations) as well as many calls to new[] and delete[], which turns out to be slow. The default is not to allocate one single block, and let the nD Array point into that. This happens transparent to the rest of the code. CAVEAT: it does mean that growing an nD array in the "first" dimensions could now be less efficient (as it will reallocate everything).

Currently ctest is happy on my VM and test_Array and test_VectorWithOffset work fine via valgrind (Ubuntu, gcc8). We'll see what GHA and AppVeyor say...

@markus-jehl this should be fine for you to test now.

Things still to do:

check if growing an nD contiguous array deallocates original block if it can
adapt other code to take advantage of this, e.g. reading/writing nD Arrays could now read/write in 1 chunk, which would be faster, e.g. read_data, convert_array
make nD Array assignment such that it doesn't create a copy first if reallocation can be avoided.
check if we need move constructors/assignments in derived classes such as Sinogram etc, or are the default ones ok. (operator= might need implemented to benefit from previous bullet)
check if it actually works for everything and improves practical run-times

markus-jehl · 2023-08-24T13:06:17Z

Thanks! I will have a look

markus-jehl · 2023-08-24T13:11:29Z

Does building SWIG work for you? I get some errors, e.g.:

In file included from /workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir-build/src/swig/CMakeFiles/_stir.dir/stirPYTHON_wrap.cxx:3964:
In file included from /workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/ProjDataInfoBlocksOnCylindricalNoArcCorr.h:27:
In file included from /workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/ProjDataInfoGenericNoArcCorr.h:27:
In file included from /workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/GeometryBlocksOnCylindrical.h:31:
In file included from /workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/Array.h:34:
In file included from /workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/NumericVectorWithOffset.h:166:
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/NumericVectorWithOffset.inl:49:5: error: no matching constructor for initialization of 'VectorWithOffset<stir::Array<1, float>>'
  : base_type(min_index, max_index, data_ptr)
    ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir-build/src/swig/CMakeFiles/_stir.dir/stirPYTHON_wrap.cxx:51302:85: note: in instantiation of member function 'stir::NumericVectorWithOffset<stir::Array<1, float>, float>::NumericVectorWithOffset' requested here
      result = (stir::NumericVectorWithOffset< stir::Array< 1,float >,float > *)new stir::NumericVectorWithOffset< stir::Array< 1,float >,float >(arg1,arg2,arg3);
                                                                                    ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:159:5: note: candidate constructor not viable: no known conversion from 'float *const' to 'stir::Array<1, float> *const' for 3rd argument
    VectorWithOffset(const int min_index, const int max_index, 
    ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:140:5: note: candidate constructor not viable: no known conversion from 'const int' to 'stir::Array<1, float> *const' for 2nd argument
    VectorWithOffset(const int hsz, 
    ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:136:10: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
  inline VectorWithOffset(const int min_index, const int max_index);
         ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:146:5: note: candidate constructor not viable: requires 2 arguments, but 3 were provided
    VectorWithOffset(const int hsz, 
    ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:153:5: note: candidate constructor not viable: requires 4 arguments, but 3 were provided
    VectorWithOffset(const int min_index, const int max_index, 
    ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:133:19: note: candidate constructor not viable: requires single argument 'hsz', but 3 arguments were provided
  inline explicit VectorWithOffset(const int hsz);
                  ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:165:10: note: candidate constructor not viable: requires single argument 'il', but 3 arguments were provided
  inline VectorWithOffset(const VectorWithOffset &il) ;
         ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:187:3: note: candidate constructor not viable: requires single argument 'other', but 3 arguments were provided
  VectorWithOffset(VectorWithOffset&& other) noexcept;
  ^
/workspace/python-reconstruction-pipeline/libs/build/stir/Stir-prefix/src/Stir/src/include/stir/VectorWithOffset.h:130:10: note: candidate constructor not viable: requires 0 arguments, but 3 were provided
  inline VectorWithOffset();
         ^

KrisThielemans · 2023-08-24T13:26:05Z

forgot to push...

markus-jehl · 2023-08-24T13:44:44Z

Perfect, thanks! It builds now, but importing stir in python leads to an error still:
/workspace/python-reconstruction-pipeline/libs/install/stir/python/_stir.so: undefined symbol: _ZN4stir5ArrayILi1EfEC1ERKNS_10IndexRangeILi1EEEPf

I'll go through the code changes in patience to understand all the things that have changed.

KrisThielemans · 2023-08-24T14:21:50Z

ok. I didn't try that yet. This appears to be stir::Array<1, float>::Array(stir::IndexRange<1> const&, float*). This isn't implemented yet, but also not used. I'm trying to comment it out and see what happens. Ultimately, the init function should be protected, and exposed via relevant constructors.

It does point to the fact that we're SWIGging too much. (anything with elemT* makes no sense in Python). We could either %ignore them, but might be easiest to put them between #ifdef SWIG in the .h/.inl file.

KrisThielemans · 2023-08-24T14:29:34Z

I'll rebase this on current master as well. I'm assuming that's ok. Should have done that before creating the PR. sorry

KrisThielemans · 2023-08-24T14:59:03Z

done. this imports now.

KrisThielemans · 2023-08-24T16:16:27Z

ah well, committed a temp stir.i file. Restored now (again with force push)

markus-jehl · 2023-08-25T09:33:10Z

Ran the latest version this morning through the python reconstruction steps and everything worked fine! Will later also test it in C++, as well as with sanitisers

KrisThielemans · 2023-08-25T09:45:58Z

that's great. but is it any faster? (or does it use more memory?)

markus-jehl · 2023-08-25T09:56:12Z

Not noticeably in the setup I used, but I want to test this with C++ where everything is a bit more controllable.

KrisThielemans · 2023-08-30T07:19:03Z

Oops, I accidentally pushed a merge with #1237. That wasn't my intention. Depending on how I feel, I might still revert that...

markus-jehl · 2023-08-30T12:28:06Z

Tested it now in C++ and both speed and memory consumption look identical.

KrisThielemans · 2023-08-30T17:03:54Z

ah well. maybe if we exploit it a bit more. But profiling often throws "conventional wisdom" in the bin...

KrisThielemans · 2023-08-31T11:28:19Z

Tested it now in C++ and both speed and memory consumption look identical.

the current test_Array contains some (de)allocation timings for a 4D array of size 20x100x400x600. On my desktop I get

creation of non-contiguous 4D Array 654.412ms
deletion 7.956 ms
contiguous array creation (total) 224.647ms
deletion 7.317 ms

That might be non-negligible for GPU applications, but obviously it is essentially 20 3D images, so in practice it might just not matter.

I guess we should add some timings on doing copies etc.

KrisThielemans · 2024-02-13T08:21:47Z

First job failure due to #1378. gcc12-cuda0 job failure: there seems to be a time-out or something in recon_test_pack/simulate_data.sh. I cannot reproduce this.

KrisThielemans · 2024-02-14T00:03:32Z

I cannot figure out what is wrong with the gcc12-cuda0 (C++-20) job.

I've used tmate to ssh in. I can then run the test without problems.
018196f added let the script output to stdout as opposed to a log file (and disabled the ctest). This goes a few more tests further and stalls at a later test at another command which just takes 1s or so (after about 34 minutes, so nothing to do with the 6 hours or so job limit).
on my local machine with similar configuration, I have no problems at all.
All other jobs are working fine.

One difference is that other jobs have their builds ccached, such that the build time is < 2min, while for this job it is still 23 mins, but I have no idea why that'd be relevant.

stumped. @casperdcl any ideas? (I could remove C++20 etc, but I can't see what this has to do with that)

KrisThielemans · 2024-02-15T10:59:56Z

I cannot figure out what is wrong with the gcc12-cuda0 (C++-20) job.

Looks like this as a disk-space issue! Seems resolved now.

KrisThielemans · 2024-02-15T13:03:53Z

gcc12-cuda0 job resolved. gcc12-cuda2.1 has a segfault in test_proj_data_info_subset, which hasn't happened before...

- make sure it works with other STIR classes, not just floats etc - re-instate VectorWithOffset constructors that take both start and end pointers for backwards compatibility

The NumericVectorWithOffset constructor would only work when T=NUMBER. The 1D Array constructor taking an `elemT*` wasn't implemented yet.

Seems that VC gets confused by the new Array::swap, so add std:: explicitly

make Array:init from ptr private test_Array is crashing due to memory bug in grow

nD Array's now store the "full" memory via shared_ptr<T[]>. This automates memory management. 1D Array's are still TODO. test_Array now works ok, except for 1D arrays. Some testing code remains in test_Array

needed for shared_ptr<T[]>, unless we'd implement deleters ourselves

all Array tests pass

no longer needed as we're using a shared<T[]> and can test that.

- get rid of copy_data argument for constructors but use sensible defaults - add VectorWithOffset::init such that Array doesn't need to know much

This was an attempt to handle pre-C++-17, but it would need work for make_shared. In any case, it isn't available on VS compilers apparently.

This was previously only done when finding the CONFIG file

KrisThielemans · 2024-05-17T11:20:41Z

I have to rebase on master again. Sorry.

if data is contiguous, we don't need an extra copy.

KrisThielemans · 2024-05-22T13:12:14Z

While more can be done here, I will merge it such that we can move on. I just need to add release notes, so anyone (@NikEfth @markus-jehl...) feels like checking, now's the time :-)

NikEfth · 2024-05-22T14:05:44Z

Hi Kris, I would like to have a look, but I won't be able before the weekend.
If you need to move forward please do so.

markus-jehl · 2024-05-22T15:10:50Z

Giving it a go with a python based reconstruction!

markus-jehl · 2024-05-22T16:25:19Z

Works fine for me :-)

NikEfth · 2024-05-22T16:26:19Z

Is it faster?

…

On Wed, May 22, 2024 at 12:25 PM markus-jehl ***@***.***> wrote: Works fine for me :-) — Reply to this email directly, view it on GitHub <#1236 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACEUB7TYH4HZM2ZAKYG7MCDZDTBINAVCNFSM6AAAAAA343XLEKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRVGIZDGNRRG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

markus-jehl · 2024-05-22T16:40:24Z

Not noticeably from my one test run, but I haven't done a proper timing comparison. That would take a bit longer.

KrisThielemans added enhancement in-progress labels Aug 24, 2023

KrisThielemans force-pushed the Array branch from 3d33555 to 6871e46 Compare August 24, 2023 14:58

KrisThielemans force-pushed the Array branch from 6871e46 to f09b35d Compare August 24, 2023 16:15

KrisThielemans mentioned this pull request Aug 29, 2023

add utility to perform timings and some performance improvements #1237

Merged

KrisThielemans mentioned this pull request Feb 4, 2024

Small improvements #1366

Merged

KrisThielemans force-pushed the Array branch from bc63f52 to 6e0e47e Compare February 12, 2024 00:28

KrisThielemans force-pushed the Array branch 2 times, most recently from b3ad8c4 to 7cfdadf Compare February 13, 2024 22:40

KrisThielemans force-pushed the Array branch 2 times, most recently from 597b35a to ce36182 Compare February 15, 2024 09:33

KrisThielemans added 15 commits May 17, 2024 12:18

Array: clean-up

49146ce

- make sure it works with other STIR classes, not just floats etc - re-instate VectorWithOffset constructors that take both start and end pointers for backwards compatibility

Array: remove wrong/not implemented constructors

86a449c

The NumericVectorWithOffset constructor would only work when T=NUMBER. The 1D Array constructor taking an `elemT*` wasn't implemented yet.

resolve std::swap for VC

205197c

Seems that VC gets confused by the new Array::swap, so add std:: explicitly

add Array constructor from ptr [ci skip]

bc5a282

make Array:init from ptr private test_Array is crashing due to memory bug in grow

Array: first version using shared_ptr<T[]> (WIP)

a399a96

nD Array's now store the "full" memory via shared_ptr<T[]>. This automates memory management. 1D Array's are still TODO. test_Array now works ok, except for 1D arrays. Some testing code remains in test_Array

require C++-17

12cec9a

needed for shared_ptr<T[]>, unless we'd implement deleters ourselves

Array: use shared_ptr<T[]> for VectorWithOffset

3c29a2a

all Array tests pass

[SWIG] ignore swap and *full_data_ptr

f4b4db8

Array: remove private member _owns_memory_for_data

2ba9924

no longer needed as we're using a shared<T[]> and can test that.

Array: clean-up constructors that take existing data

433bd82

- get rid of copy_data argument for constructors but use sensible defaults - add VectorWithOffset::init such that Array doesn't need to know much

run clang-format

0ae05b1

remove experimental/memory work-around for shared_ptr

345b0d7

This was an attempt to handle pre-C++-17, but it would need work for make_shared. In any case, it isn't available on VS compilers apparently.

remove VS 2015 job as no C++-17 for shared_ptr<float[]>

be32661

[CMake] fix FindCERN_ROOT to always check version

510e429

This was previously only done when finding the CONFIG file

[CMake] require ROOT 6.28.0 (needed for C++-17)

715ede8

optimise reading/writing for contiguous arrays

4bf2d13

KrisThielemans force-pushed the Array branch from 10f61f0 to 4bf2d13 Compare May 17, 2024 19:33

KrisThielemans added 2 commits May 17, 2024 20:40

[SWIG] avoid warning on stir::swap

74579af

Avoid image copies in Parallelproj

8752e45

if data is contiguous, we don't need an extra copy.

KrisThielemans removed the in-progress label May 22, 2024

created release_6.2.htm with info on Array PR

ae82826

KrisThielemans merged commit f3719b4 into UCL:master May 23, 2024
1 of 8 checks passed

KrisThielemans deleted the Array branch May 23, 2024 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update Array hierarchy and allocate nD arrays in a contiguous block by default #1236

update Array hierarchy and allocate nD arrays in a contiguous block by default #1236

KrisThielemans commented Aug 24, 2023 •

edited

markus-jehl commented Aug 24, 2023

markus-jehl commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

markus-jehl commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023 •

edited

markus-jehl commented Aug 25, 2023

KrisThielemans commented Aug 25, 2023

markus-jehl commented Aug 25, 2023

KrisThielemans commented Aug 30, 2023

markus-jehl commented Aug 30, 2023

KrisThielemans commented Aug 30, 2023

KrisThielemans commented Aug 31, 2023

KrisThielemans commented Feb 13, 2024

KrisThielemans commented Feb 14, 2024

KrisThielemans commented Feb 15, 2024

KrisThielemans commented Feb 15, 2024

KrisThielemans commented May 17, 2024

KrisThielemans commented May 22, 2024

NikEfth commented May 22, 2024

markus-jehl commented May 22, 2024

markus-jehl commented May 22, 2024

NikEfth commented May 22, 2024 via email

markus-jehl commented May 22, 2024

update Array hierarchy and allocate nD arrays in a contiguous block by default #1236

update Array hierarchy and allocate nD arrays in a contiguous block by default #1236

Conversation

KrisThielemans commented Aug 24, 2023 • edited

markus-jehl commented Aug 24, 2023

markus-jehl commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

markus-jehl commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023

KrisThielemans commented Aug 24, 2023 • edited

markus-jehl commented Aug 25, 2023

KrisThielemans commented Aug 25, 2023

markus-jehl commented Aug 25, 2023

KrisThielemans commented Aug 30, 2023

markus-jehl commented Aug 30, 2023

KrisThielemans commented Aug 30, 2023

KrisThielemans commented Aug 31, 2023

KrisThielemans commented Feb 13, 2024

KrisThielemans commented Feb 14, 2024

KrisThielemans commented Feb 15, 2024

KrisThielemans commented Feb 15, 2024

KrisThielemans commented May 17, 2024

KrisThielemans commented May 22, 2024

NikEfth commented May 22, 2024

markus-jehl commented May 22, 2024

markus-jehl commented May 22, 2024

NikEfth commented May 22, 2024 via email

markus-jehl commented May 22, 2024

KrisThielemans commented Aug 24, 2023 •

edited

KrisThielemans commented Aug 24, 2023 •

edited