From 7a6cda991530685c0272fe2fe1bbafa939583a74 Mon Sep 17 00:00:00 2001 From: David Rose Date: Mon, 6 Feb 2006 19:40:02 +0000 Subject: [PATCH] better cycle() --- panda/src/putil/pipeline.cxx | 83 ++++++++++++++++++- panda/src/putil/pipeline.h | 5 ++ panda/src/putil/pipelineCyclerTrueImpl.I | 61 ++++++++++++-- panda/src/putil/pipelineCyclerTrueImpl.cxx | 92 +++++++++++++++------- panda/src/putil/pipelineCyclerTrueImpl.h | 14 ++-- 5 files changed, 211 insertions(+), 44 deletions(-) diff --git a/panda/src/putil/pipeline.cxx b/panda/src/putil/pipeline.cxx index 800c1daa52..a62a8eb811 100644 --- a/panda/src/putil/pipeline.cxx +++ b/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) - ReMutexHolder holder(_lock); - Cyclers::iterator ci; - for (ci = _cyclers.begin(); ci != _cyclers.end(); ++ci) { - (*ci)->cycle(); + 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 = _dirty_cyclers.begin(); ci != _dirty_cyclers.end(); ++ci) { + 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 diff --git a/panda/src/putil/pipeline.h b/panda/src/putil/pipeline.h index d6fd66142b..b7f21cd06d 100644 --- a/panda/src/putil/pipeline.h +++ b/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 Cyclers; Cyclers _cyclers; + Cyclers _dirty_cyclers; + + // This is true only during cycle(). + bool _cycling; ReMutex _lock; #endif // DO_PIPELINING && HAVE_THREADS diff --git a/panda/src/putil/pipelineCyclerTrueImpl.I b/panda/src/putil/pipelineCyclerTrueImpl.I index c57b3ed356..ff4418f087 100644 --- a/panda/src/putil/pipelineCyclerTrueImpl.I +++ b/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; +} diff --git a/panda/src/putil/pipelineCyclerTrueImpl.cxx b/panda/src/putil/pipelineCyclerTrueImpl.cxx index 3b04b344a6..317d3c7f80 100644 --- a/panda/src/putil/pipelineCyclerTrueImpl.cxx +++ b/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 ©) : - _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 ©) : 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 ©) { 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 ©) { 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) { - _data[k]._cycle_data = new_data; + while (k >= 0 && _data[k] == old_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; diff --git a/panda/src/putil/pipelineCyclerTrueImpl.h b/panda/src/putil/pipelineCyclerTrueImpl.h index 69ccf26e2d..ddcd62067b 100644 --- a/panda/src/putil/pipelineCyclerTrueImpl.h +++ b/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 { - public: - PT(CycleData) _cycle_data; - }; - - StageData *_data; + // An array of PT(CycleData) objects. + PT(CycleData) *_data; int _num_stages; + bool _dirty; ReMutex _lock;