Memory leak in eicrecon::ParticlesWithTruthPID::process(...) (ParticlesWithTruthPID.cpp:160)

wdconinc commented 1 year ago

Environment: (where does this bug occur, have you tried other environments)

Which branch (often main for latest released): main
Which version (or HEAD for the most recent on git): HEAD
Any specific OS or system where the issue occurs? CI

Steps to reproduce: (give a step by step account of how to trigger the bug)

https://github.com/eic/EICrecon/actions/runs/3795776388
- valgrind-memcheck-eicrecon-pi-arches.log

Expected Result: (what do you expect when you execute the steps above)

No memory leaks.

Actual Result: (what do you get when you execute the steps above)

Multiple memory leaks reported in ParticlesWithTruthPID.cpp:160:

==46== 64,656 (768 direct, 63,888 indirect) bytes in 96 blocks are definitely lost in loss record 16,875 of 17,314
==46==    at 0x4840F2F: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==46==    by 0x8188B09: eicrecon::ParticlesWithTruthPID::process(std::vector<edm4hep::MCParticle const*, std::allocator<edm4hep::MCParticle const*> > const&, std::vector<edm4eic::TrackParameters const*, std::allocator<edm4eic::TrackParameters const*> > const&) (ParticlesWithTruthPID.cpp:160)
==46==    by 0x942E60E: eicrecon::ParticlesWithTruthPID_factory::Process(std::shared_ptr<JEvent const> const&) (ParticlesWithTruthPID_factory.cc:69)
==46==    by 0x8F8D921: JFactoryT<eicrecon::ParticlesWithAssociation>::GetOrCreate(std::shared_ptr<JEvent const> const&, JApplication*, int) (JFactoryT.h:119)
==46==    by 0x8F8C74B: eicrecon::ParticlesWithAssociation const* JEvent::GetSingle<eicrecon::ParticlesWithAssociation>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (JEvent.h:255)
==46==    by 0x8F96B63: eicrecon::ReconstructedParticles_factory::Process(std::shared_ptr<JEvent const> const&) (ReconstructedParticles_factory.cc:30)
==46==    by 0x8DD4D91: JFactoryT<edm4eic::ReconstructedParticle>::GetOrCreate(std::shared_ptr<JEvent const> const&, JApplication*, int) (JFactoryT.h:119)
==46==    by 0xB12BFD9: std::vector<edm4eic::ReconstructedParticle const*, std::allocator<edm4eic::ReconstructedParticle const*> > JEvent::Get<edm4eic::ReconstructedParticle>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (JEvent.h:291)
==46==    by 0xB121F8F: GetFactoryObjects<edm4eic::ReconstructedParticle, edm4eic::ReconstructedParticleCollection>::operator()(std::shared_ptr<JEvent const> const&, JFactory*) (JEventProcessorPODIO.cc:39)
==46==    by 0xB11C9D6: std::optional<unsigned long> CallWithPODIOType<GetFactoryObjects, unsigned long, std::shared_ptr<JEvent const> const&, JFactory*>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::shared_ptr<JEvent const> const&, JFactory*) (datamodel_glue.h:611)
==46==    by 0xB117A2D: JEventProcessorPODIO::Process(std::shared_ptr<JEvent const> const&) (JEventProcessorPODIO.cc:319)
==46==    by 0x583D96A: JEventProcessor::DoMap(std::shared_ptr<JEvent const> const&) (in /home/runner/work/EICrecon/EICrecon/lib/EICrecon/plugins/log.so)

Additional context

         /// Resulting associations
        std::vector<edm4eic::MCRecoParticleAssociation *> associations;

// ...

           if (best_match >= 0) {
                auto rec_assoc = edm4eic::MutableMCRecoParticleAssociation();
                rec_assoc.setRecID(rec_part.getObjectID().index);
                rec_assoc.setSimID(mc_particles[best_match]->getObjectID().index);
                rec_assoc.setWeight(1);
                rec_assoc.setRec(rec_part);
                auto sim = mc_particles[best_match]->clone();
                rec_assoc.setSim(sim);
                associations.emplace_back(new edm4eic::MCRecoParticleAssociation(rec_assoc));

                if (m_log->level() <= spdlog::level::debug) {

                    const auto &mcpart = mc_particles[best_match];
                    const auto &p = mcpart->getMomentum();
                    const auto p_mag = edm4eic::magnitude(p);
                    const auto p_phi = edm4eic::angleAzimuthal(p);
                    const auto p_theta = edm4eic::anglePolar(p);
                    m_log->debug(" MCPart: {:<4} {:<8.3f} {:<8.3f} {:<8.2f} {:<6}",
                                 mcpart->getObjectID().index, p_mag, p_theta, p_phi, mcpart->getCharge(),
                                 mcpart->getPDG());

                    m_log->debug(" Assoc: id={} SimId={} RecId={}",
                                 rec_assoc.getObjectID().index, rec_assoc.getSimID(), rec_assoc.getSimID());
                }
            }
            else {
                m_log->debug(" MCPart: Did not find a good match");
            }

            // Add particle to the output vector
            reco_particles.push_back(new edm4eic::ReconstructedParticle(rec_part));
        }

        // Assembling the results
        return new ParticlesWithAssociation(std::move(reco_particles), std::move(associations));
    }

I think you're relying on the allocation in associations.emplace_back being cleaned up after transferring ownership with the move in the return (where the std::move has probably no effect anyway). But there is no automatic cleanup of the contents of the std::vector<edm4eic::MCRecoParticleAssociation *> associations, so that's just leaking again.

wdconinc commented 1 year ago

This is not fixed. See logs at https://github.com/eic/EICrecon/actions/runs/4320254537. Because of the ad hoc type that is needed to handle return of associations in jana factories, this isn't as easy a fix as elsewhere.

wdconinc commented 1 year ago

Moving to podio collections, which handle memory management, this issue has been resolved.

eic / EICrecon