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

AnaToolHandle in ElectronEfficiencyCorrector and MuonEfficiencyCorrector? #1330

Open
lschaefer opened this issue Mar 4, 2019 · 6 comments
Open

Comments

@lschaefer
Copy link
Contributor

There are a lot of reports in valgrind from the ElectronEfficiencyCorrector and MuonEfficiencyCorrector algorithms, because of the new CP tools that aren't deleted anywhere. It was recommended that we switch to asg::AnaToolHandle but this didn't work in the past (a0ae289) I'm wondering if the issues back then are still relevant, or should we try again? Looking for inspiration in SUSYTools, it does look like some extra work is needed for the electron efficiencies (https://gitlab.cern.ch/atlas/athena/blob/master/PhysicsAnalysis/SUSYPhys/SUSYTools/Root/SUSYToolsInit.cxx#L667).

(Adding deletes in the finalize() method for all the tools doesn't remove all the valgrind reports.)

Best,
Leigh

@kratsg
Copy link
Contributor

kratsg commented Mar 4, 2019

There are a lot of reports in valgrind from the ElectronEfficiencyCorrector and MuonEfficiencyCorrector algorithms, because of the new CP tools that aren't deleted anywhere. It was recommended that we switch to asg::AnaToolHandle but this didn't work in the past (a0ae289) I'm wondering if the issues back then are still relevant, or should we try again? Looking for inspiration in SUSYTools, it does look like some extra work is needed for the electron efficiencies (https://gitlab.cern.ch/atlas/athena/blob/master/PhysicsAnalysis/SUSYPhys/SUSYTools/Root/SUSYToolsInit.cxx#L667).

This was reverted in the past since the AnaToolHandles were not mature enough. They are now. But they won't fix memory leaks -- they simply remove the need for extra deletes by converting to smart pointers that delete themselves. The tools themselves will have leaks and we can't fix that.

@lschaefer
Copy link
Contributor Author

Hi Giordon,

Thanks for the response. I thought the leaks were due to xAH's handling of these tools because of feedback I got from the CP groups -- see for example https://its.cern.ch/jira/browse/ATLASMCP-83 -- but you're saying that's not the case. I don't have enough experience reading valgrind logs to argue in either direction myself.

@kratsg
Copy link
Contributor

kratsg commented Mar 4, 2019

Again, in cases when there's a new (and no delete) -- it's a memory leak because it's not going to get deleted. In cases when there's a new and a delete -- there's no memory leak from xAH. So if you find parts of the code that have a new, but no delete -- they'll need to get fixed.

@lschaefer
Copy link
Contributor Author

Hi Giordon -- ok I see what you're saying now, of course. I'll add deletes as quick fixes. I don't think I have the expertise to migrate to AnaToolHandle myself, but it's not so urgent to do this if the memory leaks (which are very small anyway) aren't mitigated by switching to AnaToolHandle.

@ntadej
Copy link
Contributor

ntadej commented Mar 5, 2019

@lschaefer, any "leaks" in initialize or finalize do not really matter as this will not increase per event. Only leaks in execute do, so if you have problems running on the grid, you should focus on those.

@lschaefer
Copy link
Contributor Author

Hi Tadej, you're right, but these particular leaks are easy enough to fix. Thanks for the advice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants