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.

Sign up for GitHub

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

Jump to bottom

Fix L1TCaloSummary memory leak in UCTSummaryCard related to UCTRegion… #40735

Merged
merged 2 commits into from Feb 24, 2023
Merged

Fix L1TCaloSummary memory leak in UCTSummaryCard related to UCTRegion… #40735

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9393b6
Fix L1TCaloSummary memory leak in UCTSummaryCard related to UCTRegion…
aloeliger Feb 9, 2023
14b494a
Change summaryCard to local variable
aloeliger Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 3 additions & 6 deletions L1Trigger/L1TCaloLayer1/plugins/L1TCaloSummary.cc
View file
Open in desktop
Expand Up @@ -148,13 +148,9 @@ L1TCaloSummary::L1TCaloSummary(const edm::ParameterSet& iConfig)
}
}
produces<L1JetParticleCollection>("Boosted");
summaryCard = new UCTSummaryCard(&pumLUT, jetSeed, tauSeed, tauIsolationFactor, eGammaSeed, eGammaIsolationFactor);
}

L1TCaloSummary::~L1TCaloSummary() {
if (summaryCard != nullptr)
delete summaryCard;
}
L1TCaloSummary::~L1TCaloSummary() {}

//
// member functions
Expand All @@ -172,7 +168,7 @@ void L1TCaloSummary::produce(edm::Event& iEvent, const edm::EventSetup& iSetup)
// independently creating regions from TPGs for processing by the summary card. This results
// in a single region vector of size 252 whereas from independent creation we had 3*6 vectors
// of size 7*2. Indices are mapped in UCTSummaryCard accordingly.
summaryCard->clearRegions();
summaryCard = new UCTSummaryCard(&pumLUT, jetSeed, tauSeed, tauIsolationFactor, eGammaSeed, eGammaIsolationFactor);
Copy link
Contributor

Choose a reason for hiding this comment

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

too bad the first review didn't suggest to use smart pointers here...

but at this point, why not just make this a local variable?

UCTSummaryCard summaryCard(&pumLUT, jetSeed, tauSeed...);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seem to recall looking at this problem a while ago, and trying to make changes to smart pointers, running into some thing that explicitly expected a pointer, and quitting before I ended up needing to make changes to a bunch of crucial L1 Emulator legacy code.

However, looking again, I can't find where that would be an issue in this code, and I don't see a reason why it couldn't just be a local variable either.

I'll make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am very probably suggesting this in the wrong place, but in the future, I think an implementation here:

cmssw/L1Trigger/L1TCaloLayer1/src/UCTSummaryCard.cc

Lines 502 to 505 in b2a6f4e

bool UCTSummaryCard::clearRegions() {
regions.clear();
return true;
}

that explicitly frees up this memory (to my knowledge a std::vector .clear() does not explicitly delete the contents) would circumvent the need for this particular change

std::vector<UCTRegion*> inputRegions;
inputRegions.clear();
edm::Handle<std::vector<L1CaloRegion>> regionCollection;
Expand Down Expand Up @@ -257,6 +253,7 @@ void L1TCaloSummary::produce(edm::Event& iEvent, const edm::EventSetup& iSetup)
}

iEvent.put(std::move(bJetCands), "Boosted");
delete summaryCard;
}

void L1TCaloSummary::print() {}
Expand Down