better cycle()

panda/src/putil/pipeline.cxx

@@ -32,6 +32,10 @@ Pipeline(const string &name) :
   Namable(name)
 {
   _num_stages = 1;
+
+#if defined(DO_PIPELINING) && defined(HAVE_THREADS)
+  _cycling = false;
+#endif  // DO_PIPELINING && HAVE_THREADS
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -43,6 +47,7 @@ Pipeline::
 ~Pipeline() {
 #if defined(DO_PIPELINING) && defined(HAVE_THREADS)
   nassertv(_cyclers.empty());
+  nassertv(!_cycling);
 #endif  // DO_PIPELINING && HAVE_THREADS
 }
 
@@ -54,11 +59,44 @@ Pipeline::
 void Pipeline::
 cycle() {
 #if defined(DO_PIPELINING) && defined(HAVE_THREADS)
+  pvector< PT(CycleData) > saved_cdatas;
+  saved_cdatas.reserve(_dirty_cyclers.size());
+  {
     ReMutexHolder holder(_lock);
+    
+    nassertv(!_cycling);
+    _cycling = true;
+    
+    Cyclers next_dirty_cyclers;
+    
     Cyclers::iterator ci;
-  for (ci = _cyclers.begin(); ci != _cyclers.end(); ++ci) {
+    for (ci = _dirty_cyclers.begin(); ci != _dirty_cyclers.end(); ++ci) {
-    (*ci)->cycle();
+      PipelineCyclerTrueImpl *cycler = (*ci);
+      ReMutexHolder holder2(cycler->_lock);
+      
+      // We save the result of cycle(), so that we can defer the
+      // side-effects that might occur when CycleDatas destruct, at
+      // least until the end of this loop.
+      saved_cdatas.push_back(cycler->cycle());
+      
+      if (cycler->_dirty) {
+        // The cycler is still dirty after cycling.  Preserve it in the
+        // set for next time.
+        bool inserted = next_dirty_cyclers.insert(cycler).second;
+        nassertv(inserted);
       }
+    }
+    
+    // Finally, we're ready for the next frame.
+    _dirty_cyclers.swap(next_dirty_cyclers);
+    _cycling = false;
+  }
+
+  // And now it's safe to let the CycleData pointers in saved_cdatas
+  // destruct, which may cause cascading deletes, and which will in
+  // turn case PipelineCyclers to remove themselves from (or add
+  // themselves to) the _dirty_cyclers list.
+
 #endif  // DO_PIPELINING && HAVE_THREADS
 }
 
@@ -122,11 +160,37 @@ get_num_stages() const {
 void Pipeline::
 add_cycler(PipelineCyclerTrueImpl *cycler) {
   ReMutexHolder holder(_lock);
+  nassertv(!cycler->_dirty);
+  nassertv(!_cycling);
   bool inserted = _cyclers.insert(cycler).second;
   nassertv(inserted);
 }
 #endif  // DO_PIPELINING && HAVE_THREADS
 
+#if defined(DO_PIPELINING) && defined(HAVE_THREADS)
+////////////////////////////////////////////////////////////////////
+//     Function: Pipeline::add_dirty_cycler
+//       Access: Public
+//  Description: Marks the indicated cycler as "dirty", meaning it
+//               will need to be cycled next frame.  This both adds it
+//               to the "dirty" set and also sets the "dirty" flag
+//               within the cycler.  This method only exists when true
+//               pipelining is configured on.
+////////////////////////////////////////////////////////////////////
+void Pipeline::
+add_dirty_cycler(PipelineCyclerTrueImpl *cycler) {
+  nassertv(cycler->_lock.debug_is_locked());
+
+  ReMutexHolder holder(_lock);
+  nassertv(!_cycling);
+  nassertv(!cycler->_dirty);
+  cycler->_dirty = true;
+
+  bool inserted = _dirty_cyclers.insert(cycler).second;
+  nassertv(inserted);
+}
+#endif  // DO_PIPELINING && HAVE_THREADS
+
 #if defined(DO_PIPELINING) && defined(HAVE_THREADS)
 ////////////////////////////////////////////////////////////////////
 //     Function: Pipeline::remove_cycler
@@ -137,10 +201,21 @@ add_cycler(PipelineCyclerTrueImpl *cycler) {
 ////////////////////////////////////////////////////////////////////
 void Pipeline::
 remove_cycler(PipelineCyclerTrueImpl *cycler) {
+  nassertv(cycler->_lock.debug_is_locked());
+
   ReMutexHolder holder(_lock);
+  nassertv(!_cycling);
+  
   Cyclers::iterator ci = _cyclers.find(cycler);
   nassertv(ci != _cyclers.end());
   _cyclers.erase(ci);
+
+  if (cycler->_dirty) {
+    cycler->_dirty = false;
+    Cyclers::iterator ci = _dirty_cyclers.find(cycler);
+    nassertv(ci != _dirty_cyclers.end());
+    _dirty_cyclers.erase(ci);
+  }
 }
 #endif  // DO_PIPELINING && HAVE_THREADS
 

panda/src/putil/pipeline.h

@@ -54,6 +54,7 @@ public:
 
 #if defined(DO_PIPELINING) && defined(HAVE_THREADS)
   void add_cycler(PipelineCyclerTrueImpl *cycler);
+  void add_dirty_cycler(PipelineCyclerTrueImpl *cycler);
   void remove_cycler(PipelineCyclerTrueImpl *cycler);
 #endif  // DO_PIPELINING && HAVE_THREADS
 
@@ -66,6 +67,10 @@ private:
 #if defined(DO_PIPELINING) && defined(HAVE_THREADS)
   typedef pset<PipelineCyclerTrueImpl *> Cyclers;
   Cyclers _cyclers;
+  Cyclers _dirty_cyclers;
+
+  // This is true only during cycle().
+  bool _cycling;
 
   ReMutex _lock;
 #endif  // DO_PIPELINING && HAVE_THREADS

panda/src/putil/pipelineCyclerTrueImpl.I

@@ -33,7 +33,7 @@ read() const {
   int pipeline_stage = Thread::get_current_thread()->get_pipeline_stage();
   nassertr(pipeline_stage >= 0 && pipeline_stage < _num_stages, NULL);
   _lock.lock();
-  return _data[pipeline_stage]._cycle_data;
+  return _data[pipeline_stage];
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -48,7 +48,7 @@ increment_read(const CycleData *pointer) const {
 #ifndef NDEBUG
   int pipeline_stage = Thread::get_current_thread()->get_pipeline_stage();
   nassertv(pipeline_stage >= 0 && pipeline_stage < _num_stages);
-  nassertv(_data[pipeline_stage]._cycle_data == pointer);
+  nassertv(_data[pipeline_stage] == pointer);
 #endif
   _lock.lock();
 }
@@ -64,7 +64,7 @@ release_read(const CycleData *pointer) const {
 #ifndef NDEBUG
   int pipeline_stage = Thread::get_current_thread()->get_pipeline_stage();
   nassertv(pipeline_stage >= 0 && pipeline_stage < _num_stages);
-  nassertv(_data[pipeline_stage]._cycle_data == pointer);
+  nassertv(_data[pipeline_stage] == pointer);
 #endif
   _lock.release();
 }
@@ -146,7 +146,7 @@ is_stage_unique(int n) const {
   if (n == 0) {
     return true;
   } else {
-    return _data[n]._cycle_data == _data[n - 1]._cycle_data;
+    return _data[n] == _data[n - 1];
   }
 }
 
@@ -160,7 +160,7 @@ INLINE void PipelineCyclerTrueImpl::
 release_write_stage(int n, CycleData *pointer) {
 #ifndef NDEBUG
   nassertv(n >= 0 && n < _num_stages);
-  nassertv(_data[n]._cycle_data == pointer);
+  nassertv(_data[n] == pointer);
 #endif
   _lock.release();
 }
@@ -179,7 +179,7 @@ INLINE CycleData *PipelineCyclerTrueImpl::
 cheat() const {
   int pipeline_stage = Thread::get_current_thread()->get_pipeline_stage();
   nassertr(pipeline_stage >= 0 && pipeline_stage < _num_stages, NULL);
-  return _data[pipeline_stage]._cycle_data;
+  return _data[pipeline_stage];
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -212,3 +212,52 @@ get_write_count() const {
   _lock.lock();
   return _lock.get_lock_count();
 }
+
+////////////////////////////////////////////////////////////////////
+//     Function: PipelineCyclerTrueImpl::cycle_2
+//       Access: Private
+//  Description: This is a special implementation of cycle() for the
+//               special case of just two stages to the pipeline.  It
+//               does the same thing as cycle(), but is a little bit
+//               faster because it knows there are exactly two stages.
+////////////////////////////////////////////////////////////////////
+INLINE PT(CycleData) PipelineCyclerTrueImpl::
+cycle_2() {
+  PT(CycleData) last_val = _data[1];
+  nassertr(_lock.debug_is_locked(), last_val);
+  nassertr(_dirty, last_val);
+  nassertr(_num_stages == 2, last_val);
+
+  _data[1] = _data[0];
+
+  // No longer dirty.
+  _dirty = false;
+  return last_val;
+}
+
+////////////////////////////////////////////////////////////////////
+//     Function: PipelineCyclerTrueImpl::cycle_3
+//       Access: Private
+//  Description: This is a special implementation of cycle() for the
+//               special case of exactly three stages to the pipeline.
+//               It does the same thing as cycle(), but is a little
+//               bit faster because it knows there are exactly three
+//               stages.
+////////////////////////////////////////////////////////////////////
+INLINE PT(CycleData) PipelineCyclerTrueImpl::
+cycle_3() {
+  PT(CycleData) last_val = _data[2];
+  nassertr(_lock.debug_is_locked(), last_val);
+  nassertr(_dirty, last_val);
+  nassertr(_num_stages == 3, last_val);
+
+  _data[2] = _data[1];
+  _data[1] = _data[0];
+
+  if (_data[2] == _data[1]) {
+    // No longer dirty.
+    _dirty = false;
+  }
+
+  return last_val;
+}

panda/src/putil/pipelineCyclerTrueImpl.cxx

@@ -22,8 +22,6 @@
 
 #include "config_util.h"
 #include "pipeline.h"
-#include "reMutexHolder.h"
-
 
 ////////////////////////////////////////////////////////////////////
 //     Function: PipelineCyclerTrueImpl::Constructor
@@ -32,7 +30,8 @@
 ////////////////////////////////////////////////////////////////////
 PipelineCyclerTrueImpl::
 PipelineCyclerTrueImpl(CycleData *initial_data, Pipeline *pipeline) :
-  _pipeline(pipeline)
+  _pipeline(pipeline),
+  _dirty(false)
 {
   if (_pipeline == (Pipeline *)NULL) {
     _pipeline = Pipeline::get_render_pipeline();
@@ -40,9 +39,9 @@ PipelineCyclerTrueImpl(CycleData *initial_data, Pipeline *pipeline) :
   _pipeline->add_cycler(this);
 
   _num_stages = _pipeline->get_num_stages();
-  _data = new StageData[_num_stages];
+  _data = new PT(CycleData)[_num_stages];
   for (int i = 0; i < _num_stages; ++i) {
-    _data[i]._cycle_data = initial_data;
+    _data[i] = initial_data;
   }
 }
 
@@ -53,14 +52,19 @@ PipelineCyclerTrueImpl(CycleData *initial_data, Pipeline *pipeline) :
 ////////////////////////////////////////////////////////////////////
 PipelineCyclerTrueImpl::
 PipelineCyclerTrueImpl(const PipelineCyclerTrueImpl &copy) :
-  _pipeline(copy._pipeline)
+  _pipeline(copy._pipeline),
+  _dirty(false)
 {
+  ReMutexHolder holder(_lock);
   _pipeline->add_cycler(this);
+  if (copy._dirty) {
+    _pipeline->add_dirty_cycler(this);
+  }
 
-  ReMutexHolder holder(copy._lock);
+  ReMutexHolder holder2(copy._lock);
   _num_stages = _pipeline->get_num_stages();
   nassertv(_num_stages == copy._num_stages);
-  _data = new StageData[_num_stages];
+  _data = new PT(CycleData)[_num_stages];
 
   // It's important that we preserve pointerwise equivalence in the
   // copy: if a and b of the original pipeline are the same pointer,
@@ -73,11 +77,11 @@ PipelineCyclerTrueImpl(const PipelineCyclerTrueImpl &copy) :
   Pointers pointers;
 
   for (int i = 0; i < _num_stages; ++i) {
-    PT(CycleData) &new_pt = pointers[copy._data[i]._cycle_data];
+    PT(CycleData) &new_pt = pointers[copy._data[i]];
     if (new_pt == NULL) {
-      new_pt = copy._data[i]._cycle_data->make_copy();
+      new_pt = copy._data[i]->make_copy();
     }
-    _data[i]._cycle_data = new_pt;
+    _data[i] = new_pt;
   }
 }
 
@@ -93,7 +97,7 @@ operator = (const PipelineCyclerTrueImpl &copy) {
 
   nassertv(_num_stages == copy._num_stages);
   for (int i = 0; i < _num_stages; ++i) {
-    _data[i]._cycle_data = copy._data[i]._cycle_data;
+    _data[i] = copy._data[i];
   }
 }
 
@@ -105,9 +109,11 @@ operator = (const PipelineCyclerTrueImpl &copy) {
 PipelineCyclerTrueImpl::
 ~PipelineCyclerTrueImpl() {
   ReMutexHolder holder(_lock);
-  _pipeline->remove_cycler(this);
-
   delete[] _data;
+  _data = NULL;
+  _num_stages = 0;
+
+  _pipeline->remove_cycler(this);
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -131,7 +137,7 @@ write_stage(int n) {
   }
 #endif  // NDEBUG
 
-  CycleData *old_data = _data[n]._cycle_data;
+  CycleData *old_data = _data[n];
 
   if (old_data->get_ref_count() != 1) {
     // Copy-on-write.
@@ -154,7 +160,7 @@ write_stage(int n) {
     // at all.
     int count = old_data->get_ref_count() - 1;
     int k = n - 1;
-    while (k >= 0 && _data[k]._cycle_data == old_data) {
+    while (k >= 0 && _data[k] == old_data) {
       --k;
       --count;
     }
@@ -163,31 +169,63 @@ write_stage(int n) {
       PT(CycleData) new_data = old_data->make_copy();
 
       int k = n - 1;
-      while (k >= 0 && _data[k]._cycle_data == old_data) {
+      while (k >= 0 && _data[k] == old_data) {
-        _data[k]._cycle_data = new_data;
+        _data[k] = new_data;
         --k;
       }
       
-      _data[n]._cycle_data = new_data;
+      _data[n] = new_data;
+
+      // Now we have differences between some of the data pointers, so
+      // we're "dirty".  Mark it so.
+      if (!_dirty) {
+        _pipeline->add_dirty_cycler(this);
+      }
     }
   }
 
-  return _data[n]._cycle_data;
+  return _data[n];
 }
 
 ////////////////////////////////////////////////////////////////////
 //     Function: PipelineCyclerTrueImpl::cycle
 //       Access: Private
 //  Description: Cycles the data between frames.  This is only called
-//               from Pipeline::cycle().
+//               from Pipeline::cycle(), and presumably it is only
+//               called if the cycler is "dirty".
+//
+//               At the conclusion of this method, the cycler should
+//               clear its dirty flag if it is no longer "dirty"--that
+//               is, if all of the pipeline pointers are the same.
+//
+//               The return value is the CycleData pointer which fell
+//               off the end of the cycle.  If this is allowed to
+//               destruct immediately, there may be side-effects that
+//               cascade through the system, so the caller may choose
+//               to hold the pointer until it can safely be released
+//               later.
 ////////////////////////////////////////////////////////////////////
-void PipelineCyclerTrueImpl::
+PT(CycleData) PipelineCyclerTrueImpl::
 cycle() {
-  ReMutexHolder holder(_lock);
+  PT(CycleData) last_val = _data[_num_stages - 1];
+  nassertr(_lock.debug_is_locked(), last_val);
+  nassertr(_dirty, last_val);
 
-  for (int i = _num_stages - 1; i > 0; --i) {
+  int i;
+  for (i = _num_stages - 1; i > 0; --i) {
     _data[i] = _data[i - 1];
   }
+
+  for (i = 1; i < _num_stages; ++i) {
+    if (_data[i] != _data[i - 1]) {
+      // Still dirty.
+      return last_val;
+    }
+  }
+
+  // No longer dirty.
+  _dirty = false;
+  return last_val;
 }
 
 ////////////////////////////////////////////////////////////////////
@@ -204,7 +242,7 @@ set_num_stages(int num_stages) {
     // Don't bother to reallocate the array smaller; we just won't use
     // the rest of the array.
     for (int i = _num_stages; i < num_stages; ++i) {
-      _data[i]._cycle_data.clear();
+      _data[i].clear();
     }
 
     _num_stages = num_stages;
@@ -212,13 +250,13 @@ set_num_stages(int num_stages) {
 
   } else {
     // To increase the array, we must reallocate it larger.
-    StageData *new_data = new StageData[num_stages];
+    PT(CycleData) *new_data = new PT(CycleData)[num_stages];
     int i;
     for (i = 0; i < _num_stages; ++i) {
       new_data[i] = _data[i];
     }
     for (i = _num_stages; i < num_stages; ++i) {
-      new_data[i]._cycle_data = _data[_num_stages - 1]._cycle_data;
+      new_data[i] = _data[_num_stages - 1];
     }
     delete[] _data;
 

panda/src/putil/pipelineCyclerTrueImpl.h

@@ -27,6 +27,7 @@
 #include "pointerTo.h"
 #include "thread.h"
 #include "reMutex.h"
+#include "reMutexHolder.h"
 
 class Pipeline;
 
@@ -70,19 +71,18 @@ public:
   INLINE int get_write_count() const;
 
 private:
-  void cycle();
+  PT(CycleData) cycle();
+  INLINE PT(CycleData) cycle_2();
+  INLINE PT(CycleData) cycle_3();
   void set_num_stages(int num_stages);
 
 private:
   Pipeline *_pipeline;
 
-  class StageData {
+  // An array of PT(CycleData) objects.
-  public:
+  PT(CycleData) *_data;
-    PT(CycleData) _cycle_data;
-  };
-
-  StageData *_data;
   int _num_stages;
+  bool _dirty;
 
   ReMutex _lock;