Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

algorithms/calorimetry: pass PODIO collections instead of std::vector #741

Merged
merged 23 commits into from
Jul 23, 2023

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Jul 5, 2023

This fixes memory leaks like:

Direct leak of 407056 byte(s) in 50882 object(s) allocated from:
    #0 0x55e48bee007d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 5069f7d2185cef71541ac1e93e2f6643618de2c4)
    #1 0x7fa316881fc5 in CalorimeterHitDigi::signal_sum_digi() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:254:23
    #2 0x7fa3168807eb in CalorimeterHitDigi::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:127:9
    #3 0x7fa3150b2e0c in RawCalorimeterHit_factory_LFHCALRawHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h:86:9
    #4 0x7fa315a0fcc1 in eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
    #5 0x7fa315a4e8a8 in JFactoryT<edm4hep::RawCalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
    #6 0x7fa315a4d87b in std::vector<edm4hep::RawCalorimeterHit const*, std::allocator<edm4hep::RawCalorimeterHit const*> > JEvent::Get<edm4hep::RawCalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27

Leaks were discovered by LSAN that was enabled on CI recently #514.

Alternative way is to free the memory manually #731, or fix bug in JANA#742.

veprbl added 6 commits July 5, 2023 15:21
…tead of std::vector

This fixes memory leaks like:

```
Direct leak of 407056 byte(s) in 50882 object(s) allocated from:
    #0 0x55e48bee007d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 5069f7d2185cef71541ac1e93e2f6643618de2c4)
    #1 0x7fa316881fc5 in CalorimeterHitDigi::signal_sum_digi() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:254:23
    #2 0x7fa3168807eb in CalorimeterHitDigi::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:127:9
    #3 0x7fa3150b2e0c in RawCalorimeterHit_factory_LFHCALRawHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h:86:9
    #4 0x7fa315a0fcc1 in eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
    #5 0x7fa315a4e8a8 in JFactoryT<edm4hep::RawCalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
    #6 0x7fa315a4d87b in std::vector<edm4hep::RawCalorimeterHit const*, std::allocator<edm4hep::RawCalorimeterHit const*> > JEvent::Get<edm4hep::RawCalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
```
…tead of std::vector

This fixes memory leaks like:

    Direct leak of 389136 byte(s) in 48642 object(s) allocated from:
        #0 0x562ee9bce07d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 32bcf2da69e5409b22003a41cdb3023b1ebdbb21)
        #1 0x7fe7e2189715 in CalorimeterHitReco::AlgorithmProcess() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitReco.cc:240:20
        #2 0x7fe7e114a70c in CalorimeterHit_factory_EcalBarrelScFiRecHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelScFiRecHits.h:88:9
        #3 0x7fe7e133f8a1 in eicrecon::JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
        #4 0x7fe7e1352148 in JFactoryT<edm4eic::CalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
        #5 0x7fe7e135096b in std::vector<edm4eic::CalorimeterHit const*, std::allocator<edm4eic::CalorimeterHit const*> > JEvent::Get<edm4eic::CalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
…ad of std::vector

This fixes a memory leak:

    Direct leak of 640736 byte(s) in 80092 object(s) allocated from:
        #0 0x562ee9bce07d in operator new(unsigned long) (/home/runner/work/EICrecon/EICrecon/bin/eicrecon+0xeb07d) (BuildId: 32bcf2da69e5409b22003a41cdb3023b1ebdbb21)
        #1 0x7fe7e11690b2 in ImagingPixelReco::execute() /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/ImagingPixelReco.h:133:32
        #2 0x7fe7e11674e9 in CalorimeterHit_factory_EcalBarrelImagingRecHits::Process(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelImagingRecHits.h:65:9
        #3 0x7fe7e133f8a1 in eicrecon::JFactoryPodioT<edm4eic::CalorimeterHit>::Create(std::shared_ptr<JEvent const> const&) /home/runner/work/EICrecon/EICrecon/src/services/io/podio/JFactoryPodioT.h:171:13
        #4 0x7fe7e1352148 in JFactoryT<edm4eic::CalorimeterHit>::GetOrCreate(std::shared_ptr<JEvent const> const&) /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JFactoryT.h:83:13
        #5 0x7fe7e135096b in std::vector<edm4eic::CalorimeterHit const*, std::allocator<edm4eic::CalorimeterHit const*> > JEvent::Get<edm4eic::CalorimeterHit>(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const /opt/software/linux-debian12-x86_64_v2/gcc-12.2.0/jana2-2.1.0-hj25lfdff2mcbllcvr5lojwa4sqpnpb5/include/JANA/JEvent.h:325:27
