Home » PANDA » PandaRoot » Bugs, Fixes, Releases » [FIXED] FairWriteoutBuffer::FillNewData and object ownership (memory leak)
| [FIXED] FairWriteoutBuffer::FillNewData and object ownership (memory leak) [message #14374] |
Fri, 18 January 2013 23:03  |
Oliver Merle
Messages: 13
Registered: December 2010
|
occasional visitor |
From: *vpn.uni-giessen.de
|
|
I am a bit confused about the question whether object ownership of the FairTimeStamp instance passed to FairWriteoutBuffer::FillNewData is transferred to the buffer or not.
I had a look at the implementation of the buffers in the SDS package and found that the digis therein are allocated on the heap but never freed. Therefore I assumed that the ownership has been transferred to the buffer and searched for the corresponding deallocation in the implementation of FairWriteoutBuffer+derived. Actually there is none. Finally, I've ran the TimeBasedSimulation macro in the macros/mvd folder and counted the FairTimeStamp instances at runtime - the number was increasing continuously, as expected.
But if the buffer is not freeing the digits, how can the user know if a digit can be safely deallocated? Wouldn't it make more sense to delete the digits after they have been written to the TClonesArray?
[Updated on: Thu, 31 January 2013 15:59] by Moderator Report message to a moderator |
|
|
|
| Re: FairWriteoutBuffer::FillNewData and object ownership (memory leak) [message #14399 is a reply to message #14374] |
Sat, 26 January 2013 14:31  |
Oliver Merle
Messages: 13
Registered: December 2010
|
occasional visitor |
From: *vpn.uni-giessen.de
|
|
Another point I do not quite understand is why what you call "active time" is fixed to -1 in the call to Modify (at FairWriteoutBuffer.cxx:186)
std::vector<std::pair<double, FairTimeStamp*> > modifiedData = Modify(std::pair<double, FairTimeStamp*>(currentdeadtime, oldData), std::pair<double, FairTimeStamp*>(-1, data));
In case I do not combine the signals given by both arguments but just use a dead time (aka active time) to emulate occupancy, I need the active time of the new hit in Modify - but it is replaced by -1.
Seems fishy to me, but maybe I just don't understand the deeper meaning of this mysterious number. Looking at the code of the SDS package doesn't clarify things either:
PndSdsDigiStripWriteoutBuffer.cxx:32
std::vector<std::pair<double, PndSdsDigiStrip*> > PndSdsDigiStripWriteoutBuffer::Modify(std::pair<double, PndSdsDigiStrip*> oldData, std::pair<double, PndSdsDigiStrip*> newData)
{
std::vector<std::pair<double, PndSdsDigiStrip*> > result;
std::pair<double, PndSdsDigiStrip*> singleResult;
if (newData.first > 0)
singleResult.first = oldData.first + newData.first;
singleResult.second = oldData.second;
The if statement makes no sense because newData.first is always -1 as shown above (I don't find any other call to Modify either). The same if statement can be found in PndSdsDigiPixelWriteoutBuffer.cxx:31 and in the Gem package.
Seems to be inconsistent - what do I miss here?
|
|
|
|
| Re: FairWriteoutBuffer::FillNewData and object ownership (memory leak) [message #14400 is a reply to message #14374] |
Sat, 26 January 2013 16:33 |
Mohammad Al-Turany
Messages: 518
Registered: April 2004
Location: GSI, Germany
|
first-grade participant |
From: *dip.t-dialin.net
|
|
Hi,
Sorry for the delay in answering this mail, in any case objects added to TClonesArray are added via new with placement, so you do not call the new and delete in the traditional way (That is why it is much faster than STL or TObjectArray). If the objects added to the TClonesArray do not allocate memory, it is enough to call the clear of the array to free the memory, other wise (e.g: your object has a TString or any other object inside it) then you need to call delete.
Normally each Task call the delete (Clear) of the TClonesArray in the finish event method.
Now for the Time based simulation it is more complicated because we use the TClonesArray::AbsorbObjects method which simply move objects from one array to the other, and it really move and not copy them. Now after moving and writing the objects a method FairRootManager::DeleteOldWriteoutBufferData is called to free the memory in the Buffer. In the Task the memory is freed by the Finishevent. in Other words, we have three TClonesArray:
1. The one used internally to read objects from tree
2. The one in the Buffer
3. The one in the task, which is connected to the output tree
Objects are moved from one array to the other by calling TClonesArray::AbsorbObjects, we clean the buffer from the rest and the Task has to clean what it write to the output tree.
regards,
Mohammad
|
|
|
|
| Re: FairWriteoutBuffer::FillNewData and object ownership (memory leak) [message #14401 is a reply to message #14400] |
Sat, 26 January 2013 17:39 |
Oliver Merle
Messages: 13
Registered: December 2010
|
occasional visitor |
From: *vpn.uni-giessen.de
|
|
Thank you for this in-depth answer! But I think that the problem is not related to memory-pooling - it happens outside of the pool (TClonesArray):
The 'digits' are usually created on the heap and not in a pool. They are passed as pointers to FairWriteoutBuffer, which stores the pointers in maps. When the data has to be written out, the relating portion of digits is *copied* to a pool via AddNewDataToTClonesArray:
void PndSdsDigiStripWriteoutBuffer::AddNewDataToTClonesArray(FairTimeStamp* data)
{
FairRootManager* ioman = FairRootManager::Instance();
TClonesArray* myArray = ioman->GetTClonesArray(fBranchName);
if (fVerbose > 1) std::cout << "Data Inserted: " << *(PndSdsDigiStrip*)(data) << std::endl;
new ((*myArray)[myArray->GetEntries()]) PndSdsDigiStrip(*(PndSdsDigiStrip*)(data));
}
At least I read this as the invocation of a copy-constructor. So the original object on the heap - which was initially passed to the buffer - is still alive and will at some point go out of scope -> memory leak.
For the creation of the objects, see for example PndSdsHybridHitProducer.cxx:370:
PndSdsDigiPixel *tempPixel = new PndSdsDigiPixel( fPixelList[iPix].GetMCIndex(), fInBranchId, fPixelList[iPix].GetSensorID() ,fPixelList[iPix].GetFE(),
fPixelList[iPix].GetCol(), fPixelList[iPix].GetRow(),
smearedCharge, correctedTimeStamp); //fChargeConverter->GetTimeStamp(point->GetTime(), charge,fEventHeader->GetEventTime()) );
if (fTimeOrderedDigi){
tempPixel->ResetLinks();
std::vector<int> indices = fPixelList[iPix].GetMCIndex();
FairEventHeader* evtHeader = (FairEventHeader*)FairRootManager::Instance()->GetObject("EventHeader.");
for (int i = 0; i < indices.size(); i++){
tempPixel->AddLink(FairLink(evtHeader->GetInputFileId(), evtHeader->GetMCEntryNumber(), fInBranchId, indices[i]));
}
tempPixel->AddLink(FairLink(-1, fEventNr, "EventHeader.", -1));
}
fDataBuffer->FillNewData(tempPixel, fChargeConverter->ChargeToDigiValue(fPixelList[iPix].GetCharge())*6 + EventTime, point->GetTime()+EventTime);
I'd say FairWriteoutBuffer should either delete the digits on the heap after copying them to the TClonesArray (transfer of ownership by policy) or use one of the popular shared_ptr implementations.
As I said, the number of allocated FairTimestamp derived instances was increasing steadily during a digitization run while they were correctly freed in the simulation. This can be checked.
Kind regards,
Oliver
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
| Re: FairWriteoutBuffer::FillNewData and object ownership (memory leak) [message #14459 is a reply to message #14418] |
Mon, 25 February 2013 16:46 |
Philipp Mahlberg
Messages: 2
Registered: July 2012
|
occasional visitor |
From: *cb.uni-bonn.de
|
|
Two additional notes concerning the copy creation and deletion of the FairTimeStamp objects in the timebased simulation.
1. From my point of view, the default FairTimeStamp::Modify function still causes a memory leak, as the newly incoming and further on ignored object is not deleted.
2. I am working on a kind of extended timebased simulation for the emc, where I pass interim objects (keeping track of all the needed information) to the event mixing buffers and construct a corresponding Emc waveform right before filling the data to the TClonesArray.
Therefore the interim object contains a pointer to the waveform generator, which will take care of the waveform building process. Since the TObject::Clone() copies the object via streaming, ROOT is constructing new generator objects etc. as well. A simple copy of the address would be sufficient, but the method here creates lots of (useless) objects and brings up a runtime error (ROOT complains about a missing default constructor of an abstract class involved). The latter should be fixed, but I think it is not the main problem though)
As mentioned before, the objects represent an interim state and are not primarily designed to be written into a file, what is done with the final waveforms.
At the moment, I refrain from getting an extra copy of the object by overriding the FillNewData Method.
One could also think of writing a customary streamer which then hopefully just copies the reference address stored in that specific pointer (I am not an expert on this at all, so I don't know weather it will work). But then I could never make use of all of ROOT streaming power to write an object with all its dependencies to file, e.g. for debug reasons. Perhaps it is also possible to distinguish between writing to a file and cloning the object in the streaming function?
Maybe somebody even knows a smarter way how to overcome the problem....
|
|
|
|
Goto Forum:
Current Time: Fri Nov 22 08:54:00 CET 2024
Total time taken to generate the page: 0.00811 seconds
|
GSI Forum
Search
Help
Members
Login
Home
