Drop requirement for user code to call CBase_foo::pup(p)
authorPhil Miller <mille121@illinois.edu>
Fri, 2 Dec 2011 08:11:21 +0000 (02:11 -0600)
committerPhil Miller <mille121@illinois.edu>
Mon, 26 Dec 2011 08:43:12 +0000 (02:43 -0600)
Requiring users to explicitly call CBase_foo::pup(p) at the beginning
of any chare's pup routine was easy to forget and led to substantial
frustration. Thus, make the system responsible for this, and take the
burden off users. Update tests and documentation to match.

Add an additional virtual method base_pup(PUP::er &p) to take over the
responsibility of code that was previously in the user-called base
class pup() methods. Insert calls to base_pup() in the main
array-migration code paths.

Currently *not* modified are the checkpoint code and migration for
plain chares. The fault-tolerance code paths under various #ifdef
guards are also not changed.

After consideration, I'm fairly confident that this change will not
break existing code that directly inherits from
Chare/Group/NodeGroup/ArrayElementND, so long as its pup routine stays
old-school to match (i.e. calls Chare::pup directly). The
implementation of base_pup is empty in those classes, and the
generated class's base_pup would call the base class pup routines.

Existing code with user-defined inheritance is a tricky case where
this change may break. In the simplest variants, some data might run
through pup twice. More disconcerting is the possibility of double
allocation on unpacking and memory leaks. If there is any actual code
that encounters this problem, I'll be happy to provide the labor
necessary to repair it.

20 files changed:
doc/charm++/arrays.tex
doc/charm++/loadb.tex
doc/charm++/pup.tex
doc/charm++/sdag.tex
src/ck-core/charm++.h
src/ck-core/cklocation.C
tests/charm++/chkpt/hello.C
tests/charm++/commtest/comlib/bench.C
tests/charm++/commtest/comlib/benchmulti.C
tests/charm++/commtest/comlib/benchsectionmulti.C
tests/charm++/commtest/comlib/hello.C
tests/charm++/commtest/eachtomany-test-2/eachtomany.C
tests/charm++/commtest/eachtomany/eachtomany.C
tests/charm++/delegation/multicast/hello.C
tests/charm++/delegation/pipelined-section-reduction/hello.C
tests/charm++/load_balancing/lb_test/lb_test.C
tests/charm++/load_balancing/lb_test/predictor/test.C
tests/charm++/megatest/inherit.C
tests/charm++/megatest/migration.C
tests/charm++/sdag/migration/test1.C

index af1e7c20294804691a45a5f376c20b237d7c6f7d..f240d4d4d13147aaefbe00b90adf4d321b169886 100644 (file)
@@ -681,7 +681,6 @@ public:
 //In the .C file:
 void A2::pup(PUP::er \&p)
 \{
-    CBase\_A2::pup(p); //<- MUST call superclass's pup routine
     p|nt;
     p|chr;
     p(flt,7);
index 01722746586c96c2076b771f39c51f4115400ef1..87d0637e3c544f25db88134665e408fdeede1a23 100644 (file)
@@ -716,7 +716,6 @@ public:
   
   /* Must be written for migration to succeed */
   void pup(PUP::er &p){
-    CBase_LBExample::pup(p);
     p|workcnt;
     /* There may be some more variables used in doWork */
   }
index 38a458899be25e05304041fc8a67ae2c60f5ef05..902e3cd5540cc0c431a91390ae93f1862e37f3d0 100644 (file)
@@ -24,7 +24,6 @@ class foo : public mySuperclass \{
 
     //pack/unpack routine: describe my fields to charm++
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       p|a;
       p|x; p|y; p|z;
       PUParray(p,arr,3);
@@ -50,11 +49,16 @@ We often use ``pup'' as a verb, meaning ``to save/restore the value of''
 or equivalently, ``to call the pup routine of''.
 
 Pup routines for complicated objects normally call the pup routines
-for their simpler parts.  Since all objects depend on their immediate
-superclass, the first line of every pup routine is a call to the 
-superclass's pup routine---the only time you shouldn't call your superclass's
-pup routine is when you don't have a superclass.  If your superclass has
-no pup routine, you must pup the values in the superclass yourself.
+for their simpler parts. If your class's declaration in the ci file
+includes a parent class, the generated code in CBase will call that
+class's pup routine automatically\footnote{In versions of Charm++
+  prior to 6.4, this was not automatic. Older code called
+  \verb|CBase_ClassName::pup(p)| at the beginning of each pup
+  routine. This has been made a no-op so as not to break the API.}. If
+your class has a parent class that is not declared in the ci file, you
+are responsible for packing it appropriately, either by calling its
+pup routine explicitly from your own, or by packing its fields
+individually.
 
 
 \paragraph{PUP operator}
@@ -179,7 +183,6 @@ class bar : public barParent \{
     ...other methods...
 
     virtual void pup(PUP::er& p) \{
-      barParent::pup(p);
       __sdag_pup(p);
       ...pup other data here...
     \}
@@ -279,7 +282,6 @@ private:
 public:
     keepsFoo(void) \{ \}
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       p|f; // pup f's fields (calls f.pup(p);) 
     \}
     ~keepsFoo() \{ \}
@@ -301,7 +303,6 @@ public:
       f=new foo;
     \}
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       p|*f; // pup f's fields (calls f->pup(p))
     \}
     ~keepsHeapFoo() \{delete f;\}
@@ -322,7 +323,6 @@ public:
     keepsOneFoo(...) \{f=new foo(...);\}
     keepsOneFoo() \{f=NULL;\} /* pup constructor */
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       ...
       if (p.isUnpacking()) /* must allocate foo now */
          f=new foo(...);
@@ -353,7 +353,6 @@ public:
     keepsDoubles() \{ \} 
     
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       p|n;//pup the array length n
       if (p.isUnpacking())  arr=new double[n];
       PUParray(p,arr,n); //pup data in the array
@@ -377,7 +376,6 @@ public:
     keepsNullFoo(...) \{ if (...) f=new foo(...);\}
     keepsNullFoo() \{f=NULL;\}
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       int has_f=(f!=NULL);
       p|has_f;
       if (has_f) {
@@ -412,7 +410,6 @@ public:
     keepsFoos() \{ arr=NULL; \} 
     
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       p|n;//pup the array length n
       if (p.isUnpacking())  arr=new foo[n];
       PUParray(p,arr,n); //pup each foo in the array
@@ -444,7 +441,6 @@ public:
     keepsFooPtrs() \{ arr=NULL; \} 
     
     void pup(PUP::er &p) \{
-      mySuperclass::pup(p);
       p|n;//pup the array length n
       if (p.isUnpacking()) arr=new foo*[n]; // allocate array
       for (int i=0;i<n;i++) \{
index 0500b52b6aff500a629dadd5f5c6a990183d9015..697ec30ccba3f6fae6f6ae92f1b42b6b6fc2b703 100644 (file)
@@ -202,7 +202,6 @@ public:
     \}
     
     void pup(PUP::er &p) \{
-       CBase_Foo::pup(p);
        __sdag_pup(p);
     \}
 \};
index da8e1b591cbc63e98e42471297537bbe764a6c95..c5908213ff92b81c545b91d1f40e8eba5fdeeb3e 100644 (file)
@@ -269,6 +269,7 @@ class Chare {
     Chare(CkMigrateMessage *m);
     Chare();
     virtual ~Chare(); //<- needed for *any* child to have a virtual destructor
+    virtual void base_pup(PUP::er &p) { }
     virtual void pup(PUP::er &p);//<- pack/unpack routine
     inline const CkChareID &ckGetChareID(void) const {return thishandle;}
     inline void CkGetChareID(CkChareID *dest) const {*dest=thishandle;}
@@ -340,10 +341,11 @@ public:
 
        CBaseT1(void) :Parent()  { thisProxy=this; }
        CBaseT1(CkMigrateMessage *m) :Parent(m) { thisProxy=this; }
-       void pup(PUP::er &p) {
+        void base_pup(PUP::er &p) {
                Parent::pup(p);
                p|thisProxy;
        }
+        void pup(PUP::er &p) { }
 };
 
 /*Templated version of above for multiple (at least duplicate) inheritance:*/
@@ -356,11 +358,12 @@ public:
                { thisProxy = (Parent1 *)this; }
        CBaseT2(CkMigrateMessage *m) :Parent1(m), Parent2(m)
                { thisProxy = (Parent1 *)this; } 
-       void pup(PUP::er &p) {
+        void base_pup(PUP::er &p) {
                Parent1::pup(p);
                Parent2::pup(p);
                p|thisProxy;
        }
+        void pup(PUP::er &p) { }
 
 //These overloads are needed to prevent ambiguity for multiple inheritance:
        inline const CkChareID &ckGetChareID(void) const
@@ -376,10 +379,11 @@ public:
   BASEN(n)() : base(), PARENTN(n)() {}                               \
   BASEN(n)(CkMigrateMessage *m)                                       \
          : base(m), PARENTN(n)(m) {}                                 \
-  void pup(PUP::er &p) {                                              \
+  void base_pup(PUP::er &p) {                                         \
     base::pup(p);                                                     \
     PARENTN(n)::pup(p);                                               \
   }                                                                   \
+  void pup(PUP::er &p) { }                                            \
   static int isIrreducible() {                                        \
     return (base::isIrreducible() && PARENTN(n)::isIrreducible());    \
   }
index 5917156bd44738b632b4b109119e6bf595fe439a..3b9acd0a7eaaf17b6fe09cfce14247d830449323 100644 (file)
@@ -901,6 +901,7 @@ void CkArrayPrefetch_writeToSwap(FILE *swapfile,void *objptr) {
 
   //Save the element's data to disk:
   PUP::toDisk p(swapfile);
+  elt->base_pup(p);
   elt->pup(p);
 
   //Call the element's destructor in-place (so pointer doesn't change)
@@ -923,6 +924,7 @@ void CkArrayPrefetch_readFromSwap(FILE *swapfile,void *objptr) {
   
   //Restore the element's data from disk:
   PUP::fromDisk p(swapfile);
+  elt->base_pup(p);
   elt->pup(p);
 }
 
@@ -2099,7 +2101,9 @@ CmiBool CkLocMgr::addElementToRec(CkLocRec_local *rec,ManagerRec *m,
 
 #if CMK_OUT_OF_CORE
        /* Register new element with out-of-core */
-       PUP::sizer p_getSize; elt->pup(p_getSize);
+       PUP::sizer p_getSize;
+        elt->base_pup(p_getSize);
+        elt->pup(p_getSize);
        elt->prefetchObjID=CooRegisterObject(&CkArrayElementPrefetcher,p_getSize.size(),elt);
 #endif
        
@@ -2511,9 +2515,10 @@ void CkLocMgr::pupElementsFor(PUP::er &p,CkLocRec_local *rec,
        //Next pup the element data
        for (m=firstManager;m!=NULL;m=m->next) {
                CkMigratable *elt=m->element(localIdx);
-               if (elt!=NULL) 
-                {      
-                       elt->pup(p);
+               if (elt!=NULL)
+                {
+                        elt->base_pup(p);
+                        elt->pup(p);
                 }
        }
 }
index 78ed7a2ebb09562530344d85f44ffbc6d56982b8..a956a34b939c38918442a79ff3b2f0f8e4ccb514 100644 (file)
@@ -63,7 +63,6 @@ public:
   }
 
   void pup(PUP::er &p){
-    CBase_Main::pup(p);
     p|step;
     p|a; p(b,2);
     CkPrintf("Main's PUPer. a=%d(%p), b[0]=%d(%p), b[1]=%d.\n",a,&a,b[0],b,b[1]);
@@ -88,7 +87,6 @@ public:
   }
   
   void pup(PUP::er &p){
-    CBase_Hello::pup(p);
     p|step;
   }
 };
@@ -106,7 +104,6 @@ public:
   }
 
   void pup(PUP::er &p){
-    Chare::pup(p);
     p|step;
     printf("CHello's PUPer. step=%d.\n", step);
   }
index 3134bc30666a3340289584df9dfdad0d67f9ddcb..878b9fe6af0e8ba58051670fe67fb49f1dcd5ba4 100644 (file)
@@ -297,7 +297,6 @@ public:
         //if(p.isPacking())
         //  CkPrintf("Migrating from %d\n", CkMyPe());
 
-        ArrayElement1D::pup(p);
         p | pass ;
         p | mcount ;
         p | ite ;
index f63406a3f5a167a00f198fb9b7063afd73bb64d3..795b78512fb085dbd2ba98b2a6f45158433453cc 100644 (file)
@@ -189,7 +189,6 @@ public:
         //if(p.isPacking())
         //  CkPrintf("Migrating from %d\n", CkMyPe());
 
-        ArrayElement1D::pup(p);
         p | pass ;
         p | mcount ;
         p | time;
index 8572d82a6a7b503ef5ee9bd59ae9b124296f430c..0454a303c8297927692be8dae474877829fe6349 100644 (file)
@@ -182,7 +182,6 @@ public:
         //if(p.isPacking())
         //            CkPrintf("Migrating from %d\n", CkMyPe());
 
-        ArrayElement1D::pup(p);
         p | pass ;
         p | mcount ;
         p | time;
index be9a85943421331a96190f02fccfa64d787a5374..d17eaf512bd745052b5279398e3d96912299fe71 100644 (file)
@@ -217,7 +217,6 @@ public:
   }
 
   void pup(PUP::er &p) {
-      ArrayElement1D::pup(p);
       p | comlib;
       p | verbose;
 
index 56dbe61ebdb086210cfb0fff01c9c8789f43cc37..acc65c650cd92cf7374a4b0d4d9275ee73a6002c 100644 (file)
@@ -149,10 +149,6 @@ public:
   }
   
   void pup(PUP::er &p){
-         
-         // Call PUP Routine for superclass
-         CBase_EachToManyArray::pup(p);
-         
          if(p.isUnpacking()){
                  localProxy = thisProxy;
                  ComlibAssociateProxy(stratEachToManyArray, localProxy);
index d3579f85e490bf5ab08b6d4369b3a68c7d387a80..c1ebecf53ba04e8e3db783f0be1a0d3d2b5c9c73 100644 (file)
@@ -453,7 +453,6 @@ public:
   }
 
   void pup(PUP::er &p){
-         CBase_EachToManyArray::pup(p);
          p | cinst;
          if(p.isUnpacking()){
                  myDelegatedProxy = thisProxy;
index 0db54e20092e614502c935100c37f09285a76b25..0df7ea9287e07e4fd73ab5aa26deb710871e0f17 100644 (file)
@@ -204,7 +204,6 @@ CmiPrintf("start %d elements\n", nElements);
   }
 
   void pup(PUP::er &p) {
-    ArrayElement1D::pup(p);//Call superclass
     p|sid;
     p(init);
     p|cnt;
index 2486e6954bb0aa9f843a7ecad7dfa3e3109d3096..841f51d7313db03e118d32ba32aac1392ba081be 100644 (file)
@@ -182,7 +182,6 @@ public:
   }
 
   void pup(PUP::er &p) {
-    ArrayElement1D::pup(p);//Call superclass
     p|sid;
     p(init);
     p|cnt;
index e8b29d43e87281da01df17180372bd7c5e53074e..76ebd26bcfa8acaedf9d0890454f2ebfb4a270c5 100644 (file)
@@ -204,7 +204,6 @@ public:
 
   virtual void pup(PUP::er &p)
   {
-     ArrayElement1D::pup(p);           // pack our superclass
      p(nTimes);
      p(sendTime);
      p(usec);
index f7fd986a2fa73e5eb0bc18357a135458ae655396..687ba4fd629cd4cefbd1d1c06560a6cd011e17e7 100644 (file)
@@ -63,7 +63,6 @@ void MyArray::ResumeFromSync(void) {
 };
 
 void MyArray::pup(PUP::er &p) {
-  CBase_MyArray::pup(p);
   p|length;
   p|iterations;
 }
index 7b3d9818cf358e982f3dabdb30955f75eccb26d7..0a69216c00f29e54dd02c6a4c8f8ab0a450c9d8d 100644 (file)
@@ -193,10 +193,7 @@ public: \
        virtual void inhVirtual(int t) { \
                if (t!=type+gen+methInh) badMeth(#className,"inhVirtual",t); \
                ((CProxy_inhCoord)coordinator).done();\
-        }\
-       virtual void pup(PUP::er &p) {\
-               parentName::pup(p);\
-       }\
+        }
 
 //Declares the parent method
 #define PARENT(className,type) \
index 325aa327f1718fb0193cb68db4bc80b103365a33..32215f6ec583328adf2b1ac8ab9fab7058581277 100644 (file)
@@ -24,7 +24,6 @@ mig_Element::mig_Element()
 
 void mig_Element::pup(PUP::er &p)
 {
-  ArrayElement1D::pup(p);//Call superclass
   p(origPE);
   p(sum);
   p(numDone);
index 413e8856e49af4a74914c128cb435559290947f0..632d551d61468fad69b1cc590c4fb1b02d74116b 100644 (file)
@@ -46,7 +46,6 @@ public:
   void pup(PUP::er &p) {
     CkPrintf("called PUP for cell %s\n", p.isPacking() ? "packing" : "unpacking or sizing");
  
-    CBase_Cell::pup(p);
     __sdag_pup(p);
 
     p | val;