Changeset 9580

Timestamp:

05/05/10 10:03:14 (15 years ago)

Author:

tbretz

Message:

*** empty log message ***

Location:

trunk/MagicSoft/Mars

Files:

• Changelog (modified) (1 diff)
• mbase/MStatusArray.cc (modified) (8 diffs)
• mbase/MStatusArray.h (modified) (3 diffs)
• mbase/MStatusDisplay.cc (modified) (5 diffs)
• showplot.cc (modified) (4 diffs)

trunk/MagicSoft/Mars/Changelog

@@ -38 +38 @@
    * 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
 
 

trunk/MagicSoft/Mars/mbase/MStatusArray.cc

@@ -58 +58 @@
 #include "MStatusDisplay.h"
 
+//#define DEBUG
+
 ClassImp(MStatusArray);
 
@@ -73 +75 @@
 }
 
+#ifdef DEBUG
+static void *ptr = 0;
+#endif
+
 // --------------------------------------------------------------------------
 //
@@ -78 +84 @@
 // 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<TVirtualPad*>(o) && (o->ClassName()[0]==id || id==0))
@@ -95 +103 @@
             // get removed from the pad if deleted, and therefore
             // gets deleted twice (FIXME: To be investigated)
-            if (dynamic_cast<TPaveText*>(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<TGraph*>(o);
-            if (g)
-                g->SetHistogram(0);
-             */
-
+            //if (dynamic_cast<TPaveText*>(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<TVirtualPad*>(o), id);
-    }
-
+#ifdef DEBUG
+            ptr = 0;
+            cout << "Done  Delete " << o<< endl;
+#endif
+            return kFALSE;
+        }
+
+        while (!RecursiveDelete(dynamic_cast<TVirtualPad*>(o), id));
+    }
+
+    return kTRUE;
 }
 
@@ -136 +142 @@
         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<TGraph*>(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 +174 @@
 }
 
-// --------------------------------------------------------------------------
-//
-// 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<TGraph*>(o))
-        {
-            // FIXME: This should not be called for the TGraph
-            // which is currently getting deleted.
-            TGraph *g = static_cast<TGraph*>(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<TPad*>(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<TGraph*>(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<TVirtualPad*>(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<MyGraph*>(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<TVirtualPad*>(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 +299 @@
     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 +310 @@
     TIter Next2(this);
     while ((o=Next2()))
-        RecursiveDelete(dynamic_cast<TVirtualPad*>(o), 'M');
+        while (!RecursiveDelete(dynamic_cast<TVirtualPad*>(o), 'M'));
 
     // Now delete all root objects
     TIter Next3(this);
     while ((o=Next3()))
-        RecursiveDelete(dynamic_cast<TVirtualPad*>(o));
+        while (!RecursiveDelete(dynamic_cast<TVirtualPad*>(o)));
 
     // And delete all the rest

trunk/MagicSoft/Mars/mbase/MStatusArray.h

@@ -20 +20 @@
 
     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 +27 @@
     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 +63 @@
     void Delete(Option_t *option="");
 
-    void RecursiveRemove(TVirtualPad *p, TObject *o);
     void RecursiveRemove(TObject *o);
 

trunk/MagicSoft/Mars/mbase/MStatusDisplay.cc

@@ -790 +790 @@
     MapWindow();
 
+    UpdateMemory();
+
     // FIXME: This is a workaround, because TApplication::Run is not
     //        thread safe against ProcessEvents. We assume, that if
@@ -1010 +1012 @@
     MapSubwindows();
     Layout();
+
+    UpdateMemory();
 
     // display new tab in the main frame
@@ -1164 +1168 @@
     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 +1448 @@
     MapSubwindows();   // maps the TGCompositeFrame
     Layout();          // layout the embedded canvas in the frame
+
+    UpdateMemory();
 
     // display new tab in the main frame
@@ -2253 +2261 @@
         // 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

trunk/MagicSoft/Mars/showplot.cc

@@ -41 +41 @@
     gLog << " Options: "<< endl;
     gLog.Usage();
-    gLog << "   -q                        Quit when job is finished" << endl;
     gLog << endl;
     gLog << " General Output Options: "<< endl;
@@ -92 +91 @@
     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 +163 @@
     }
 
-    const Bool_t kQuit       = arg.HasOnlyAndRemove("-q");
     const Bool_t kBatch      = arg.HasOnlyAndRemove("-b");
     const Bool_t kRoot       = arg.HasOnlyAndRemove("-r");
@@ -308 +306 @@
     gLog << endl;
 
-    if (kBatch || kQuit || (arg.GetNumArguments()>0 && d->GetNumTabs()==0))
+    if (kBatch || (arg.GetNumArguments()>0 && d->GetNumTabs()==0))
     {
         delete d;