@veprbl veprbl changed the title algorithms/calorimetry/CalorimeterHitDigi: pass PODIO collections instead of std::vector algorithms/calorimetry: pass PODIO collections instead of std::vector Jul 5, 2023
@veprbl
Copy link
Member Author

veprbl commented Jul 5, 2023

My understanding, we'd need to eventually deprecate JFactoryPodioT altogether and replace it with JChainFactoryT and JChainMultifactoryT. Without any other refactoring, it would like many changes along the lines of:

Expand
diff --git a/src/detectors/B0ECAL/B0ECAL.cc b/src/detectors/B0ECAL/B0ECAL.cc
index d75ec611..03e7c740 100644
--- a/src/detectors/B0ECAL/B0ECAL.cc
+++ b/src/detectors/B0ECAL/B0ECAL.cc
@@ -3,6 +3,7 @@
 //
 //
 
+#include <extensions/jana/JChainFactoryGeneratorT.h>
 #include <extensions/jana/JChainMultifactoryGeneratorT.h>
 
 #include <factories/calorimetry/CalorimeterClusterRecoCoG_factoryT.h>
@@ -37,7 +38,7 @@ extern "C" {
 
         InitJANAPlugin(app);
 
-        app->Add(new JFactoryGeneratorT<RawCalorimeterHit_factory_B0ECalRawHits>());
+        app->Add(new JChainFactoryGeneratorT<RawCalorimeterHit_factory_B0ECalRawHits>({"B0ECalRawHits"}, "B0ECalRawHits"));
         app->Add(new JFactoryGeneratorT<CalorimeterHit_factory_B0ECalRecHits>());
         app->Add(new JFactoryGeneratorT<ProtoCluster_factory_B0ECalTruthProtoClusters>());
         app->Add(new JFactoryGeneratorT<ProtoCluster_factory_B0ECalIslandProtoClusters>());
diff --git a/src/detectors/B0ECAL/RawCalorimeterHit_factory_B0ECalRawHits.h b/src/detectors/B0ECAL/RawCalorimeterHit_factory_B0ECalRawHits.h
index f829bb53..627dd091 100644
--- a/src/detectors/B0ECAL/RawCalorimeterHit_factory_B0ECalRawHits.h
+++ b/src/detectors/B0ECAL/RawCalorimeterHit_factory_B0ECalRawHits.h
@@ -12,7 +12,7 @@
 #include <Evaluator/DD4hepUnits.h>
 #include <JANA/JEvent.h>
 
-#include <services/io/podio/JFactoryPodioT.h>
+#include <extensions/jana/JChainFactoryT.h>
 #include <services/geometry/dd4hep/JDD4hep_service.h>
 #include <algorithms/calorimetry/CalorimeterHitDigi.h>
 #include <services/log/Log_service.h>
@@ -20,15 +20,15 @@
 
 
 
-class RawCalorimeterHit_factory_B0ECalRawHits : public eicrecon::JFactoryPodioT<edm4hep::RawCalorimeterHit>, CalorimeterHitDigi {
+class RawCalorimeterHit_factory_B0ECalRawHits : public JChainFactoryT<edm4hep::RawCalorimeterHit>, CalorimeterHitDigi {
 
 public:
 
     //------------------------------------------
     // Constructor
-    RawCalorimeterHit_factory_B0ECalRawHits() {
-        SetTag("B0ECalRawHits");
-        m_log = japp->GetService<Log_service>()->logger(GetTag());
+    RawCalorimeterHit_factory_B0ECalRawHits(std::vector<std::string> default_input_tags)
+      : JChainFactoryT<edm4hep::RawCalorimeterHit>(std::move(default_input_tags))
+    {
     }
 
     //------------------------------------------
@@ -52,6 +52,7 @@ public:
 
         m_geoSvc = app->GetService<JDD4hep_service>(); // TODO: implement named geometry service?
 
+        m_log = japp->GetService<Log_service>()->logger("B0ECAL:B0ECalRawHits");
 
         // This is another option for exposing the data members as JANA configuration parameters.
         app->SetDefaultParameter("B0ECAL:B0ECalRawHits:input_tag",        m_input_tag, "Name of input collection to use");

But let's not do it in this PR.

@veprbl
Copy link
Member Author

veprbl commented Jul 5, 2023

Basically we'll need #666 but for every algorithm.

@veprbl veprbl force-pushed the pr/calo_alg_use_collections branch from dc16c74 to 6702133 Compare July 6, 2023 13:19
@wdconinc
Copy link
Contributor

wdconinc commented Jul 6, 2023

Basically we'll need #666 but for every algorithm.

Don't you wish we'd done this before we had the great Cambrian explosion in factories...

…ns instead of std::vector (no multifactory conversion)
@veprbl veprbl marked this pull request as ready for review July 11, 2023 10:35
@veprbl
Copy link
Member Author

veprbl commented Jul 11, 2023

This produces an identical output as main when processing brycecanyon DIS on CI. Should be good to review.

@veprbl veprbl requested review from wdconinc and c-dilks July 11, 2023 10:38
src/algorithms/calorimetry/CalorimeterHitDigi.cc Outdated Show resolved Hide resolved
src/algorithms/calorimetry/CalorimeterHitDigi.cc Outdated Show resolved Hide resolved
src/algorithms/calorimetry/CalorimeterHitReco.cc Outdated Show resolved Hide resolved
src/algorithms/calorimetry/CalorimeterHitReco.h Outdated Show resolved Hide resolved
src/algorithms/calorimetry/CalorimeterIslandCluster.h Outdated Show resolved Hide resolved
src/algorithms/calorimetry/ImagingPixelReco.h Show resolved Hide resolved
@veprbl veprbl force-pushed the pr/calo_alg_use_collections branch from b054cbd to 2350206 Compare July 14, 2023 21:38
veprbl added 4 commits July 14, 2023 17:47
…ollections instead of std::vector (no multifactory conversion)"

This reverts commit b0fd11f.

Bad change. Should remove algorithm altogether.
@veprbl veprbl force-pushed the pr/calo_alg_use_collections branch from 7a36eb0 to 9490348 Compare July 18, 2023 01:37
@veprbl veprbl force-pushed the pr/calo_alg_use_collections branch from 427c8ea to 61b8c26 Compare July 19, 2023 18:28
@veprbl veprbl force-pushed the pr/calo_alg_use_collections branch from 61b8c26 to 7b7d6b2 Compare July 19, 2023 18:39
@veprbl veprbl requested review from wdconinc and c-dilks July 23, 2023 18:25
@veprbl
Copy link
Member Author

veprbl commented Jul 23, 2023

Took out changes to IslandClustering algo (to be submitted separately)

…ctions

 Conflicts:
	src/detectors/B0ECAL/CalorimeterHit_factory_B0ECalRecHits.h
	src/detectors/B0ECAL/ProtoCluster_factory_B0ECalTruthProtoClusters.h
	src/detectors/B0ECAL/RawCalorimeterHit_factory_B0ECalRawHits.h
	src/detectors/B0ECAL/TruthCluster_factory_B0ECalTruthProtoClusters.h
	src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelImagingRecHits.h
	src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelScFiMergedHits.h
	src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelScFiRecHits.h
	src/detectors/BEMC/CalorimeterHit_factory_EcalBarrelSciGlassRecHits.h
	src/detectors/BEMC/ProtoCluster_factory_EcalBarrelSciGlassTruthProtoClusters.h
	src/detectors/BEMC/RawCalorimeterHit_factory_EcalBarrelImagingRawHits.h
	src/detectors/BEMC/RawCalorimeterHit_factory_EcalBarrelScFiRawHits.h
	src/detectors/BEMC/RawCalorimeterHit_factory_EcalBarrelSciGlassRawHits.h
	src/detectors/BHCAL/CalorimeterHit_factory_HcalBarrelRecHits.h
	src/detectors/BHCAL/ProtoCluster_factory_HcalBarrelTruthProtoClusters.h
	src/detectors/BHCAL/RawCalorimeterHit_factory_HcalBarrelRawHits.h
	src/detectors/EEMC/CalorimeterHit_factory_EcalEndcapNRecHits.h
	src/detectors/EEMC/ProtoCluster_factory_EcalEndcapNTruthProtoClusters.h
	src/detectors/EEMC/RawCalorimeterHit_factory_EcalEndcapNRawHits.h
	src/detectors/EHCAL/CalorimeterHit_factory_HcalEndcapNMergedHits.h
	src/detectors/EHCAL/CalorimeterHit_factory_HcalEndcapNRecHits.h
	src/detectors/EHCAL/ProtoCluster_factory_HcalEndcapNTruthProtoClusters.h
	src/detectors/EHCAL/RawCalorimeterHit_factory_HcalEndcapNRawHits.h
	src/detectors/FEMC/CalorimeterHit_factory_EcalEndcapPInsertRecHits.h
	src/detectors/FEMC/CalorimeterHit_factory_EcalEndcapPRecHits.h
	src/detectors/FEMC/ProtoCluster_factory_EcalEndcapPInsertTruthProtoClusters.h
	src/detectors/FEMC/ProtoCluster_factory_EcalEndcapPTruthProtoClusters.h
	src/detectors/FEMC/RawCalorimeterHit_factory_EcalEndcapPInsertRawHits.h
	src/detectors/FEMC/RawCalorimeterHit_factory_EcalEndcapPRawHits.h
	src/detectors/FHCAL/CalorimeterHit_factory_HcalEndcapPInsertMergedHits.h
	src/detectors/FHCAL/CalorimeterHit_factory_HcalEndcapPInsertRecHits.h
	src/detectors/FHCAL/CalorimeterHit_factory_HcalEndcapPMergedHits.h
	src/detectors/FHCAL/CalorimeterHit_factory_HcalEndcapPRecHits.h
	src/detectors/FHCAL/CalorimeterHit_factory_LFHCALRecHits.h
	src/detectors/FHCAL/ProtoCluster_factory_HcalEndcapPInsertTruthProtoClusters.h
	src/detectors/FHCAL/ProtoCluster_factory_HcalEndcapPTruthProtoClusters.h
	src/detectors/FHCAL/ProtoCluster_factory_LFHCALTruthProtoClusters.h
	src/detectors/FHCAL/RawCalorimeterHit_factory_HcalEndcapPInsertRawHits.h
	src/detectors/FHCAL/RawCalorimeterHit_factory_HcalEndcapPRawHits.h
	src/detectors/FHCAL/RawCalorimeterHit_factory_LFHCALRawHits.h
	src/detectors/LUMISPECCAL/CalorimeterHit_factory_EcalLumiSpecRecHits.h
	src/detectors/LUMISPECCAL/ProtoCluster_factory_EcalLumiSpecTruthProtoClusters.h
	src/detectors/LUMISPECCAL/RawCalorimeterHit_factory_EcalLumiSpecRawHits.h
	src/detectors/ZDC/CalorimeterHit_factory_ZDCEcalRecHits.h
	src/detectors/ZDC/ProtoCluster_factory_ZDCEcalTruthProtoClusters.h
	src/detectors/ZDC/RawCalorimeterHit_factory_ZDCEcalRawHits.h
@veprbl veprbl force-pushed the pr/calo_alg_use_collections branch from 3bc0dd8 to 95ab9b7 Compare July 23, 2023 19:59
Copy link
Contributor

@wdconinc wdconinc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any remaining comments here (and I'll go and resolve any conversations of mine that may still be open).

@veprbl veprbl enabled auto-merge (squash) July 23, 2023 22:28
@veprbl veprbl merged commit 9af9308 into main Jul 23, 2023
@veprbl veprbl deleted the pr/calo_alg_use_collections branch July 23, 2023 22:32
veprbl added a commit that referenced this pull request Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants