Merge pull request #46567 from aloeliger/Fix_UCTSummary_Leak_14_1

[14_1] Fix memory leak issues in Calo Layer 1 UCT Infrastructure

Author: cmsbuild

L1Trigger/L1TCaloLayer1/plugins/L1TCaloLayer1.cc

@@ -74,16 +74,16 @@ class L1TCaloLayer1 : public edm::stream::EDProducer<> {
   edm::EDPutTokenT<L1CaloRegionCollection> regionPutToken;
   const L1TCaloLayer1FetchLUTsTokens lutsTokens;
 
-  std::vector<std::array<std::array<std::array<uint32_t, nEtBins>, nCalSideBins>, nCalEtaBins> > ecalLUT;
-  std::vector<std::array<std::array<std::array<uint32_t, nEtBins>, nCalSideBins>, nCalEtaBins> > hcalLUT;
-  std::vector<std::array<std::array<uint32_t, nEtBins>, nHfEtaBins> > hfLUT;
+  std::vector<std::array<std::array<std::array<uint32_t, nEtBins>, nCalSideBins>, nCalEtaBins>> ecalLUT;
+  std::vector<std::array<std::array<std::array<uint32_t, nEtBins>, nCalSideBins>, nCalEtaBins>> hcalLUT;
+  std::vector<std::array<std::array<uint32_t, nEtBins>, nHfEtaBins>> hfLUT;
   std::vector<unsigned long long int> hcalFBLUT;
 
   std::vector<unsigned int> ePhiMap;
   std::vector<unsigned int> hPhiMap;
   std::vector<unsigned int> hfPhiMap;
 
-  std::vector<UCTTower*> twrList;
+  std::vector<std::shared_ptr<UCTTower>> twrList;
 
   bool useLSB;
   bool useCalib;
@@ -140,7 +140,7 @@ L1TCaloLayer1::L1TCaloLayer1(const edm::ParameterSet& iConfig)
     for (uint32_t crd = 0; crd < cards.size(); crd++) {
       vector<UCTRegion*> regions = cards[crd]->getRegions();
       for (uint32_t rgn = 0; rgn < regions.size(); rgn++) {
-        vector<UCTTower*> towers = regions[rgn]->getTowers();
+        vector<std::shared_ptr<UCTTower>> towers = regions[rgn]->getTowers();
         for (uint32_t twr = 0; twr < towers.size(); twr++) {
           twrList.push_back(towers[twr]);
         }
@@ -150,7 +150,7 @@ L1TCaloLayer1::L1TCaloLayer1(const edm::ParameterSet& iConfig)
 
   // This sort corresponds to the sort condition on
   // the output CaloTowerBxCollection
-  std::sort(twrList.begin(), twrList.end(), [](UCTTower* a, UCTTower* b) {
+  std::sort(twrList.begin(), twrList.end(), [](std::shared_ptr<UCTTower> a, std::shared_ptr<UCTTower> b) {
     return CaloTools::caloTowerHash(a->caloEta(), a->caloPhi()) < CaloTools::caloTowerHash(b->caloEta(), b->caloPhi());
   });
 }

L1Trigger/L1TCaloLayer1/plugins/L1TCaloSummary.cc

@@ -195,8 +195,8 @@ void L1TCaloSummary<INPUT, OUTPUT>::produce(edm::Event& iEvent, const edm::Event
   // of size 7*2. Indices are mapped in UCTSummaryCard accordingly.
   UCTSummaryCard summaryCard =
       UCTSummaryCard(&pumLUT, jetSeed, tauSeed, tauIsolationFactor, eGammaSeed, eGammaIsolationFactor);
-  std::vector<UCTRegion*> inputRegions;
-  inputRegions.clear();
+  std::vector<UCTRegion> inputRegions;
+  inputRegions.reserve(252);  // 252 calorimeter regions. 18 phi * 14 eta
   edm::Handle<std::vector<L1CaloRegion>> regionCollection;
   if (!iEvent.getByToken(regionToken, regionCollection))
     edm::LogError("L1TCaloSummary") << "UCT: Failed to get regions from region collection!";
@@ -222,8 +222,8 @@ void L1TCaloSummary<INPUT, OUTPUT>::produce(edm::Event& iEvent, const edm::Event
     uint32_t crate = g.getCrate(t.first, t.second);
     uint32_t card = g.getCard(t.first, t.second);
     uint32_t region = g.getRegion(absCaloEta, absCaloPhi);
-    UCTRegion* test = new UCTRegion(crate, card, negativeEta, region, fwVersion);
-    test->setRegionSummary(i.raw());
+    UCTRegion test = UCTRegion(crate, card, negativeEta, region, fwVersion);
+    test.setRegionSummary(i.raw());
     inputRegions.push_back(test);
     //This *should* fill the tensor in the proper order to be fed to the anomaly model
     //We take 4 off of the GCT eta/iEta.
@@ -282,9 +282,10 @@ void L1TCaloSummary<INPUT, OUTPUT>::produce(edm::Event& iEvent, const edm::Event
   double phi = -999.;
   double mass = 0;
 
-  std::list<UCTObject*> boostedJetObjs = summaryCard.getBoostedJetObjs();
-  for (std::list<UCTObject*>::const_iterator i = boostedJetObjs.begin(); i != boostedJetObjs.end(); i++) {
-    const UCTObject* object = *i;
+  std::list<std::shared_ptr<UCTObject>> boostedJetObjs = summaryCard.getBoostedJetObjs();
+  for (std::list<std::shared_ptr<UCTObject>>::const_iterator i = boostedJetObjs.begin(); i != boostedJetObjs.end();
+       i++) {
+    const std::shared_ptr<UCTObject> object = *i;
     pt = ((double)object->et()) * caloScaleFactor * boostedJetPtFactor;
     eta = g.getUCTTowerEta(object->iEta());
     phi = g.getUCTTowerPhi(object->iPhi());

L1Trigger/L1TCaloLayer1/src/UCTLayer1.cc

@@ -62,7 +62,7 @@ const UCTRegion* UCTLayer1::getRegion(int regionEtaIndex, uint32_t regionPhiInde
   return region;
 }
 
-const UCTTower* UCTLayer1::getTower(int caloEta, int caloPhi) const {
+const std::shared_ptr<UCTTower> UCTLayer1::getTower(int caloEta, int caloPhi) const {
   if (caloPhi < 0) {
     LOG_ERROR << "UCT::getTower - Negative caloPhi is unacceptable -- bailing" << std::endl;
     exit(1);
@@ -71,7 +71,7 @@ const UCTTower* UCTLayer1::getTower(int caloEta, int caloPhi) const {
   UCTTowerIndex twr = UCTTowerIndex(caloEta, caloPhi);
   const UCTRegionIndex rgn = g.getUCTRegionIndex(twr);
   const UCTRegion* region = getRegion(rgn);
-  const UCTTower* tower = region->getTower(twr);
+  const std::shared_ptr<UCTTower> tower = region->getTower(twr);
   return tower;
 }
 

L1Trigger/L1TCaloLayer1/src/UCTLayer1.hh

@@ -2,6 +2,7 @@
 #define UCTLayer1_hh
 
 #include <vector>
+#include <memory>
 
 class UCTCrate;
 class UCTRegion;
@@ -36,7 +37,7 @@ public:
 
   std::vector<UCTCrate*>& getCrates() { return crates; }
   const UCTRegion* getRegion(UCTRegionIndex r) const { return getRegion(r.first, r.second); }
-  const UCTTower* getTower(UCTTowerIndex t) const { return getTower(t.first, t.second); }
+  const std::shared_ptr<UCTTower> getTower(UCTTowerIndex t) const { return getTower(t.first, t.second); }
 
   // To zero out event in case of selective tower filling
   bool clearEvent();
@@ -58,7 +59,7 @@ private:
   // Helper functions
 
   const UCTRegion* getRegion(int regionEtaIndex, uint32_t regionPhiIndex) const;
-  const UCTTower* getTower(int caloEtaIndex, int caloPhiIndex) const;
+  const std::shared_ptr<UCTTower> getTower(int caloEtaIndex, int caloPhiIndex) const;
 
   //Private data
 

L1Trigger/L1TCaloLayer1/src/UCTRegion.cc

@@ -68,6 +68,14 @@ uint32_t getHitTowerLocation(uint32_t* et) {
   return iAve;
 }
 
+UCTRegion::UCTRegion(const UCTRegion& otherRegion)
+    : crate(otherRegion.crate),
+      card(otherRegion.card),
+      region(otherRegion.region),
+      towers(otherRegion.towers),
+      regionSummary(otherRegion.regionSummary),
+      fwVersion(otherRegion.fwVersion) {}
+
 UCTRegion::UCTRegion(uint32_t crt, uint32_t crd, bool ne, uint32_t rgn, int fwv)
     : crate(crt), card(crd), region(rgn), negativeEta(ne), regionSummary(0), fwVersion(fwv) {
   UCTGeometry g;
@@ -76,24 +84,17 @@ UCTRegion::UCTRegion(uint32_t crt, uint32_t crd, bool ne, uint32_t rgn, int fwv)
   towers.clear();
   for (uint32_t iEta = 0; iEta < nEta; iEta++) {
     for (uint32_t iPhi = 0; iPhi < nPhi; iPhi++) {
-      towers.push_back(new UCTTower(crate, card, ne, region, iEta, iPhi, fwVersion));
+      towers.push_back(std::make_shared<UCTTower>(crate, card, ne, region, iEta, iPhi, fwVersion));
     }
   }
 }
 
-UCTRegion::~UCTRegion() {
-  for (uint32_t i = 0; i < towers.size(); i++) {
-    if (towers[i] != nullptr)
-      delete towers[i];
-  }
-}
-
-const UCTTower* UCTRegion::getTower(uint32_t caloEta, uint32_t caloPhi) const {
+const std::shared_ptr<UCTTower> UCTRegion::getTower(uint32_t caloEta, uint32_t caloPhi) const {
   UCTGeometry g;
   uint32_t nPhi = g.getNPhi(region);
   uint32_t iEta = g.getiEta(caloEta);
   uint32_t iPhi = g.getiPhi(caloPhi);
-  UCTTower* tower = towers[iEta * nPhi + iPhi];
+  std::shared_ptr<UCTTower> tower = towers[iEta * nPhi + iPhi];
   return tower;
 }
 
@@ -229,7 +230,7 @@ bool UCTRegion::setECALData(UCTTowerIndex t, bool ecalFG, uint32_t ecalET) {
   uint32_t absCaloPhi = abs(t.second);
   uint32_t iEta = g.getiEta(absCaloEta);
   uint32_t iPhi = g.getiPhi(absCaloPhi);
-  UCTTower* tower = towers[iEta * nPhi + iPhi];
+  std::shared_ptr<UCTTower> tower = towers[iEta * nPhi + iPhi];
   return tower->setECALData(ecalFG, ecalET);
 }
 
@@ -244,7 +245,7 @@ bool UCTRegion::setHCALData(UCTTowerIndex t, uint32_t hcalFB, uint32_t hcalET) {
     // Valid data are:
     //    absCaloEta = 30-39, 1 < absCaloPhi <= 72 (every second value)
     for (uint32_t iPhi = iPhiStart; iPhi < iPhiStart + 2; iPhi++) {  // For artificial splitting in half
-      UCTTower* tower = towers[iEta * nPhi + iPhi];
+      std::shared_ptr<UCTTower> tower = towers[iEta * nPhi + iPhi];
       // We divide by 2 in output section, after LUT
       if (!tower->setHFData(hcalFB, hcalET))
         return false;
@@ -253,14 +254,14 @@ bool UCTRegion::setHCALData(UCTTowerIndex t, uint32_t hcalFB, uint32_t hcalET) {
     // Valid data are:
     //    absCaloEta = 40,41, 1 < absCaloPhi <= 72 (every fourth value)
     for (uint32_t iPhi = 0; iPhi < 4; iPhi++) {  // For artificial splitting in quarter
-      UCTTower* tower = towers[iEta * nPhi + iPhi];
+      std::shared_ptr<UCTTower> tower = towers[iEta * nPhi + iPhi];
       // We divide by 4 in output section, after LUT
       if (!tower->setHFData(hcalFB, hcalET))
         return false;
     }
   } else {
     uint32_t iPhi = g.getiPhi(absCaloPhi);
-    UCTTower* tower = towers[iEta * nPhi + iPhi];
+    std::shared_ptr<UCTTower> tower = towers[iEta * nPhi + iPhi];
     return tower->setHCALData(hcalFB, hcalET);
   }
   return true;

L1Trigger/L1TCaloLayer1/src/UCTRegion.hh

@@ -3,6 +3,7 @@
 
 #include <vector>
 #include <iostream>
+#include <memory>
 
 #include "UCTTower.hh"
 
@@ -30,19 +31,19 @@ public:
 
   UCTRegion() = delete;
 
-  // No copy constructor is needed
-
-  UCTRegion(const UCTRegion&) = delete;
+  //copy constructor helps gettting around some memory leakage
+  // and ownership problems in the summary card emulation
+  UCTRegion(const UCTRegion&);
 
   // No equality operator is needed
 
   const UCTRegion& operator=(const UCTRegion&) = delete;
 
-  virtual ~UCTRegion();
+  virtual ~UCTRegion() = default;
 
   // To setData for towers before processing
 
-  const std::vector<UCTTower*>& getTowers() { return towers; }
+  const std::vector<std::shared_ptr<UCTTower>>& getTowers() { return towers; }
 
   // To process event
 
@@ -92,14 +93,14 @@ public:
 
   const bool isNegativeEta() const { return negativeEta; }
 
-  const UCTTower* getTower(UCTTowerIndex t) const { return getTower(t.first, t.second); }
+  const std::shared_ptr<UCTTower> getTower(UCTTowerIndex t) const { return getTower(t.first, t.second); }
 
   friend std::ostream& operator<<(std::ostream&, const UCTRegion&);
 
 protected:
   // Helper functions
 
-  const UCTTower* getTower(uint32_t caloEta, uint32_t caloPhi) const;
+  const std::shared_ptr<UCTTower> getTower(uint32_t caloEta, uint32_t caloPhi) const;
 
   // Region location definition
 
@@ -110,7 +111,7 @@ protected:
 
   // Owned region level data
 
-  std::vector<UCTTower*> towers;
+  std::vector<std::shared_ptr<UCTTower>> towers;
 
   uint32_t regionSummary;
 

L1Trigger/L1TCaloLayer1/src/UCTSummaryCard.cc

@@ -40,13 +40,6 @@ UCTSummaryCard::UCTSummaryCard(const std::vector<std::vector<std::vector<uint32_
   }
 }
 
-UCTSummaryCard::~UCTSummaryCard() {
-  for (uint32_t i = 0; i < regions.size(); i++) {
-    if (regions[i] != nullptr)
-      delete regions[i];
-  }
-}
-
 bool UCTSummaryCard::process() {
   clearEvent();
   UCTGeometry g;
@@ -126,10 +119,10 @@ bool UCTSummaryCard::process() {
   double mhtPhi = (atan2(sumHy, sumHx) * 180. / 3.1415927) + 180.;
   int mhtIPhi = (int)(72. * (mhtPhi / 360.));
 
-  ET = new UCTObject(UCTObject::ET, etValue, 0, metIPhi, pileup, 0, 0);
-  HT = new UCTObject(UCTObject::HT, htValue, 0, mhtIPhi, pileup, 0, 0);
-  MET = new UCTObject(UCTObject::MET, metValue, 0, metIPhi, pileup, 0, 0);
-  MHT = new UCTObject(UCTObject::MHT, mhtValue, 0, mhtIPhi, pileup, 0, 0);
+  ET = std::make_shared<UCTObject>(UCTObject::ET, etValue, 0, metIPhi, pileup, 0, 0);
+  HT = std::make_shared<UCTObject>(UCTObject::HT, htValue, 0, mhtIPhi, pileup, 0, 0);
+  MET = std::make_shared<UCTObject>(UCTObject::MET, metValue, 0, metIPhi, pileup, 0, 0);
+  MHT = std::make_shared<UCTObject>(UCTObject::MHT, mhtValue, 0, mhtIPhi, pileup, 0, 0);
 
   // Then sort the candidates for output usage
   emObjs.sort();
@@ -365,12 +358,16 @@ bool UCTSummaryCard::processRegion(UCTRegionIndex center) {
   if (centralET >= northET && centralET >= nwET && centralET >= westET && centralET >= swET && centralET > southET &&
       centralET > seET && centralET > eastET && centralET > neET && centralET > jetSeed) {
     if (centralRegion)
-      centralJetObjs.push_back(new UCTObject(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3));
+      centralJetObjs.push_back(
+          std::make_shared<UCTObject>(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3));
     else
-      forwardJetObjs.push_back(new UCTObject(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3));
+      forwardJetObjs.push_back(
+          std::make_shared<UCTObject>(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3));
   }
 
-  auto boostedJet = new UCTObject(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3);
+  //auto boostedJet = new UCTObject(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3);
+  std::shared_ptr<UCTObject> boostedJet =
+      std::make_shared<UCTObject>(UCTObject::jet, jetET, hitCaloEta, hitCaloPhi, pu3x3, 0, et3x3);
   boostedJet->setNTaus(nTauLike);
   boostedJet->setBoostedJetRegionET(boostedJetRegionET);
   boostedJet->setBoostedJetRegionTauVeto(boostedJetRegionTauVeto);
@@ -419,13 +416,15 @@ bool UCTSummaryCard::processRegion(UCTRegionIndex center) {
       }
     }
     if (tauET != 0) {
-      tauObjs.push_back(new UCTObject(UCTObject::tau, tauET, hitCaloEta, hitCaloPhi, tauPU, 0xDEADBEEF, et3x3));
+      tauObjs.push_back(
+          std::make_shared<UCTObject>(UCTObject::tau, tauET, hitCaloEta, hitCaloPhi, tauPU, 0xDEADBEEF, et3x3));
       // Subtract footprint
       uint32_t isolation = 0;
       if (et3x3 > tauET)
         isolation = et3x3 - tauET;
       if (isolation < ((uint32_t)(tauIsolationFactor * (double)tauET))) {
-        isoTauObjs.push_back(new UCTObject(UCTObject::isoTau, tauET, hitCaloEta, hitCaloPhi, pu3x3, isolation, et3x3));
+        isoTauObjs.push_back(
+            std::make_shared<UCTObject>(UCTObject::isoTau, tauET, hitCaloEta, hitCaloPhi, pu3x3, isolation, et3x3));
       }
     }
   }
@@ -474,13 +473,14 @@ bool UCTSummaryCard::processRegion(UCTRegionIndex center) {
       }
     }
     if (eGammaET != 0) {
-      emObjs.push_back(new UCTObject(UCTObject::eGamma, eGammaET, hitCaloEta, hitCaloPhi, eGammaPU, 0xDEADBEEF, et3x3));
+      emObjs.push_back(std::make_shared<UCTObject>(
+          UCTObject::eGamma, eGammaET, hitCaloEta, hitCaloPhi, eGammaPU, 0xDEADBEEF, et3x3));
       uint32_t isolation = 0;
       if (et3x3 > eGammaET)
         isolation = et3x3 - eGammaET;
       if (isolation < ((uint32_t)(eGammaIsolationFactor * (double)eGammaET))) {
-        isoEMObjs.push_back(
-            new UCTObject(UCTObject::isoEGamma, eGammaET, hitCaloEta, hitCaloPhi, pu3x3, isolation, et3x3));
+        isoEMObjs.push_back(std::make_shared<UCTObject>(
+            UCTObject::isoEGamma, eGammaET, hitCaloEta, hitCaloPhi, pu3x3, isolation, et3x3));
       }
     }
   }
@@ -528,10 +528,10 @@ const UCTRegion* UCTSummaryCard::getRegion(int regionEtaIndex, uint32_t regionPh
     edm::LogError("L1TCaloSummary") << "UCTSummaryCard: Incorrect region requested -- bailing" << std::endl;
     exit(1);
   }
-  return regions[i];
+  return &regions[i];
 }
 
-bool UCTSummaryCard::setRegionData(std::vector<UCTRegion*> inputRegions) {
+bool UCTSummaryCard::setRegionData(std::vector<UCTRegion> inputRegions) {
   for (long unsigned int i = 0; i < inputRegions.size(); i++) {
     regions.push_back(inputRegions[i]);
   }