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

Workaround a memory leak with old ROOT IO #417

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

karuboniru
Copy link
Member

This is related to a long existing bug in ROOT, with related workaround in GENIE traced back to this commit from Apr 2006. But such workaround by manually calling the Clean method after each entry of event being processed is not possible with working with the latest ROOT's RDataFrame analysis framework like I did in the reweighting testing code.

The bug is that a class is being read from a ROOT file and:

  • the class contains a pointer to another object
  • the root dict for the class is generated without a "+" in #pragma link C++ class ClassName+;.

Since the bug is related to the old IO implementation in ROOT they are not going to fix it. But there are 2 approaches to fix or workaround this:

  1. move to new IO implementation, always specify "+" in LinkDef.h
  2. Use user defined Streamer to free the memory before the pointer is overwritten by TBuffer.

The first one seemd to be feasible, but the fatal issue is that the old and new IO don't compatible. The root file generated with new IO method won't be recognized by old dictionaty and vice versa. Developers from ROOT did provide an example to show how to partially workaround the incompatibility with the version number tricks to keep old files readable, but it would still break the compatibility from the other side.

So this PR is the conservative approach, the second one, the IO and file format is kept the same. This should not be breaking anything, the only difficulty it would introduce is when we change the defination of NtpMCEventRecord we would have to update the definition of NtpMCEventRecord::Streamer here accordingly. (But at that time we can really move to the new IO from ROOT since change the definition of a class will introduce incompatibility)

@karuboniru
Copy link
Member Author

refs: #82

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

Successfully merging this pull request may close these issues.

1 participant