ATLASRECTS-5057: Avoid holes search in GSF
Merge request reports
Activity
added Egamma InnerDetector Reconstruction Tracking master review-pending-level-1 labels
Resolved by Roger NaranjoHi better to put it here over the many small comments.
I would I have gone with adding in the summaryTool
+updateTrackNoHoleSearch(Track& track) const +{ + // first check if track has summary already. + if (track.m_trackSummary!=nullptr) { + delete track.m_trackSummary; + track.m_trackSummary = nullptr; + } + track.m_trackSummary=summaryNoHoleSearch(track).release(); + return; +} +and then in the code call this and pick Trk::TrackSummary* summary = Info.Track->trackSummary();
or doing something equivalent given the track is non const inside the EMBremCollectionBuilder so no need to add to the tool
Trk::TrackSummary* summary = Info.Track->trackSummary(); if(summary != nullptr){ delete summary; summary = nullptr; } summary = summaryNoHoleSearch(*(Info.Track)).release();And then update that one. Now
- you call createTrackSummaryNoHoleSearch, which calls summaryNoHoleSearch .
- then you go via the path of
const_castalthough you have non-const track - then you get a const summary (although you could have avoided it)
- which leads to another allocation via make_unique to get a non-const copy ...
So 2 allocations + 1
const_castwhen you need just 1 allocation ...Edited by Christos Anastopoulos- Last reply by Roger Naranjo
Athena AthSimulation externals cmake make required tests optional tests Full details available at NICOS MR-26513-2019-09-12-00-52
- Resolved by Roger Naranjo
On another note since
std::unique_ptr<Trk::TrackSummary> Trk::TrackSummaryTool::createSummary( const Track& track, bool doHolesInDet, bool doHolesMuon) const { // first check if track has summary already and then return a clone // (remember the TrackSummaryTool is a factory!) if (track.trackSummary()!=nullptr) { ATH_MSG_DEBUG ("Return cached summary for author : "<<track.info().dumpInfo()); return std::make_unique<Trk::TrackSummary>(*(track.trackSummary())); }is always called . e.g. (even after the removal you did)
Trk::TrackSummary* ts= createSummary(track,m_doHolesInDet, m_doHolesMuon ).release(); Trk::Track& nonConstTrack = const_cast<Trk::Track&>(track); if (onlyUpdateTrack) { // not returning summary. Add it directly the track nonConstTrack.m_trackSummary = ts; ts=nullptr; // returning nullptr } else { // need to return summary too. So add a copy to the track nonConstTrack.m_trackSummary = new Trk::TrackSummary(*ts); } return ts;-
not sure why you removed the comment on the "clone".
-
I am bit scheptical on the changes you did there as you now create a clone and you can overwrite what was there without delete ...
before if something was there you were getting just clone. Now you create a clone for it anyhow, and then you go on also to do
nonConstTrack.m_trackSummary = new Trk::TrackSummary(*ts);overriding what was there with a clone of itself without deleting it , so mem leak alert ...Edited by Christos Anastopoulos -
- Last reply by Roger Naranjo
- Resolved by Roger Naranjo
So I tried to implement solution in my comments above. Not 100% sure it will work but should give an idea of what I mean concretely.
-
Athena AthSimulation externals cmake make required tests optional tests Full details available at NICOS MR-26513-2019-09-12-05-18
-
Athena AthSimulation externals cmake make required tests optional tests Full details available at NICOS MR-26513-2019-09-12-05-28
I'll approve this, but I would like to bring up a point of concern for me. I personally think that the memory management as per discussion above is fairly confusing. It would be nice to clean it all up and consistently use smart pointer throughout instead of release() it to a raw pointer, which hard to keep track. However, that part has already been discussed quite a bit, and they look ok for now.--MR L1
added review-approved label and removed review-pending-level-1 label
-
Hey to do it throughout one needs to change also the Trk::Track class (if we discuss this after my changes). As this hold a plain Trk::TrackSummary* which it owns. So the release is used to tranfer the summary to the Trk::Track.
This design can indeed become a bit tricky as you manually remove the old and add the new so perhaps it worths to change it at the Trk::Track level, but is another level down in the code ...
if the Trk::Track was holding a unique_ptr rather than plain then the release would be move or reset and would not need the manual delete of the existing . So yes there is scope if we touch the EDM class.
Edited by Christos Anastopoulos mentioned in commit cd876e27
added sweep:ignore label
mentioned in merge request !26586 (merged)