Skip to content

Commit

Permalink
fix: memory leak in CalorimeterClusterRecoCoG (#974)
Browse files Browse the repository at this point in the history
### Briefly, what does this PR introduce?
A memory leak was caused by creating a new pointer `cl`, then push_back
`*cl`, but not deleting the pointer. This modifies the algorithm to use
objects instead of pointers. RVO and podio object encapsulation should
ensure performance is not affected.

Leak sanitizer output resolved:
```
Direct leak of 108512 byte(s) in 13564 object(s) allocated from:
    #0 0x55cbc7e9fded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 4702f11c9afe7b8bff422367fb6901ebdee7ea40)
    #1 0x7f92f7afe251 in eicrecon::CalorimeterClusterRecoCoG::reconstruct(edm4eic::ProtoCluster const&) const /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:293:10
    #2 0x7f92f7af9dc9 in eicrecon::CalorimeterClusterRecoCoG::process(edm4eic::ProtoClusterCollection const*, edm4hep::SimCalorimeterHitCollection const*) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:55:18
    #3 0x7f92f7cb9713 in eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:73:48
    #4 0x7f92f7cc0e54 in JMultifactoryHelperPodio<edm4eic::MCRecoClusterParticleAssociation>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20
    #5 0x7f92ffc7ccf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3)

Direct leak of 45664 byte(s) in 5708 object(s) allocated from:
    #0 0x55cbc7e9fded in operator new(unsigned long) (/home/wdconinc/git/EICrecon/.worktree/main/bin/eicrecon+0xeaded) (BuildId: 4702f11c9afe7b8bff422367fb6901ebdee7ea40)
    #1 0x7f92f7afe251 in eicrecon::CalorimeterClusterRecoCoG::reconstruct(edm4eic::ProtoCluster const&) const /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:293:10
    #2 0x7f92f7af9dc9 in eicrecon::CalorimeterClusterRecoCoG::process(edm4eic::ProtoClusterCollection const*, edm4hep::SimCalorimeterHitCollection const*) /home/wdconinc/git/EICrecon/.worktree/main/src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc:55:18
    #3 0x7f92f7cb9713 in eicrecon::CalorimeterClusterRecoCoG_factoryT::Process(std::shared_ptr<JEvent const> const&) /home/wdconinc/git/EICrecon/.worktree/main/src/factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h:73:48
    #4 0x7f92f7cbac34 in JMultifactoryHelperPodio<edm4eic::Cluster>::Process(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.1-txdpt66ozlvvdquuovpy44emsmsnuip4/include/JANA/JMultifactory.h:228:20
    #5 0x7f92ffc7ccf7 in JFactory::Create(std::shared_ptr<JEvent const> const&) (/usr/._local/jhttvx4bcm3wdo7jmamfg6pazkythsai/lib/libJANA.so+0x59cf7) (BuildId: e11aaf0447ef572861e6577e3cfce0750b7aa2c3)
```

### What kind of change does this PR introduce?
- [x] Bug fix (issue #__)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

Co-authored-by: Dmitry Kalinkin <[email protected]>
  • Loading branch information
wdconinc and veprbl authored Sep 17, 2023
1 parent 6e91f99 commit 14508d0
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 11 deletions.
23 changes: 13 additions & 10 deletions src/algorithms/calorimetry/CalorimeterClusterRecoCoG.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,13 +52,16 @@ namespace eicrecon {
auto associations = std::make_unique<edm4eic::MCRecoClusterParticleAssociationCollection>();

for (const auto& pcl : *proto) {
auto *cl = reconstruct(pcl);

// skip null clusters
if (cl == nullptr) continue;
// skip protoclusters with no hits
if (pcl.hits_size() == 0) {
continue;
}

auto cl = reconstruct(pcl);

m_log->debug("{} hits: {} GeV, ({}, {}, {})", cl->getNhits(), cl->getEnergy() / dd4hep::GeV, cl->getPosition().x / dd4hep::mm, cl->getPosition().y / dd4hep::mm, cl->getPosition().z / dd4hep::mm);
clusters->push_back(*cl);
m_log->debug("{} hits: {} GeV, ({}, {}, {})", cl.getNhits(), cl.getEnergy() / dd4hep::GeV, cl.getPosition().x / dd4hep::mm, cl.getPosition().y / dd4hep::mm, cl.getPosition().z / dd4hep::mm);
clusters->push_back(cl);

// If mcHits are available, associate cluster with MCParticle
// 1. find proto-cluster hit with largest energy deposition
Expand Down Expand Up @@ -124,10 +127,10 @@ namespace eicrecon {

// set association
auto clusterassoc = associations->create();
clusterassoc.setRecID(cl->getObjectID().index); // if not using collection, this is always set to -1
clusterassoc.setRecID(cl.getObjectID().index); // if not using collection, this is always set to -1
clusterassoc.setSimID(mcp.getObjectID().index);
clusterassoc.setWeight(1.0);
clusterassoc.setRec(*cl);
clusterassoc.setRec(cl);
clusterassoc.setSim(mcp);
} else {
m_log->debug("No mcHitCollection was provided, so no truth association will be performed.");
Expand All @@ -138,15 +141,15 @@ namespace eicrecon {
}

//------------------------------------------------------------------------
edm4eic::Cluster* CalorimeterClusterRecoCoG::reconstruct(const edm4eic::ProtoCluster& pcl) const {
edm4eic::Cluster CalorimeterClusterRecoCoG::reconstruct(const edm4eic::ProtoCluster& pcl) const {
edm4eic::MutableCluster cl;
cl.setNhits(pcl.hits_size());

m_log->debug("hit size = {}", pcl.hits_size());

// no hits
if (pcl.hits_size() == 0) {
return nullptr;
return cl;
}

// calculate total energy, find the cell with the maximum energy deposit
Expand Down Expand Up @@ -290,7 +293,7 @@ edm4eic::Cluster* CalorimeterClusterRecoCoG::reconstruct(const edm4eic::ProtoClu
cl.addToShapeParameters( eigenValues_3D[1].real() ); // 3D x-y-z cluster width 2
cl.addToShapeParameters( eigenValues_3D[2].real() ); // 3D x-y-z cluster width 3

return new edm4eic::Cluster(cl);
return std::move(cl);
}

} // eicrecon
2 changes: 1 addition & 1 deletion src/algorithms/calorimetry/CalorimeterClusterRecoCoG.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ namespace eicrecon {

private:

edm4eic::Cluster* reconstruct(const edm4eic::ProtoCluster& pcl) const;
edm4eic::Cluster reconstruct(const edm4eic::ProtoCluster& pcl) const;

};

Expand Down

0 comments on commit 14508d0

Please sign in to comment.