Fix L1TCaloSummary memory leak in UCTSummaryCard related to UCTRegion… #40735
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40735/34139
|
|
A new Pull Request was created by @aloeliger (Andrew Loeliger) for master. It involves the following packages:
@epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| @@ -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); | |||
There was a problem hiding this comment.
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...);
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-40735/34143
|
|
Pull request #40735 was updated. @epalencia, @cmsbuild, @cecilecaillol, @rekovic can you please check and sign again. |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-3b6ee1/30791/summary.html Comparison SummarySummary:
|
|
+l1 |
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
L1TCaloSummary has a memory leak in it's created UCTSummaryCard. It would attempt to clear the associated UCTRegions on every event, which calls a
std::vector.clear(), which would clear the vector contents, but not free the memory associated with the added UCTRegions. Peak memory usage on a single RAW file could be double post-fix memory usage due to abandoned regions.Because the UCTSummaryCard properly cleans up the UCTRegions when doing it's destructor tasks (and expects to do the deletion, preventing deleting the regions by hand), I have made the summary card allocated per event, and then deleted at the end of the event.
PR validation:
Due to this emulator not being associated with any standard workflows yet, no changes are expected to standard workflows, and they are not useful in validation. All code compiles, and has had code formatting applied. I have verified that this module is capable of running on RAW files without segfaulting or otherwise breaking.
If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
This PR is not a backport.