Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MFT] Move static arrays to be initialized before the processing starts #10665

Merged
merged 5 commits into from
Feb 6, 2023

Conversation

robincaron13
Copy link
Collaborator

To follow PR#10654, here the arrays are kept as static member and are moved to the tracker class to be initialized before the processing starts (not sure if it was what you were thinking @shahor02)

Copy link
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @robincaron13
Thanks for taking care, but this not what I meant: moving the static array from the base class to the derived one will change nothing: it will still be allocated w/o the creation of the actual object, i.e. in any process which links to this library.
Instead, I would define in the TrackerConfig.h

using BinContainer = std::array<std::array<std::array<std::vector<Int_t>, constants::index_table::MaxRPhiBins>, (constants::mft::LayersNumber - 1)>, (constants::mft::LayersNumber - 1)>;

static std_unique_ptr<BinContainer> mBins;
static std_unique_ptr<BinContainer> mBinsS;
static initBinContainers() { // implementation can go to cxx file
  if (!mBins) {
    mBins = std::make_unique<BinContainer>();
    initBins(mBins.get()); // init bins with static method. If not possible to do using static method, then make this method non-static and lock the code by mutex.
   //
  }
  // same for mBinsS
}

In the TrackerSpec call this initBinContainers before instantiating the trackers (e.g. in its init method (or in the beginning of updateTimeDependentParams) if this method can be static.
Otherwise, put the non-static method (protected by mutex) in the Tracker constructed.
Same for destroying arrays via mBins.reset() : do this either in the Tracker destructor of the protecting the call with mutex or in the TrackerSpec destructor.

For the usage of the mutex see e.g. how the

is used in
std::mutex GeometryManager::sTGMutex;
//______________________________________________________________________
Bool_t GeometryManager::getOriginalMatrix(const char* symname, TGeoHMatrix& m)
{
m.Clear();
if (!gGeoManager || !gGeoManager->IsClosed()) {
LOG(error) << "No active geometry or geometry not yet closed!";
;
return kFALSE;
}
std::lock_guard<std::mutex> guard(sTGMutex);
and below.

@robincaron13
Copy link
Collaborator Author

Hi @shahor02,

Thanks a lot for your answer and your proposal.
Currently I'm making the changes but I have a question regarding the initBins. What should contain this function exactly? Because from my understanding an arrays is intialized at the moment we define its size but here it's now about std::unique_ptr, also I tried different way like for example using std::unique_ptr<BinContainer>(new BinContainer) and no success... Sorry I'm not an expert in the use of some functionalities in c++.

@shahor02
Copy link
Collaborator

shahor02 commented Feb 2, 2023

I have a question regarding the initBins. What should contain this function exactly?
Hi @robincaron13
The initialization of the uniqe_ptr will just allocate the memory but not assign the values you need.
The initBins should contain what

mBinsS[layer1][layer2 - 1][binIndex1].emplace_back(binIndex2S);
}
}
int binwPhi = mPhiBinWin[layer2];
int binhwPhi = binwPhi / 2;
for (Int_t binR = rBinMin; binR <= rBinMax; ++binR) {
for (Int_t iPhi = 0; iPhi < binwPhi; ++iPhi) {
int binPhi = binPhi_proj + (iPhi - binhwPhi);
if (binPhi < 0) {
continue;
}
binIndex2 = getBinIndex(binR, binPhi);
mBins[layer1][layer2 - 1][binIndex1].emplace_back(binIndex2);
does: assignment of the values.
I did not check if
void Tracker<T>::initializeFinder()
does anything else but setting mBins(S) values, if this function should be called only once before the processing starts, then this is actually the "initBins" I meant.

@robincaron13
Copy link
Collaborator Author

robincaron13 commented Feb 2, 2023

Hi @shahor02,

Thanks for your answer. Some modifications are pushed according to your comments.
For the moment I still kept a dummy static initBins in the TrackerConfig.cxx to do nothing, also basically the initBinConntainer is just doing mBins = std::make_unique<BinContainer>(); and it should be called once in the init of the TrackerSpec.cxx. The mBins.reset() is called through the new destructor called in TrackerSpec.cxx. The mutex is also added to the intializeFinder funnction where the assignment of the mBins values are perfomed.
Let me know if it goes in the good direction, or there are still modifications to do.

Copy link
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @robincaron13
In general ok, but please see some comments below.

Detectors/ITSMFT/MFT/tracking/src/Tracker.cxx Outdated Show resolved Hide resolved
Detectors/ITSMFT/MFT/tracking/src/Tracker.cxx Outdated Show resolved Hide resolved
Detectors/ITSMFT/MFT/tracking/src/Tracker.cxx Outdated Show resolved Hide resolved
Detectors/ITSMFT/MFT/tracking/src/Tracker.cxx Outdated Show resolved Hide resolved
Detectors/ITSMFT/MFT/tracking/src/TrackerConfig.cxx Outdated Show resolved Hide resolved
Detectors/ITSMFT/MFT/workflow/src/TrackerSpec.cxx Outdated Show resolved Hide resolved
Detectors/ITSMFT/MFT/workflow/src/TrackerSpec.cxx Outdated Show resolved Hide resolved
@robincaron13 robincaron13 requested a review from shahor02 February 3, 2023 09:55
@robincaron13
Copy link
Collaborator Author

Hi @shahor02
Let me know if the last changes added were fine with you. Thanks.

Copy link
Collaborator

@shahor02 shahor02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants