Skip to content
Snippets Groups Projects

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

Edited by Goetz Gaycken

Merge request reports

Pipeline #1494079 passed

Pipeline passed for f71183ca on goetz:Extrapolator_master_fix_memory_management

Merged by Frank WinklmeierFrank Winklmeier 4 years ago (Mar 20, 2020 8:38am UTC)

Merge details

  • Changes merged into with 83a36e73.
  • Did not delete the source branch.

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Developer

    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.

  • Goetz Gaycken added 1 commit

    added 1 commit

    Compare with previous version

  • Goetz Gaycken added 3 commits

    added 3 commits

    • 1a873a7d - Replace track parameter related memory management in the extrapolator.
    • b59f63d9 - Use smart pointer to manage track parameters.
    • ff07f6a6 - Git do not leak track parameters created by the extrapolator.

    Compare with previous version

  • Author Developer

    squashed commits, and removed one commit which was not intended for distribution.

  • Goetz Gaycken added 806 commits

    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.

    Compare with previous version

  • Goetz Gaycken added 2 commits

    added 2 commits

    • 4c207269 - Do not search for a matching object since the reference it could have is known.
    • bb3a7ae1 - Silence threads safety checker for the unit test.

    Compare with previous version

  • Author Developer

    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 Gaycken
  • Goetz Gaycken added 3 commits

    added 3 commits

    • 13630907 - Replace track parameter related memory management in the extrapolator.
    • 9a292631 - Use smart pointer to manage track parameters.
    • cb8a35ae - Do not leak track parameters created by the extrapolator.

    Compare with previous version

  • Author Developer

    squashed commits.

  • Goetz Gaycken unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Goetz Gaycken changed the description

    changed the description

  • WARNING: big files (>100K) are found in the changeset

    📝 116K in file Tracking/TrkFitter/TrkiPatFitter/src/MaterialAllocator.cxx

    📝 268K in file Tracking/TrkExtrapolation/TrkExTools/src/Extrapolator.cxx

  • This 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

  • 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] so move 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

    https://acode-browser1.usatlas.bnl.gov/lxr/source/athena/Tracking/TrkFitter/TrkGaussianSumFilter/TrkGaussianSumFilter/GsfExtrapolator.h?%21v=21.0 to

     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

    https://acode-browser1.usatlas.bnl.gov/lxr/source/athena/Tracking/TrkFitter/TrkGaussianSumFilter/TrkGaussianSumFilter/GsfExtrapolator.h?v=21.0&%21v=head

    did a big change on cleaning things up there.

    Edited by Christos Anastopoulos
  • Goetz Gaycken added 4 commits

    added 4 commits

    • ef5339ff - Replace track parameter related memory management in the extrapolator.
    • 96a49e35 - Use smart pointer to manage track parameters.
    • da5754ac - Do not leak track parameters created by the extrapolator.
    • 6a6b6ce2 - Fix handling of extrapolator method sequence number.

    Compare with previous version

  • WARNING: big files (>100K) are found in the changeset

    📝 116K in file Tracking/TrkFitter/TrkiPatFitter/src/MaterialAllocator.cxx

    📝 268K in file Tracking/TrkExtrapolation/TrkExTools/src/Extrapolator.cxx

  • This 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

  • Author Developer

    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
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading