Index: trunk/MagicSoft/Mars/Changelog =================================================================== --- trunk/MagicSoft/Mars/Changelog (revision 9579) +++ trunk/MagicSoft/Mars/Changelog (revision 9580) @@ -38,4 +38,16 @@ * msimcamera/MSimCalibrationSignal.cc: - changed distribution of signal from Gauss to Poissonian + + * showplot.cc: + - removed nonsense "-q" option + + * mbase/MStatusArray.[h,cc]: + - fixed/improved (once again) deleting of the array + + * mbase/MStatusDisplay.cc: + - removed the MAstroCatalog workaround - is it still necessary? + (needs testing) + - added UpdateMemory every here and there to get the memory + consumption is more circumstances Index: trunk/MagicSoft/Mars/mbase/MStatusArray.cc =================================================================== --- trunk/MagicSoft/Mars/mbase/MStatusArray.cc (revision 9579) +++ trunk/MagicSoft/Mars/mbase/MStatusArray.cc (revision 9580) @@ -58,4 +58,6 @@ #include "MStatusDisplay.h" +//#define DEBUG + ClassImp(MStatusArray); @@ -73,4 +75,8 @@ } +#ifdef DEBUG +static void *ptr = 0; +#endif + // -------------------------------------------------------------------------- // @@ -78,12 +84,14 @@ // name) recuresively // -void MStatusArray::RecursiveDelete(TVirtualPad *p, const char id) const +Bool_t MStatusArray::RecursiveDelete(TVirtualPad *p, const char id) const { if (!p) - return; - - TIter Next2(p->GetListOfPrimitives()); - TObject *o=0; - while ((o=Next2())) + return kTRUE; + + // It seems that it is necessary to keep the ListOfPrimitve consistent + // while deleting - reason unknown. + TIter Next(p->GetListOfPrimitives()); + TObject *o=0; + while ((o=Next())) { if (!dynamic_cast(o) && (o->ClassName()[0]==id || id==0)) @@ -95,26 +103,24 @@ // get removed from the pad if deleted, and therefore // gets deleted twice (FIXME: To be investigated) - if (dynamic_cast(o) && !o->TestBit(kCanDelete)) - continue; - - /* - // This is necessary because the histogram can get deleted - // but is not treated in TGraph recursive remove. Unfortunately, - // we have no possibility to find out whether it was already - // deleted. Fortunately these histograms don't contain data - // and threfore don't consume memory. - TGraph *g = dynamic_cast(o); - if (g) - g->SetHistogram(0); - */ - + //if (dynamic_cast(o) && !o->TestBit(kCanDelete)) + // continue; + +#ifdef DEBUG + cout << "Start Delete " << o << " " << o->GetName() << " " << o->ClassName() << endl; + ptr = o; +#endif delete o; - continue; - } - - RecursiveDelete(dynamic_cast(o), id); - } - +#ifdef DEBUG + ptr = 0; + cout << "Done Delete " << o<< endl; +#endif + return kFALSE; + } + + while (!RecursiveDelete(dynamic_cast(o), id)); + } + + return kTRUE; } @@ -136,9 +142,30 @@ obj->ResetBit(kMustCleanup); else + { + // If this is no pad or TCanvas set the kMustCleanup bit + // to make sure that the object is not by chance deleted twice. obj->SetBit(kMustCleanup); + + // Unfortunately, this can still happen to the fHistogram and + // fFunctions in a TGraph (because for some reason if a TGraph + // is stored twice in a file they might later share the same + // objects) + TGraph *g = dynamic_cast(obj); + if (g) + { +#ifdef DEBUG + cout << "Found TGraph " << g->GetName() << " " << g << " while deleting " << ptr << endl; +#endif + if (g->GetHistogram()) + g->GetHistogram()->SetBit(kMustCleanup); + if (g->GetListOfFunctions()) + g->GetListOfFunctions()->SetBit(kMustCleanup); + } + } if (!pad) return; + // If object was a pad go one layer deeper TIter Next(pad->GetListOfPrimitives()); TObject *o=0; @@ -147,61 +174,122 @@ } -// -------------------------------------------------------------------------- -// -// This is a workaround to make sure that TH1 which are part of two TGraphs -// (TGraph::GetHistogram) are not finally deleted twice. Unfortunately, -// TGraph doesn't have it in RecursiveRemove. -// -void MStatusArray::RecursiveRemove(TVirtualPad *p, TObject *obj) -{ - if (!p) - return; - - TIter Next(p->GetListOfPrimitives()); - TObject *o=0; - while ((o=Next())) - { - if (dynamic_cast(o)) - { - // FIXME: This should not be called for the TGraph - // which is currently getting deleted. - TGraph *g = static_cast(o); +// This is a stupid C++ trick which allos us to access the +// protected data-mebers of TGraph +class MyGraph : public TGraph +{ +public: + void ResetListOfFunctions() + { + fFunctions = 0; + } +}; + +void MStatusArray::RecursiveRemove(TCollection *col, TObject *obj) +{ + // First remove the object from the collection as often as possible + // (The collection is always a ListOfPrimitives(). This code is + // based on TCollection::RecursiveRemove and TPad::RecursiveRemove) + while (col->Remove(obj)); + + TObject *object=0; + + // Loop over all objects in the collection + TIter Next(col); + while ((object=Next())) + { + // Check if this object is a pad. If it is a pad go one + // layer deeper. + TPad *pad = dynamic_cast(object); + if (pad) + { +#ifdef DEBUG + cout << "Start RecursiveRemove in " << pad->GetName() << " " << pad << endl; +#endif + RecursiveRemove(pad->GetListOfPrimitives(), obj); +#ifdef DEBUG + cout << "Done RecursiveRemove in " << pad->GetName() << " " << pad << endl; +#endif + continue; + } + + // If it is not a TPad check if it is a TGraph + TGraph *g = dynamic_cast(object); + if (g/* && g!=ptr*/) + { +#ifdef DEBUG + cout << "MStatusArray::RecursiveRemove " << obj << " from TGraph " << g << endl; +#endif + + // If the object to be removed is the histogram from the + // TGraph the histogram got already deleted so we have to + // reset the pointer if (g->GetHistogram()==obj) + { +#ifdef DEBUG + cout << " SetHist(0)" << endl; +#endif g->SetHistogram(0); - else - g->GetHistogram()->SetBit(kMustCleanup); - - continue; - } - - RecursiveRemove(dynamic_cast(o), obj); - } - -} - -// -------------------------------------------------------------------------- -// -// This is a workaround to make sure that TH1 which are part of two TGraphs -// (TGraph::GetHistogram) are not finally deleted twice. Unfortunately, -// TGraph doesn't have it in RecursiveRemove. + continue; + } + + // If the object to be removed is the ListOfFunction from the + // TGraph the ListOfFunction got already deleted so we have to + // reset the pointer. This is done by a stupid C++ trick + // (a derived class which can access the protected data mebers) + if (g->GetListOfFunctions()==obj) + { +#ifdef DEBUG + cout << " SetFunc(0)" << endl; +#endif + static_cast(g)->ResetListOfFunctions(); + continue; + } + } + +#ifdef DEBUG + cout << " Call RecursiveRemove for " << object->GetName() << " " << object << endl; +#endif + + // Now we go on like TCollction would do calling the RecursiveRemove + // of the objects (which are no pads) + if (object->TestBit(TObject::kNotDeleted)) + object->RecursiveRemove(obj); + } +} + +// -------------------------------------------------------------------------- +// +// Use our own implementation of RecursiveRemove. // void MStatusArray::RecursiveRemove(TObject *obj) { - TObjArray::RecursiveRemove(obj); - - // FIXME: Maybe we still have to call SetBit(kMustCleanup) ??? -#if ROOT_VERSION_CODE <= ROOT_VERSION(5,26,00) - TObject *o = 0; - - TIter Next(this); - while ((o=Next())) - RecursiveRemove(dynamic_cast(o), obj); -#endif -} - -// -------------------------------------------------------------------------- -// -// Try to do a delete of the whole list in a way which is less vulnarable -// to double deletion due to wrongly set bits or other things +#ifdef DEBUG + cout << "RecursiveRemove " << obj << endl; +#endif + RecursiveRemove(this, obj); +} + +// -------------------------------------------------------------------------- +// +// The problem is that our objects (e.g. MHRate) can own histograms and +// graphs (or other objects), which are data members rather than allocated +// objects (pointers), and which are drawn into a pad. If such a pad gets +// read from a file we have to set the kCanDelete bits for all objects +// in all pads to ensure that they are properly deleted (since we +// don't know who owns them). Unfortunately, this might result in the +// data member deleted first and then second in the destructor of the +// owning class. One simple workaround is to delete all our own objects +// ("M*") first without touching the root objects ("T*") and delete the +// remaining ones later. This might not work if teh owned objects are +// MARS classes. Fortunately, none of our classes yets owns another of +// our class not dynamically allocated. (This case is covered by +// the RecursiveRemove facility). +// +// Furthermore TGraphs which are read from a file as a data member +// of a class and which are drawn to a pad share the same fFunctions +// and fHistogram. To avoid double deleting (the second TGraph won't +// be signaled if the first one is deleted) we implement our own +// RecursiveRemove facility. (This gets a bit better in root 5.28 +// because TGraph::RecursiveRemove will than also check fHistogram) // void MStatusArray::Delete(Option_t *) @@ -211,5 +299,6 @@ gROOT->GetListOfCleanups()->Add(this); - // First make sure that all kMustCleanup bits are se + // First make sure that all kMustCleanup bits are set, esepcially + // the ones of TGraph::fHistogram and TGraph::fFunctions TIter Next(this); TObject *o=0; @@ -221,10 +310,10 @@ TIter Next2(this); while ((o=Next2())) - RecursiveDelete(dynamic_cast(o), 'M'); + while (!RecursiveDelete(dynamic_cast(o), 'M')); // Now delete all root objects TIter Next3(this); while ((o=Next3())) - RecursiveDelete(dynamic_cast(o)); + while (!RecursiveDelete(dynamic_cast(o))); // And delete all the rest Index: trunk/MagicSoft/Mars/mbase/MStatusArray.h =================================================================== --- trunk/MagicSoft/Mars/mbase/MStatusArray.h (revision 9579) +++ trunk/MagicSoft/Mars/mbase/MStatusArray.h (revision 9580) @@ -20,5 +20,5 @@ void SetCleanup(TObject *obj) const; - void RecursiveDelete(TVirtualPad *p, const char id=0) const; + Bool_t RecursiveDelete(TVirtualPad *p, const char id=0) const; void SetCanDelete(const TCollection *list) const; @@ -27,4 +27,5 @@ void PrintObjectsInPad(const TCollection *list, const TString &name, Int_t lvl=0) const; TObject *FindObjectInPad(TVirtualPad *pad, const char *object, TClass *base) const; + void RecursiveRemove(TCollection *c, TObject *o); public: @@ -62,5 +63,4 @@ void Delete(Option_t *option=""); - void RecursiveRemove(TVirtualPad *p, TObject *o); void RecursiveRemove(TObject *o); Index: trunk/MagicSoft/Mars/mbase/MStatusDisplay.cc =================================================================== --- trunk/MagicSoft/Mars/mbase/MStatusDisplay.cc (revision 9579) +++ trunk/MagicSoft/Mars/mbase/MStatusDisplay.cc (revision 9580) @@ -790,4 +790,6 @@ MapWindow(); + UpdateMemory(); + // FIXME: This is a workaround, because TApplication::Run is not // thread safe against ProcessEvents. We assume, that if @@ -1010,4 +1012,6 @@ MapSubwindows(); Layout(); + + UpdateMemory(); // display new tab in the main frame @@ -1164,4 +1168,6 @@ Layout(); // layout the embedded canvas in the frame + UpdateMemory(); + // display new tab in the main frame // FIXME: This is a workaround, because TApplication::Run is not @@ -1442,4 +1448,6 @@ MapSubwindows(); // maps the TGCompositeFrame Layout(); // layout the embedded canvas in the frame + + UpdateMemory(); // display new tab in the main frame @@ -2253,6 +2261,6 @@ // FIXME: This is a workaround for the problem with the MAstroCatalog in // MHFalseSource. It doesn't harm. We'll still try to find the reason - if (clone->IsA()==TPad::Class()) - gROOT->GetListOfCleanups()->Add(clone); + //if (clone->IsA()==TPad::Class()) + // gROOT->GetListOfCleanups()->Add(clone); // Add the clone and its draw-option to the current pad Index: trunk/MagicSoft/Mars/showplot.cc =================================================================== --- trunk/MagicSoft/Mars/showplot.cc (revision 9579) +++ trunk/MagicSoft/Mars/showplot.cc (revision 9580) @@ -41,5 +41,4 @@ gLog << " Options: "<< endl; gLog.Usage(); - gLog << " -q Quit when job is finished" << endl; gLog << endl; gLog << " General Output Options: "<< endl; @@ -92,5 +91,5 @@ gLog << " showplot -b --print --print-cmd='cat %f' filename.root > filename.ps" << endl; gLog << " showplot -b --save-as-ps filename.root" << endl; - gLog << " showplot -q --save-as-gif=tab5.gif --tab=5 filename.root" << endl; + gLog << " showplot -b --save-as-gif=tab5.gif --tab=5 filename.root" << endl; gLog << " showplot -b --save-as-ps --print=lp2 filename.root" << endl; gLog << endl; @@ -164,5 +163,4 @@ } - const Bool_t kQuit = arg.HasOnlyAndRemove("-q"); const Bool_t kBatch = arg.HasOnlyAndRemove("-b"); const Bool_t kRoot = arg.HasOnlyAndRemove("-r"); @@ -308,5 +306,5 @@ gLog << endl; - if (kBatch || kQuit || (arg.GetNumArguments()>0 && d->GetNumTabs()==0)) + if (kBatch || (arg.GetNumArguments()>0 && d->GetNumTabs()==0)) { delete d;