Fix track parameter related memory management in the extrapolator
- replaces the garbage bin for the track parameters by some smart pointer solution.
- fix some missing track parameter cleanup in some clients of the extrapolator.
The "smart pointer" solution ensures that during the extrapolation track parameters which are still in use are not destructed. This fixes the crash reported in ATLASRECTS-5340
Merge request reports
Activity
q221 and q431 run and produce identical outputs. (I did not test q220,q222,q223). Also the first 100 events of the job for which a crash was reported in ATLASRECTS-5340 (in event 78 to 88), runs successfully (I did not compare the outputs). For the latter job I checked valgrind output for 10 events starting at event 78, no invalid reads, writes or jumps are reported by valgrind in the extrpolator or its callers, nor are there any memory leaks initiated by the extrapolator. In q221 and q431, I only tested that all track parameters created by the extrapolator are also cleaned up, and that during the extrapolation no pointers to invalid track parameters are passed around.
I still have to cleanup the commit history and validate the doxygen comments, I would also happily take good suggestion concerning class or method names and of course any other comments.
added 806 commits
-
ff07f6a6...bf272ca3 - 803 commits from branch
atlas:master
- 2814ee6b - Replace track parameter related memory management in the extrapolator.
- 11eaa4f5 - Use smart pointer to manage track parameters.
- 9dd8feef - Git do not leak track parameters created by the extrapolator.
Toggle commit list-
ff07f6a6...bf272ca3 - 803 commits from branch
Rebased to master 2020-03-04. And applied small optimisation and warning fixes for the unit test.
q221,q222,q431 run with unchanged output, the data15 test from ATLASRECTS-5340 crashes in the unpatched release and succeeds when patched.
Edited by Goetz GayckenThis merge request affects 4 packages:
- InnerDetector/InDetRecTools/InDetTrackHoleSearch
- MuonSpectrometer/MuonReconstruction/MuonRecTools/MuonTGRecTools
- Tracking/TrkExtrapolation/TrkExTools
- Tracking/TrkFitter/TrkiPatFitter
Adding @goetz ,@amorley ,@wleight ,@oda ,@sroe ,@nkoehler ,@rosati ,@pop as watchers
added InnerDetector MuonSpectrometer Tracking master review-pending-level-1 labels
✅ CI Result SUCCESS (hash cb8a35ae)Athena AthSimulation externals ✅ ✅ cmake ✅ ✅ make ⚠ ✅ required tests ✅ ✅ optional tests ✅ ✅ Full details available on this CI monitor view
⚠ Athena: number of compilation errors 0, warnings 1
✅ AthSimulation: number of compilation errors 0, warnings 0
📝 For experts only: Jenkins output [CI-MERGE-REQUEST-CC7 10498]Hi I will personally need to digest the new class.
Basically, I think I understand what you do, but making this kind of thing the norm, would prb something one wants to do if any of the simpler solution can not work. Or at least one needs to understand it in a very good level here. i.e having a wrapper to allow container to manage things with different lifetimes.
For ATLASRECTS-5340 perhaps one should remove the offending line not in Rel 21 and in master that my understanding is that prb does not fix the issue it got introduced for either if we want something quick.
E.g. since we return new objects why not unique_ptr from the impl and internal methods at least. Then expand to the external if possible (clients). make the garbage a
sink
[1] somove
etc can clarify when things are not to be used at least this is what we did in the GSF . OK Smaller code, but similar issues and took some steps (one by one) but the result is much cleaner as the ownership is transparent 100% and not hidden anymore.As said ATLASRECTS-5340 prb can be patched short term by doing what is there in rel 21 or at least this is what I got from @wleight
[1]
std::vector<unique_ptr<...> >
as anyhow the.second
has not practical use from what I have seen in most similar cases .... .Then this forces the ptr you create for internal use and then want to clean to be unique, then is visible when you move, and then you do not want to move things you release back. and then is a bit more visible in a loop that you want to move to thing to the sink outside it . It will auto clean when the cache having as member goes out of scope (so no need to call explicit empty all the time). Then if you manage to also make things to return unique_ptr (if to non-const also is golden ... since then is std factory methods) then you prb can solve a few problems ... At least this was the case with the
GSFExtrapolator
.This
0312 mutable std::map< const MultiComponentState*, bool > m_mcsGarbageBin; //!< Garbage bin for MultiComponentState objects 0313 mutable std::map< const TrackParameters*, bool > m_tpGarbageBin; //!< Garbage bin for TrackParameter objects
std::vector<std::unique_ptr<const MultiComponentState>> 0155 m_mcsGarbageBin; //!< Garbage bin for MultiComponentState objects 0156 std::vector<std::unique_ptr<const TrackParameters>> m_tpGarbageBin; //!< Garbage bin for TrackParameter objects
did a big change on cleaning things up there.
Edited by Christos Anastopoulosadded 4 commits
Toggle commit listThis merge request affects 4 packages:
- InnerDetector/InDetRecTools/InDetTrackHoleSearch
- MuonSpectrometer/MuonReconstruction/MuonRecTools/MuonTGRecTools
- Tracking/TrkExtrapolation/TrkExTools
- Tracking/TrkFitter/TrkiPatFitter
Adding @goetz ,@amorley ,@wleight ,@oda ,@sroe ,@nkoehler ,@rosati ,@pop as watchers
Hi @christos,
in principle it is possible to use unique pointers and clone track parameters many times (initial track parameters and when ever they are kept for reference or later use), or use shared pointers and clone the initial ones and those which are returned. In the end a lot of parameters need to be cloned. Using shared pointers would presumably be the easier option compared to unique pointers, since track parameters are often shared. But no matter which solution is picked a lot of changes are necessary.
It is hard to understand the track parameter management in the extrapolator. Boundary track parameters are kept, parameters are stored for navigation, a fallback is kept, they are stored in return vectors either directly or as part of TrackStateOnSurfaces, they are passed to the updater which sometimes creates new objects sometimes returns the same object etc. . It is possible that the quick fix might be the solution, though after changing the management scheme, some memory leaks surfaced in calling tools. So, I suspect further errors in the "garbage" handling in the extrapolator. One can continue patching or try to solve it. In the past I only patched ...
Cheers, Götz
Edited by Goetz Gaycken