User-defined copy constructor makes class non-POD (though there are tricks to declare it as POD) but your copy constructor makes the same what
Cluster(const Cluster& clu) = default would do, please remove it.

do you really need doubles? If not, prefer floats.

since you are using default c-tor, you better initialize data members, e.g.
int mMulDigit = 0; //...

the same is here, you don't initialize data members

since you have at most 12k cells, you can save 4 bytes in the size of this class by defining it as

  int mLabel;       ///< Index of the corresponding entry/entries in the MC label array
  float mAmplitude; ///< Amplitude
  float mTime;      ///< Time
  short mAbsId;       ///< cell index (absolute cell ID)
  bool mIsHighGain; ///< High Gain or Low Gain channel (for calibration)

remove as the default one will do the same

again, do you need double for energy?

Dear Ruben,
I hope I implemented all your suggestions in the current version.
Dmitri

could you please indent properly?

if you move char mMulDigit .. up by 1 line, you will save 4 bytes (due to the alignment)

done

as an input arguments instead of const std::vector<xxx> one should use gsl/span<xxx>. Note that this interface allows to pass both vectors and spans.
See https://alice.its.cern.ch/jira/browse/O2-1017 for details.

replaced input arguments except MCContainer: not sure span will work with its complicated structure

Yes, mc container at the moment need to be copied

idem here, spans instead of the vectors.

Here also. Please check all similar cases.

instead of the init loop it is much more efficient to memset(digitsUsed,0,sizeof(bool)*maxNDigits)

done

... BTW, once you change input vectors to spans, you should query it as a vector rather then a pointer (i.e. simply digits[i]). Also note that vector::at(i) (as well as a span::at(i) is slower than the vector[i], span[i] since the former is doing a range check (not needed in the correct code).

vector::erase is very expensive procedure (requires copy for all elements after the erased one. If you cannot avoid clusters elimination and it happens often, you better fill 1st a temporary vector, flag entries which should be eliminated, then copy surviving ones to the final vector.

our coding guidelines disadvise using explicit new / delete, you better do
std::unique_ptr<int[]> maxAt = std::make_unique<int[]>(o2::phos::PHOSSimParams::Instance().mNLMMax)
Then you don't need to delete it.

As I pass it to other methods, use vector instead of unique_ptr

As far as I understand, FullCluster vector is used only is an internal container, right? (as this class itself contains a vector, storing or messaging will be very expensive). In this case you don't need to declare a vector here, it is needed for the serialization only.

done

Since all these vectors are modified together, it is much more efficient to define e.g. a nested

struct ListElem {
short digID;
float energy;
int timev;
std::pair<int, float> lbl;
};

and add as a data member just 1 vector<ListElem>.
This will also simplify your multi-stage iterator loops in the methods below.

Also, why do you need a pointer datamember mPHOSGeom, which anyway will point on the same instance of the Geometry::GetInstance()?
You can either provide it as an argument of the methods wich use it, or get in these methods on the fly from that static getter.

Good idea - implemented.

Also here erase in the loop will lead to performance penalty (especially done on separate vectors, but I hope you will reduce it to single vector, see comment above).
Since these clusters are anyway transient objects, can't you simply flag the digits you want to suppress (e.g. by assigning negative time etc.) and then simply ignore flagged entries?

I left erase in few places: these very rare cases it is more efficient to move 5 ~CelElements then check special tag (e.g. energy) for all good elements in all clusters.

instead of dividing in the loop by mFullEnergy, it is more efficient to calculate once outside of the loop the 1./mFullEnergy and then use multiplication.

why don't you use simply std::find? Note that you Digit has no custom operator== used in the search, so the compiler will generate for you an automatic one which will compare all data members. I guess this is not what you want?
BTW, such a light-weight methods like operators in the Digit is better to define inline.

I added operator == This is copy-past of std::find slightly adopted to this case.

this can be constexpr.

Removed this rudiment

...e/(2*b*b)); will be faster

we are switching to spans instead of vectors when receiving the inputs, see https://alice.its.cern.ch/jira/browse/O2-1017 for details.

using span here instead of vector causes
[7968:PHOSClusterizerSpec]: [20:51:44][ERROR] Exception caught: Inconsistent serialization method for extracting span

could you send me full error message?

here is full log: https://drive.google.com/file/d/1UYuvAhLxcGZIhs2j7pHNbeMGmwpw7XXv/view?usp=sharing
For some reason I can not attach it directly here...
Dmitri

looks like the TriggerRecords are not messaged via shared memory but serialized. This is strange since you've copied the class from the emcal and there it is messagable. I will need to build your branch and test myself, but will not to do this today...

There might be problems with TR as well, but when I tried to debug a bit I found that exception is thrown while getting Digits. With vectors (as in EMCAL) it works well.
Thanks, Dmitri

I've tried with the same outcomes as yours. The problem comes from the Publisher/RootTreeReader. Will try to debug tomorrow with Giulio. Meanwhile, I see that you have conflicting branchnames as PHS... and PHOS..., could you bring them to unique standard (preferaby PHS)?

Thank you, Ruben. Concerning branch naming: I always use data producer PHS while in branch names I always use PHOS. I will cross-check.

Hi @peressounko
The RootTreeReader was now fixed, so you can use spans instead of vectors (but you need to add a few lines in the publisher and setting up the reader, see 4abc548)

Cheers,
Ruben