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

memory leaks #36

Open
c-dilks opened this issue Jun 6, 2023 · 1 comment
Open

memory leaks #36

c-dilks opened this issue Jun 6, 2023 · 1 comment

Comments

@c-dilks
Copy link
Member

c-dilks commented Jun 6, 2023

Environment: (where does this bug occur, have you tried other environments)

  • Which branch (often main for latest released): main

Steps to reproduce: (give a step by step account of how to trigger the bug)

  1. Run eicrecon with feat(dRICH): connect irt to EICrecon algorithms EICrecon#707
    (see valgrind test)

Expected Result: (what do you expect when you execute the steps above)

No leaks.

Actual Result: (what do you get when you execute the steps above)

Leaks.

More notes

  • new raw pointers with no delete is a primary source of leaks
  • this may be difficult to resolve, since we need to maintain ROOT streaming of irt class objects (ROOT may do the deleting upon write)
  • ROOT streaming also prevents us from using smart pointers at this time
  • we should address this after v2.0 (merging of pfrich branch from feat: update pfrich branch with respect to main #35)
veprbl pushed a commit to eic/EICrecon that referenced this issue Jun 29, 2023
Initial connection of [`irt`](https://github.com/eic/irt) to `EICrecon`
algorithms, via new algorithm `IrtCherenkovParticleID` and corresponding
factory `IrtCherenkovParticleID_factory`.

#### Notes
- not production ready, therefore not enabled by default in
`JEventProcessorPODIO`, but we have been using this for routine current
dRICH performance studies
- difficult to unit test `IrtCherenkovParticleID`, because of the
complexity of its inputs; prefer to defer testing to [DRICH
`reconstruction_benchmarks`](https://eicweb.phy.anl.gov/EIC/benchmarks/reconstruction_benchmarks/-/merge_requests/293)
- `valgrind` leak checks: #709 (see below for comments)
- documentation: #708 

#### Caveats
- there are several known issues, which should be resolved in separate
PRs
  - #689
  - #690
  - #691
  - #692
  - #693
  - #705
- `ROOT` dependence is needed, since `irt` heavily relies on objects
such as `TVector3`
- converters from `EDM4{hep,eic}` vectors to `ROOT` vectors are included
in common header `Tools.h` (other downstream PID algorithms will use
this header too)
- where possible, smart pointers are used in `IrtCherenkovParticleID`,
but sometimes we had to unfortunately fall back to raw pointers, for
compatibility with the `irt` repository, since:
  - `irt` does not use any smart pointers
- `irt` classes typically include custom destructors; using smart
pointers here in `EICrecon` runs the risk of 'double free' violations
- any usage of the (banned) operator `new` includes a comment indicating
where the allocated object will be destroyed (most likely within an
`irt` class destructor)
- `irt` objects can be streamed to ROOT files, and closing those ROOT
files may `delete` the corresponding object(s); using smart pointers in
this scenario can also cause 'double free' violations
- we can try to fix this issue in separate PRs, but given the general
fragility of streaming objects to ROOT files, I'm afraid this will not
be straightforward
- not surprisingly, #709 identified a few leaks in the `irt` repository
itself
- [deferring fixing them until `irt`
v2.0](eic/irt#36), where we expect a [large
refactor](eic/irt#34)
- fixed a few leaks from local geometry service `richgeo`; again
unfortunately `irt` does not cooperate with smart pointers
@wdconinc
Copy link
Contributor

Here's one that can be fixed.

class CherenkovPID {
 public:
 CherenkovPID( void ) {};
  ~CherenkovPID() {};

  void AddMassHypothesis(double mass) { m_Hypotheses.push_back(new MassHypothesis(mass)); };

github-merge-queue bot pushed a commit to eic/EICrecon that referenced this issue Sep 27, 2023
)

### Briefly, what does this PR introduce?
Until now we had enough leaks that enforcing absence of memory leaks was
not feasible. We have now fixes for the remaining memory leaks inside
EICrecon:
- [x] #974 
- [x] #977 
  - [x] #981
  - [x] #982
  - [x] #983
- [x] #979 

The remaining memory leaks are external to EICrecon (though some are not
exactly completely outside of our control either, e.g.
eic/irt#36), so we can suppress them and
require any PRs to come back clean.

### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [x] New feature (issue: require clean leak sanitizer runs)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No. (If this breaks your code, then from now on your code is the thing
that's broken.)

### Does this PR change default behavior?
No.
github-merge-queue bot pushed a commit to eic/EICrecon that referenced this issue Sep 28, 2023
)

### Briefly, what does this PR introduce?
Until now we had enough leaks that enforcing absence of memory leaks was
not feasible. We have now fixes for the remaining memory leaks inside
EICrecon:
- [x] #974 
- [x] #977 
  - [x] #981
  - [x] #982
  - [x] #983
- [x] #979 

The remaining memory leaks are external to EICrecon (though some are not
exactly completely outside of our control either, e.g.
eic/irt#36), so we can suppress them and
require any PRs to come back clean.

### What kind of change does this PR introduce?
- [ ] Bug fix (issue #__)
- [x] New feature (issue: require clean leak sanitizer runs)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No. (If this breaks your code, then from now on your code is the thing
that's broken.)

### Does this PR change default behavior?
No.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

2 participants