algo: Add PODAllocator and PODVector.

Adds definitions for PODAllocator and PODVector. PODAllocator is a allocator that doesn't initialize the underlying memory. This can greatly improve the performance of std::vector when working with trivial data types where memory initialization is not always required. This is in particular for the parallel Unpacker, where the initial vector::resize makes up 80% of the runtime.

These types should be part of algo. However they also have to be used by CbmDigiData. So they are defined in core/base with Cbm-prefix and also have typedefs in cbm::algo namespace instead.

MR should be ready from my side. However these are pretty wideranging changes. So I wanted to discuss them in the Online meeting before removing the draft tag.

changed the description

added Framework Online labels

As this is changing the type of a member variable, this will need a bump of the version index for the ROOT streamer in all CbmXxxDigiData classes, for example for CbmTofDigidata around line 49-50: !1221 (diffs)

Hopefully that should be enough to get automatic versions compatibility.
But as we also have data written with these classes, it should also be checked whether the automatic streamer evolution works with these existing files or if a custom streamer is needed to tell ROOT how to convert from std::vector<T> to CbmPODVector<T>. I would guess it should be a trivial OP that even ROOT can do but I confess I am not expert in overloading the standard container allocators at all and ROOT was until recently picky with only the standard containers by themselves.

added 1 commit

• 9e6b2f7a - CbmDigiData: Replace std::vector with PODVector.

Compare with previous version

Fixed names of installed headers.

added 1 commit

• 284f9fa3 - CbmXxxDigiData: Bump root class version.

Compare with previous version

added 1 commit

• f181b5f9 - PODAllocator: Fix construct function.

Compare with previous version

added 6 commits

• f181b5f9...8a4c0a88 - 2 commits from branch computing:master
• 7fb3b6f6 - algo: Add PODAllocator and PODVector.
• 64441b12 - CbmDigiData: Replace std::vector with PODVector.
• 18b29121 - CbmXxxDigiData: Bump root class version.
• 19cf4c0a - PODAllocator: Fix construct function.

Compare with previous version

marked this merge request as draft

changed title from Drat: algo: Add PODAllocator and PODVector. to Draft: algo: Add PODAllocator and PODVector.

assigned to @j.decuveland

As suggested by @f.uhlig, could you please talk briefly to Matthias Kretz m.kretz@gsi.de about this problem? He is involved in the development of the C++ language and may be able to advise us if this is the best solution.

requested review from @v.friese

mentioned in merge request !1047 (closed)

Before this MR fully falls into oblivion, I'd like to make an attempt to revive it. :-)

First off, Matthias Kretz answered to our problem a while ago. Here's (my translation of) his answer:

[...] std::vector::resize does value-init. I.e. std::vector<int> is initialized with int(). https://stackoverflow.com/questions/21028299 seems to be the best solution if you want to continue using std::vector.

Since the allocator is part of the type, [move between different allocators] can only be avoided with a polymorphic allocator. So use std::pmr::vector everywhere.

I can only advise what you're currently doing. But the following alternatives are possible:

1. replace const std::vector<T>& with std::span<T> in function arguments
2. replace std::vector x; x.resize(N); with std::unique_ptr<std::array<T, N>>. Allocation via std::make_unique_for_overwrite<std::array<T, N>>()
3. replace std::vector x; x.resize(N); with std::unique_ptr<T[]>. Allocation via std::make_unique_for_overwrite<T[]>(N).
4. Fully disjoint data structures for different threds (each Thread has it's own std::vector).

I'm not sure std::pmr::vector would help us much. Polymorphic allocators sound like they would create more issues than they solve. For his other suggestions:

1. I'm much in favor of using spans where possible. But this doesn't solve this particular issue.
2. Don't think this applies. We don't know the array size at compile time.
3. Now we're back to C-arrays with RAII memory management. This just seems like a step backward compared to PODVector with no additional benefits?
4. We already do that, but we need to gather the data afterwards...

So all in all, my take away is that we're on the right track with this approach and it doesn't look like there are any attractive alternatives...

Secondly, I think this MR can be simplified. It's not necessary right now to change the storage types. We can just keep them and only change the containers used by the algorithms. I see two advantages to this:

• We're still stuck with an additional copy, but it can happen before we create the archive. This allows us in the future to move it in a background thread with the archive creation while the next time slice is processed. If this turns out to be too slow we can easily adjust the storage containers later.
• We separate the storage classes from processing. In my opinion this is desirable anyway because if we find similar optimizations in the future, it will be easier to make the changes.

Interesting to see that every mail from Matthias actually makes me look up C++ constructs I didn't know before.

Concerning the failing tests of the current version of the MR (maybe disappearing if the MR is simplified as proposed by @fweig):

1. The failure of _GTestCbmStsDigi.CheckDefaultConstructor is kind of obvious as the test checks the default zeroing which is removed in the MR
   => Probably check has to be fixed/removed
2. The failing mcbm_reco_toftr_digievent_2022_2391 is more strange, as it seems to be something happening when getting an initial pointer to the data from FairRoot during the conversion of CbmDigiEvents to CbmRecoEvents. Probably because I had to give to the FairRootManager the type of data to load at l.134 of CbmTaskMakeRecoEvents
   => I suspect that something as to be changed in frm->InitObjectAs<const std::vector<CbmDigiEvent>*>("DigiEvent"); or in the allocator of the CbmDigiEvent class

Also interesting to see that most of Matthias' suggestions imply the use of C++20 or C++23.

I agree that using a polymorpic_allocator is not really required in our case. This solutions seems best for us and very well implemented to me.