diff --git a/panda/src/express/referenceCount.I b/panda/src/express/referenceCount.I index 0f6bf3cda9..0de88fbd05 100644 --- a/panda/src/express/referenceCount.I +++ b/panda/src/express/referenceCount.I @@ -346,7 +346,12 @@ weak_unref(WeakPointerToVoid *ptv) { template INLINE void unref_delete(RefCountType *ptr) { - if (((ReferenceCount *)ptr)->unref() == 0) { + // Although it may be tempting to try to upcast ptr to a + // ReferenceCount object (particularly to get around inheritance + // issues), resist that temptation, since some classes (in + // particular, TransformState and RenderState) rely on a non-virtual + // overloading of the unref() method. + if (ptr->unref() == 0) { #ifndef NDEBUG if (get_leak_memory()) { // In leak-memory mode, we don't actually delete the pointer, diff --git a/panda/src/pgraph/config_pgraph.cxx b/panda/src/pgraph/config_pgraph.cxx index d66f8d4f1a..7772dfb745 100644 --- a/panda/src/pgraph/config_pgraph.cxx +++ b/panda/src/pgraph/config_pgraph.cxx @@ -144,6 +144,13 @@ ConfigVariableBool paranoid_const "RenderAttrib, TransformState, and RenderEffect. This has no effect " "if NDEBUG is defined.")); +ConfigVariableBool auto_break_cycles +("auto-break-cycles", true, + PRC_DESC("Set this true to automatically detect and break reference-count " + "cycles in the TransformState and RenderState caches. When this " + "is false, you must explicitly call TransformState.clear_cache() " + "from time to time to prevent gradual memory bloat.")); + ConfigVariableBool polylight_info ("polylight-info", false, PRC_DESC("Set this true to view some info statements regarding the polylight. " diff --git a/panda/src/pgraph/config_pgraph.h b/panda/src/pgraph/config_pgraph.h index 825f3c4bf5..aedb4c88d2 100644 --- a/panda/src/pgraph/config_pgraph.h +++ b/panda/src/pgraph/config_pgraph.h @@ -39,6 +39,7 @@ extern ConfigVariableBool allow_unrelated_wrt; extern ConfigVariableBool paranoid_compose; extern ConfigVariableBool compose_componentwise; extern ConfigVariableBool paranoid_const; +extern ConfigVariableBool auto_break_cycles; extern ConfigVariableBool polylight_info; extern ConfigVariableDouble lod_fade_time; diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index b8a8630ca9..e2728ff90c 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -22,6 +22,7 @@ #include "cullBinManager.h" #include "fogAttrib.h" #include "transparencyAttrib.h" +#include "pStatTimer.h" #include "config_pgraph.h" #include "bamReader.h" #include "bamWriter.h" @@ -31,6 +32,9 @@ RenderState::States *RenderState::_states = NULL; CPT(RenderState) RenderState::_empty_state; +UpdateSeq RenderState::_last_cycle_detect; +PStatCollector RenderState::_cache_update_pcollector("App:State Cache"); + TypeHandle RenderState::_type_handle; @@ -93,100 +97,7 @@ RenderState:: _saved_entry = _states->end(); } - // Now make sure we clean up all other floating pointers to the - // RenderState. These may be scattered around in the various - // CompositionCaches from other RenderState objects. - - // Fortunately, since we added CompositionCache records in pairs, we - // know exactly the set of RenderState objects that have us in their - // cache: it's the same set of RenderState objects that we have in - // our own cache. - - // We do need to put considerable thought into this loop, because as - // we clear out cache entries we'll cause other RenderState - // objects to destruct, which could cause things to get pulled out - // of our own _composition_cache map. We want to allow this (so - // that we don't encounter any just-destructed pointers in our - // cache), but we don't want to get bitten by this cascading effect. - // Instead of walking through the map from beginning to end, - // therefore, we just pull out the first one each time, and erase - // it. - - // There are lots of ways to do this loop wrong. Be very careful if - // you need to modify it for any reason. - while (!_composition_cache.empty()) { - CompositionCache::iterator ci = _composition_cache.begin(); - - // It is possible that the "other" RenderState object is - // currently within its own destructor. We therefore can't use a - // PT() to hold its pointer; that could end up calling its - // destructor twice. Fortunately, we don't need to hold its - // reference count to ensure it doesn't destruct while we process - // this loop; as long as we ensure that no *other* RenderState - // objects destruct, there will be no reason for that one to. - RenderState *other = (RenderState *)(*ci).first; - - // We hold a copy of the composition result so we can dereference - // it later. - Composition comp = (*ci).second; - - // Now we can remove the element from our cache. We do this now, - // rather than later, before any other RenderState objects have - // had a chance to destruct, so we are confident that our iterator - // is still valid. - _composition_cache.erase(ci); - - if (other != this) { - CompositionCache::iterator oci = other->_composition_cache.find(this); - - // We may or may not still be listed in the other's cache (it - // might be halfway through pulling entries out, from within its - // own destructor). - if (oci != other->_composition_cache.end()) { - // Hold a copy of the other composition result, too. - Composition ocomp = (*oci).second; - - other->_composition_cache.erase(oci); - - // It's finally safe to let our held pointers go away. This may - // have cascading effects as other RenderState objects are - // destructed, but there will be no harm done if they destruct - // now. - if (ocomp._result != (const RenderState *)NULL && ocomp._result != other) { - unref_delete(ocomp._result); - } - } - } - - // It's finally safe to let our held pointers go away. (See - // comment above.) - if (comp._result != (const RenderState *)NULL && comp._result != this) { - unref_delete(comp._result); - } - } - - // A similar bit of code for the invert cache. - while (!_invert_composition_cache.empty()) { - CompositionCache::iterator ci = _invert_composition_cache.begin(); - RenderState *other = (RenderState *)(*ci).first; - nassertv(other != this); - Composition comp = (*ci).second; - _invert_composition_cache.erase(ci); - if (other != this) { - CompositionCache::iterator oci = - other->_invert_composition_cache.find(this); - if (oci != other->_invert_composition_cache.end()) { - Composition ocomp = (*oci).second; - other->_invert_composition_cache.erase(oci); - if (ocomp._result != (const RenderState *)NULL && ocomp._result != other) { - unref_delete(ocomp._result); - } - } - } - if (comp._result != (const RenderState *)NULL && comp._result != this) { - unref_delete(comp._result); - } - } + remove_cache_pointers(); // If this was true at the beginning of the destructor, but is no // longer true now, probably we've been double-deleted. @@ -371,7 +282,7 @@ compose(const RenderState *other) const { if (result != (const RenderState *)this) { // See the comments below about the need to up the reference // count only when the result is not the same as this. - result->ref(); + result->cache_ref(); } } // Here's the cache! @@ -396,7 +307,7 @@ compose(const RenderState *other) const { // explicitly increment the reference count. We have to be sure // to decrement it again later, when the composition entry is // removed from the cache. - result->ref(); + result->cache_ref(); // (If the result was just this again, we still store the // result, but we don't increment the reference count, since @@ -449,7 +360,7 @@ invert_compose(const RenderState *other) const { if (result != (const RenderState *)this) { // See the comments below about the need to up the reference // count only when the result is not the same as this. - result->ref(); + result->cache_ref(); } } // Here's the cache! @@ -473,7 +384,7 @@ invert_compose(const RenderState *other) const { // explicitly increment the reference count. We have to be sure // to decrement it again later, when the composition entry is // removed from the cache. - result->ref(); + result->cache_ref(); // (If the result was just this again, we still store the // result, but we don't increment the reference count, since @@ -609,6 +520,54 @@ get_override(TypeHandle type) const { return 0; } +//////////////////////////////////////////////////////////////////// +// Function: RenderState::unref +// Access: Published +// Description: This method overrides ReferenceCount::unref() to +// check whether the remaining reference count is +// entirely in the cache, and if so, it checks for and +// breaks a cycle in the cache involving this object. +// This is designed to prevent leaks from cyclical +// references within the cache. +// +// Note that this is not a virtual method, and cannot be +// because ReferenceCount itself declares no virtual +// methods (it avoids the overhead of a virtual function +// pointer). But this doesn't matter, because +// PT(TransformState) is a template class, and will call +// the appropriate method even though it is non-virtual. +//////////////////////////////////////////////////////////////////// +int RenderState:: +unref() const { + if (get_cache_ref_count() > 0 && + get_ref_count() == get_cache_ref_count() + 1) { + // If we are about to remove the one reference that is not in the + // cache, leaving only references in the cache, then we need to + // check for a cycle involving this RenderState and break it if + // it exists. + + if (auto_break_cycles) { + // There might be a tiny race condition if multiple different + // threads perform cycle detects on related nodes at the same + // time. But the cost of failing the race condition is low--we + // end up with a tiny leak that may eventually be discovered, big + // deal. + ++_last_cycle_detect; + if (r_detect_cycles(this, this, 1, _last_cycle_detect, NULL)) { + // Ok, we have a cycle. This will be a leak unless we break the + // cycle by freeing the cache on this object. + if (pgraph_cat.is_debug()) { + pgraph_cat.debug() + << "Breaking cycle involving " << (*this) << "\n"; + } + ((RenderState *)this)->remove_cache_pointers(); + } + } + } + + return ReferenceCount::unref(); +} + //////////////////////////////////////////////////////////////////// // Function: RenderState::output // Access: Published, Virtual @@ -750,6 +709,7 @@ get_num_unused_states() { for (sci = state_count.begin(); sci != state_count.end(); ++sci) { const RenderState *state = (*sci).first; int count = (*sci).second; + nassertr(count == state->get_cache_ref_count(), num_unused); nassertr(count <= state->get_ref_count(), num_unused); if (count == state->get_ref_count()) { num_unused++; @@ -779,14 +739,9 @@ get_num_unused_states() { // eliminate RenderState objects that are still in // use. // -// Normally, RenderState objects will remove -// themselves from the interal map when their reference -// counts go to 0, but since circular references are -// possible there may be some cycles that cannot remove -// themselves. Calling this function from time to time -// will ensure there is no wasteful memory leakage, but -// calling it too often may result in decreased -// performance as the cache is forced to be recomputed. +// Nowadays, this method should not be necessary, as +// reference-count cycles in the composition cache +// should be automatically detected and broken. // // The return value is the number of RenderStates // freed by this operation. @@ -797,6 +752,7 @@ clear_cache() { return 0; } + PStatTimer timer(_cache_update_pcollector); int orig_size = _states->size(); // First, we need to copy the entire set of states to a temporary @@ -824,7 +780,7 @@ clear_cache() { ++ci) { const RenderState *result = (*ci).second._result; if (result != (const RenderState *)NULL && result != state) { - result->unref(); + result->cache_unref(); nassertr(result->get_ref_count() > 0, 0); } } @@ -835,7 +791,7 @@ clear_cache() { ++ci) { const RenderState *result = (*ci).second._result; if (result != (const RenderState *)NULL && result != state) { - result->unref(); + result->cache_unref(); nassertr(result->get_ref_count() > 0, 0); } } @@ -858,11 +814,11 @@ clear_cache() { // cache and reports them to standard output. // // These cycles may be inadvertently created when state -// compositions cycle back to a starting point. In the -// current implementation, they are not automatically -// detected and broken, so they represent a kind of -// memory leak. They will not be freed unless -// clear_cache() is called explicitly. +// compositions cycle back to a starting point. +// Nowadays, these cycles should be automatically +// detected and broken, so this method should never list +// any cycles unless there is a bug in that detection +// logic. // // The cycles listed here are not leaks in the strictest // sense of the word, since they can be reclaimed by a @@ -871,6 +827,7 @@ clear_cache() { //////////////////////////////////////////////////////////////////// void RenderState:: list_cycles(ostream &out) { + typedef pset VisitedStates; VisitedStates visited; CompositionCycleDesc cycle_desc; @@ -880,8 +837,8 @@ list_cycles(ostream &out) { bool inserted = visited.insert(state).second; if (inserted) { - VisitedStates visited_this_cycle; - if (r_detect_cycles(state, state, 1, visited_this_cycle, cycle_desc)) { + ++_last_cycle_detect; + if (r_detect_cycles(state, state, 1, _last_cycle_detect, &cycle_desc)) { // This state begins a cycle. CompositionCycleDesc::reverse_iterator csi; @@ -1288,18 +1245,17 @@ do_invert_compose(const RenderState *other) const { // Description: Detects whether there is a cycle in the cache that // begins with the indicated state. Returns true if at // least one cycle is found, false if this state is not -// part of any cycles. If a cycle is found, cycle_desc -// is filled in with the list of the steps of the cycle, -// in reverse order. +// part of any cycles. If a cycle is found and +// cycle_desc is not NULL, then cycle_desc is filled in +// with the list of the steps of the cycle, in reverse +// order. //////////////////////////////////////////////////////////////////// bool RenderState:: r_detect_cycles(const RenderState *start_state, const RenderState *current_state, - int length, - RenderState::VisitedStates &visited_this_cycle, - RenderState::CompositionCycleDesc &cycle_desc) { - bool inserted = visited_this_cycle.insert(current_state).second; - if (!inserted) { + int length, UpdateSeq this_seq, + RenderState::CompositionCycleDesc *cycle_desc) { + if (current_state->_cycle_detect == this_seq) { // We've already seen this state; therefore, we've found a cycle. // However, we only care about cycles that return to the starting @@ -1308,6 +1264,7 @@ r_detect_cycles(const RenderState *start_state, // problem there. return (current_state == start_state && length > 2); } + ((RenderState *)current_state)->_cycle_detect = this_seq; CompositionCache::const_iterator ci; for (ci = current_state->_composition_cache.begin(); @@ -1316,10 +1273,12 @@ r_detect_cycles(const RenderState *start_state, const RenderState *result = (*ci).second._result; if (result != (const RenderState *)NULL) { if (r_detect_cycles(start_state, result, length + 1, - visited_this_cycle, cycle_desc)) { + this_seq, cycle_desc)) { // Cycle detected. - CompositionCycleDescEntry entry((*ci).first, result, false); - cycle_desc.push_back(entry); + if (cycle_desc != (CompositionCycleDesc *)NULL) { + CompositionCycleDescEntry entry((*ci).first, result, false); + cycle_desc->push_back(entry); + } return true; } } @@ -1331,10 +1290,12 @@ r_detect_cycles(const RenderState *start_state, const RenderState *result = (*ci).second._result; if (result != (const RenderState *)NULL) { if (r_detect_cycles(start_state, result, length + 1, - visited_this_cycle, cycle_desc)) { + this_seq, cycle_desc)) { // Cycle detected. - CompositionCycleDescEntry entry((*ci).first, result, true); - cycle_desc.push_back(entry); + if (cycle_desc != (CompositionCycleDesc *)NULL) { + CompositionCycleDescEntry entry((*ci).first, result, true); + cycle_desc->push_back(entry); + } return true; } } @@ -1344,6 +1305,118 @@ r_detect_cycles(const RenderState *start_state, return false; } +//////////////////////////////////////////////////////////////////// +// Function: RenderState::remove_cache_pointers +// Access: Private +// Description: Remove all pointers within the cache from and to this +// particular RenderState. The pointers to this +// object may be scattered around in the various +// CompositionCaches from other RenderState objects. +//////////////////////////////////////////////////////////////////// +void RenderState:: +remove_cache_pointers() { + // Now make sure we clean up all other floating pointers to the + // RenderState. These may be scattered around in the various + // CompositionCaches from other RenderState objects. + + // Fortunately, since we added CompositionCache records in pairs, we + // know exactly the set of RenderState objects that have us in their + // cache: it's the same set of RenderState objects that we have in + // our own cache. + + // We do need to put considerable thought into this loop, because as + // we clear out cache entries we'll cause other RenderState + // objects to destruct, which could cause things to get pulled out + // of our own _composition_cache map. We want to allow this (so + // that we don't encounter any just-destructed pointers in our + // cache), but we don't want to get bitten by this cascading effect. + // Instead of walking through the map from beginning to end, + // therefore, we just pull out the first one each time, and erase + // it. + +#ifdef DO_PSTATS + if (_composition_cache.empty() && _invert_composition_cache.empty()) { + return; + } + PStatTimer timer(_cache_update_pcollector); +#endif // DO_PSTATS + + // There are lots of ways to do this loop wrong. Be very careful if + // you need to modify it for any reason. + while (!_composition_cache.empty()) { + CompositionCache::iterator ci = _composition_cache.begin(); + + // It is possible that the "other" RenderState object is + // currently within its own destructor. We therefore can't use a + // PT() to hold its pointer; that could end up calling its + // destructor twice. Fortunately, we don't need to hold its + // reference count to ensure it doesn't destruct while we process + // this loop; as long as we ensure that no *other* RenderState + // objects destruct, there will be no reason for that one to. + RenderState *other = (RenderState *)(*ci).first; + + // We hold a copy of the composition result so we can dereference + // it later. + Composition comp = (*ci).second; + + // Now we can remove the element from our cache. We do this now, + // rather than later, before any other RenderState objects have + // had a chance to destruct, so we are confident that our iterator + // is still valid. + _composition_cache.erase(ci); + + if (other != this) { + CompositionCache::iterator oci = other->_composition_cache.find(this); + + // We may or may not still be listed in the other's cache (it + // might be halfway through pulling entries out, from within its + // own destructor). + if (oci != other->_composition_cache.end()) { + // Hold a copy of the other composition result, too. + Composition ocomp = (*oci).second; + + other->_composition_cache.erase(oci); + + // It's finally safe to let our held pointers go away. This may + // have cascading effects as other RenderState objects are + // destructed, but there will be no harm done if they destruct + // now. + if (ocomp._result != (const RenderState *)NULL && ocomp._result != other) { + cache_unref_delete(ocomp._result); + } + } + } + + // It's finally safe to let our held pointers go away. (See + // comment above.) + if (comp._result != (const RenderState *)NULL && comp._result != this) { + cache_unref_delete(comp._result); + } + } + + // A similar bit of code for the invert cache. + while (!_invert_composition_cache.empty()) { + CompositionCache::iterator ci = _invert_composition_cache.begin(); + RenderState *other = (RenderState *)(*ci).first; + nassertv(other != this); + Composition comp = (*ci).second; + _invert_composition_cache.erase(ci); + if (other != this) { + CompositionCache::iterator oci = + other->_invert_composition_cache.find(this); + if (oci != other->_invert_composition_cache.end()) { + Composition ocomp = (*oci).second; + other->_invert_composition_cache.erase(oci); + if (ocomp._result != (const RenderState *)NULL && ocomp._result != other) { + cache_unref_delete(ocomp._result); + } + } + } + if (comp._result != (const RenderState *)NULL && comp._result != this) { + cache_unref_delete(comp._result); + } + } +} //////////////////////////////////////////////////////////////////// // Function: RenderState::determine_bin_index diff --git a/panda/src/pgraph/renderState.h b/panda/src/pgraph/renderState.h index 7cae8f4ba0..46cd1d12b6 100644 --- a/panda/src/pgraph/renderState.h +++ b/panda/src/pgraph/renderState.h @@ -22,9 +22,11 @@ #include "pandabase.h" #include "renderAttrib.h" -#include "typedWritableReferenceCount.h" +#include "cachedTypedWritableReferenceCount.h" #include "pointerTo.h" #include "ordered_vector.h" +#include "updateSeq.h" +#include "pStatCollector.h" class GraphicsStateGuardianBase; class FogAttrib; @@ -44,7 +46,7 @@ class FactoryParams; // instead of modifying a RenderState object, create a // new one. //////////////////////////////////////////////////////////////////// -class EXPCL_PANDA RenderState : public TypedWritableReferenceCount { +class EXPCL_PANDA RenderState : public CachedTypedWritableReferenceCount { protected: RenderState(); @@ -90,6 +92,8 @@ PUBLISHED: const RenderAttrib *get_attrib(TypeHandle type) const; int get_override(TypeHandle type) const; + int unref() const; + void output(ostream &out) const; void write(ostream &out, int indent_level) const; @@ -131,16 +135,16 @@ private: bool _inverted; }; typedef pvector CompositionCycleDesc; - typedef pset VisitedStates; static CPT(RenderState) return_new(RenderState *state); CPT(RenderState) do_compose(const RenderState *other) const; CPT(RenderState) do_invert_compose(const RenderState *other) const; static bool r_detect_cycles(const RenderState *start_state, const RenderState *current_state, - int length, - VisitedStates &visited_this_cycle, - CompositionCycleDesc &cycle_desc); + int length, UpdateSeq this_seq, + CompositionCycleDesc *cycle_desc); + + void remove_cache_pointers(); void determine_bin_index(); void determine_fog(); @@ -183,6 +187,12 @@ private: CompositionCache _composition_cache; CompositionCache _invert_composition_cache; + // This is used to mark nodes as we visit them to detect cycles. + UpdateSeq _cycle_detect; + static UpdateSeq _last_cycle_detect; + + static PStatCollector _cache_update_pcollector; + private: // This is the actual data within the RenderState: a set of // RenderAttribs. @@ -239,9 +249,9 @@ public: return _type_handle; } static void init_type() { - TypedWritableReferenceCount::init_type(); + CachedTypedWritableReferenceCount::init_type(); register_type(_type_handle, "RenderState", - TypedWritableReferenceCount::get_class_type()); + CachedTypedWritableReferenceCount::get_class_type()); } virtual TypeHandle get_type() const { return get_class_type(); diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index 2c1066348b..03fdff341b 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -23,10 +23,14 @@ #include "datagramIterator.h" #include "indent.h" #include "compareTo.h" +#include "pStatTimer.h" #include "config_pgraph.h" TransformState::States *TransformState::_states = NULL; CPT(TransformState) TransformState::_identity_state; +UpdateSeq TransformState::_last_cycle_detect; +PStatCollector TransformState::_cache_update_pcollector("App:State Cache"); + TypeHandle TransformState::_type_handle; //////////////////////////////////////////////////////////////////// @@ -95,100 +99,7 @@ TransformState:: _saved_entry = _states->end(); } - // Now make sure we clean up all other floating pointers to the - // TransformState. These may be scattered around in the various - // CompositionCaches from other TransformState objects. - - // Fortunately, since we added CompositionCache records in pairs, we - // know exactly the set of TransformState objects that have us in their - // cache: it's the same set of TransformState objects that we have in - // our own cache. - - // We do need to put considerable thought into this loop, because as - // we clear out cache entries we'll cause other TransformState - // objects to destruct, which could cause things to get pulled out - // of our own _composition_cache map. We want to allow this (so - // that we don't encounter any just-destructed pointers in our - // cache), but we don't want to get bitten by this cascading effect. - // Instead of walking through the map from beginning to end, - // therefore, we just pull out the first one each time, and erase - // it. - - // There are lots of ways to do this loop wrong. Be very careful if - // you need to modify it for any reason. - while (!_composition_cache.empty()) { - CompositionCache::iterator ci = _composition_cache.begin(); - - // It is possible that the "other" TransformState object is - // currently within its own destructor. We therefore can't use a - // PT() to hold its pointer; that could end up calling its - // destructor twice. Fortunately, we don't need to hold its - // reference count to ensure it doesn't destruct while we process - // this loop; as long as we ensure that no *other* TransformState - // objects destruct, there will be no reason for that one to. - TransformState *other = (TransformState *)(*ci).first; - - // We hold a copy of the composition result so we can dereference - // it later. - Composition comp = (*ci).second; - - // Now we can remove the element from our cache. We do this now, - // rather than later, before any other TransformState objects have - // had a chance to destruct, so we are confident that our iterator - // is still valid. - _composition_cache.erase(ci); - - if (other != this) { - CompositionCache::iterator oci = other->_composition_cache.find(this); - - // We may or may not still be listed in the other's cache (it - // might be halfway through pulling entries out, from within its - // own destructor). - if (oci != other->_composition_cache.end()) { - // Hold a copy of the other composition result, too. - Composition ocomp = (*oci).second; - - other->_composition_cache.erase(oci); - - // It's finally safe to let our held pointers go away. This may - // have cascading effects as other TransformState objects are - // destructed, but there will be no harm done if they destruct - // now. - if (ocomp._result != (const TransformState *)NULL && ocomp._result != other) { - unref_delete(ocomp._result); - } - } - } - - // It's finally safe to let our held pointers go away. (See - // comment above.) - if (comp._result != (const TransformState *)NULL && comp._result != this) { - unref_delete(comp._result); - } - } - - // A similar bit of code for the invert cache. - while (!_invert_composition_cache.empty()) { - CompositionCache::iterator ci = _invert_composition_cache.begin(); - TransformState *other = (TransformState *)(*ci).first; - nassertv(other != this); - Composition comp = (*ci).second; - _invert_composition_cache.erase(ci); - if (other != this) { - CompositionCache::iterator oci = - other->_invert_composition_cache.find(this); - if (oci != other->_invert_composition_cache.end()) { - Composition ocomp = (*oci).second; - other->_invert_composition_cache.erase(oci); - if (ocomp._result != (const TransformState *)NULL && ocomp._result != other) { - unref_delete(ocomp._result); - } - } - } - if (comp._result != (const TransformState *)NULL && comp._result != this) { - unref_delete(comp._result); - } - } + remove_cache_pointers(); // If this was true at the beginning of the destructor, but is no // longer true now, probably we've been double-deleted. @@ -555,7 +466,7 @@ compose(const TransformState *other) const { if (result != (const TransformState *)this) { // See the comments below about the need to up the reference // count only when the result is not the same as this. - result->ref(); + result->cache_ref(); } } // Here's the cache! @@ -580,7 +491,7 @@ compose(const TransformState *other) const { // explicitly increment the reference count. We have to be sure // to decrement it again later, when the composition entry is // removed from the cache. - result->ref(); + result->cache_ref(); // (If the result was just this again, we still store the // result, but we don't increment the reference count, since @@ -641,7 +552,7 @@ invert_compose(const TransformState *other) const { if (result != (const TransformState *)this) { // See the comments below about the need to up the reference // count only when the result is not the same as this. - result->ref(); + result->cache_ref(); } } // Here's the cache! @@ -665,7 +576,7 @@ invert_compose(const TransformState *other) const { // explicitly increment the reference count. We have to be sure // to decrement it again later, when the composition entry is // removed from the cache. - result->ref(); + result->cache_ref(); // (If the result was just this again, we still store the // result, but we don't increment the reference count, since @@ -675,6 +586,54 @@ invert_compose(const TransformState *other) const { return result; } +//////////////////////////////////////////////////////////////////// +// Function: TransformState::unref +// Access: Published +// Description: This method overrides ReferenceCount::unref() to +// check whether the remaining reference count is +// entirely in the cache, and if so, it checks for and +// breaks a cycle in the cache involving this object. +// This is designed to prevent leaks from cyclical +// references within the cache. +// +// Note that this is not a virtual method, and cannot be +// because ReferenceCount itself declares no virtual +// methods (it avoids the overhead of a virtual function +// pointer). But this doesn't matter, because +// PT(TransformState) is a template class, and will call +// the appropriate method even though it is non-virtual. +//////////////////////////////////////////////////////////////////// +int TransformState:: +unref() const { + if (get_cache_ref_count() > 0 && + get_ref_count() == get_cache_ref_count() + 1) { + // If we are about to remove the one reference that is not in the + // cache, leaving only references in the cache, then we need to + // check for a cycle involving this TransformState and break it if + // it exists. + + if (auto_break_cycles) { + // There might be a tiny race condition if multiple different + // threads perform cycle detects on related nodes at the same + // time. But the cost of failing the race condition is low--we + // end up with a tiny leak that may eventually be discovered, big + // deal. + ++_last_cycle_detect; + if (r_detect_cycles(this, this, 1, _last_cycle_detect, NULL)) { + // Ok, we have a cycle. This will be a leak unless we break the + // cycle by freeing the cache on this object. + if (pgraph_cat.is_debug()) { + pgraph_cat.debug() + << "Breaking cycle involving " << (*this) << "\n"; + } + ((TransformState *)this)->remove_cache_pointers(); + } + } + } + + return ReferenceCount::unref(); +} + //////////////////////////////////////////////////////////////////// // Function: TransformState::output // Access: Published, Virtual @@ -790,7 +749,8 @@ get_num_unused_states() { } // First, we need to count the number of times each TransformState - // object is recorded in the cache. + // object is recorded in the cache. We could just trust + // get_cache_ref_count(), but we'll be extra cautious for now. typedef pmap StateCount; StateCount state_count; @@ -838,6 +798,7 @@ get_num_unused_states() { for (sci = state_count.begin(); sci != state_count.end(); ++sci) { const TransformState *state = (*sci).first; int count = (*sci).second; + nassertr(count == state->get_cache_ref_count(), num_unused); nassertr(count <= state->get_ref_count(), num_unused); if (count == state->get_ref_count()) { num_unused++; @@ -867,14 +828,9 @@ get_num_unused_states() { // eliminate TransformState objects that are still in // use. // -// Normally, TransformState objects will remove -// themselves from the interal map when their reference -// counts go to 0, but since circular references are -// possible there may be some cycles that cannot remove -// themselves. Calling this function from time to time -// will ensure there is no wasteful memory leakage, but -// calling it too often may result in decreased -// performance as the cache is forced to be recomputed. +// Nowadays, this method should not be necessary, as +// reference-count cycles in the composition cache +// should be automatically detected and broken. // // The return value is the number of TransformStates // freed by this operation. @@ -885,6 +841,7 @@ clear_cache() { return 0; } + PStatTimer timer(_cache_update_pcollector); int orig_size = _states->size(); // First, we need to copy the entire set of states to a temporary @@ -912,7 +869,7 @@ clear_cache() { ++ci) { const TransformState *result = (*ci).second._result; if (result != (const TransformState *)NULL && result != state) { - result->unref(); + result->cache_unref(); nassertr(result->get_ref_count() > 0, 0); } } @@ -923,7 +880,7 @@ clear_cache() { ++ci) { const TransformState *result = (*ci).second._result; if (result != (const TransformState *)NULL && result != state) { - result->unref(); + result->cache_unref(); nassertr(result->get_ref_count() > 0, 0); } } @@ -946,11 +903,11 @@ clear_cache() { // cache and reports them to standard output. // // These cycles may be inadvertently created when state -// compositions cycle back to a starting point. In the -// current implementation, they are not automatically -// detected and broken, so they represent a kind of -// memory leak. They will not be freed unless -// clear_cache() is called explicitly. +// compositions cycle back to a starting point. +// Nowadays, these cycles should be automatically +// detected and broken, so this method should never list +// any cycles unless there is a bug in that detection +// logic. // // The cycles listed here are not leaks in the strictest // sense of the word, since they can be reclaimed by a @@ -959,6 +916,7 @@ clear_cache() { //////////////////////////////////////////////////////////////////// void TransformState:: list_cycles(ostream &out) { + typedef pset VisitedStates; VisitedStates visited; CompositionCycleDesc cycle_desc; @@ -968,8 +926,8 @@ list_cycles(ostream &out) { bool inserted = visited.insert(state).second; if (inserted) { - VisitedStates visited_this_cycle; - if (r_detect_cycles(state, state, 1, visited_this_cycle, cycle_desc)) { + ++_last_cycle_detect; + if (r_detect_cycles(state, state, 1, _last_cycle_detect, &cycle_desc)) { // This state begins a cycle. CompositionCycleDesc::reverse_iterator csi; @@ -1240,18 +1198,17 @@ do_invert_compose(const TransformState *other) const { // Description: Detects whether there is a cycle in the cache that // begins with the indicated state. Returns true if at // least one cycle is found, false if this state is not -// part of any cycles. If a cycle is found, cycle_desc -// is filled in with the list of the steps of the cycle, -// in reverse order. +// part of any cycles. If a cycle is found and +// cycle_desc is not NULL, then cycle_desc is filled in +// with the list of the steps of the cycle, in reverse +// order. //////////////////////////////////////////////////////////////////// bool TransformState:: r_detect_cycles(const TransformState *start_state, const TransformState *current_state, - int length, - TransformState::VisitedStates &visited_this_cycle, - TransformState::CompositionCycleDesc &cycle_desc) { - bool inserted = visited_this_cycle.insert(current_state).second; - if (!inserted) { + int length, UpdateSeq this_seq, + TransformState::CompositionCycleDesc *cycle_desc) { + if (current_state->_cycle_detect == this_seq) { // We've already seen this state; therefore, we've found a cycle. // However, we only care about cycles that return to the starting @@ -1260,6 +1217,7 @@ r_detect_cycles(const TransformState *start_state, // problem there. return (current_state == start_state && length > 2); } + ((TransformState *)current_state)->_cycle_detect = this_seq; CompositionCache::const_iterator ci; for (ci = current_state->_composition_cache.begin(); @@ -1268,10 +1226,12 @@ r_detect_cycles(const TransformState *start_state, const TransformState *result = (*ci).second._result; if (result != (const TransformState *)NULL) { if (r_detect_cycles(start_state, result, length + 1, - visited_this_cycle, cycle_desc)) { + this_seq, cycle_desc)) { // Cycle detected. - CompositionCycleDescEntry entry((*ci).first, result, false); - cycle_desc.push_back(entry); + if (cycle_desc != (CompositionCycleDesc *)NULL) { + CompositionCycleDescEntry entry((*ci).first, result, false); + cycle_desc->push_back(entry); + } return true; } } @@ -1283,10 +1243,12 @@ r_detect_cycles(const TransformState *start_state, const TransformState *result = (*ci).second._result; if (result != (const TransformState *)NULL) { if (r_detect_cycles(start_state, result, length + 1, - visited_this_cycle, cycle_desc)) { + this_seq, cycle_desc)) { // Cycle detected. - CompositionCycleDescEntry entry((*ci).first, result, true); - cycle_desc.push_back(entry); + if (cycle_desc != (CompositionCycleDesc *)NULL) { + CompositionCycleDescEntry entry((*ci).first, result, true); + cycle_desc->push_back(entry); + } return true; } } @@ -1296,6 +1258,115 @@ r_detect_cycles(const TransformState *start_state, return false; } +//////////////////////////////////////////////////////////////////// +// Function: TransformState::remove_cache_pointers +// Access: Private +// Description: Remove all pointers within the cache from and to this +// particular TransformState. The pointers to this +// object may be scattered around in the various +// CompositionCaches from other TransformState objects. +//////////////////////////////////////////////////////////////////// +void TransformState:: +remove_cache_pointers() { + // Fortunately, since we added CompositionCache records in pairs, we + // know exactly the set of TransformState objects that have us in their + // cache: it's the same set of TransformState objects that we have in + // our own cache. + + // We do need to put considerable thought into this loop, because as + // we clear out cache entries we'll cause other TransformState + // objects to destruct, which could cause things to get pulled out + // of our own _composition_cache map. We want to allow this (so + // that we don't encounter any just-destructed pointers in our + // cache), but we don't want to get bitten by this cascading effect. + // Instead of walking through the map from beginning to end, + // therefore, we just pull out the first one each time, and erase + // it. + +#ifdef DO_PSTATS + if (_composition_cache.empty() && _invert_composition_cache.empty()) { + return; + } + PStatTimer timer(_cache_update_pcollector); +#endif // DO_PSTATS + + // There are lots of ways to do this loop wrong. Be very careful if + // you need to modify it for any reason. + while (!_composition_cache.empty()) { + CompositionCache::iterator ci = _composition_cache.begin(); + + // It is possible that the "other" TransformState object is + // currently within its own destructor. We therefore can't use a + // PT() to hold its pointer; that could end up calling its + // destructor twice. Fortunately, we don't need to hold its + // reference count to ensure it doesn't destruct while we process + // this loop; as long as we ensure that no *other* TransformState + // objects destruct, there will be no reason for that one to. + TransformState *other = (TransformState *)(*ci).first; + + // We hold a copy of the composition result so we can dereference + // it later. + Composition comp = (*ci).second; + + // Now we can remove the element from our cache. We do this now, + // rather than later, before any other TransformState objects have + // had a chance to destruct, so we are confident that our iterator + // is still valid. + _composition_cache.erase(ci); + + if (other != this) { + CompositionCache::iterator oci = other->_composition_cache.find(this); + + // We may or may not still be listed in the other's cache (it + // might be halfway through pulling entries out, from within its + // own destructor). + if (oci != other->_composition_cache.end()) { + // Hold a copy of the other composition result, too. + Composition ocomp = (*oci).second; + + other->_composition_cache.erase(oci); + + // It's finally safe to let our held pointers go away. This may + // have cascading effects as other TransformState objects are + // destructed, but there will be no harm done if they destruct + // now. + if (ocomp._result != (const TransformState *)NULL && ocomp._result != other) { + cache_unref_delete(ocomp._result); + } + } + } + + // It's finally safe to let our held pointers go away. (See + // comment above.) + if (comp._result != (const TransformState *)NULL && comp._result != this) { + cache_unref_delete(comp._result); + } + } + + // A similar bit of code for the invert cache. + while (!_invert_composition_cache.empty()) { + CompositionCache::iterator ci = _invert_composition_cache.begin(); + TransformState *other = (TransformState *)(*ci).first; + nassertv(other != this); + Composition comp = (*ci).second; + _invert_composition_cache.erase(ci); + if (other != this) { + CompositionCache::iterator oci = + other->_invert_composition_cache.find(this); + if (oci != other->_invert_composition_cache.end()) { + Composition ocomp = (*oci).second; + other->_invert_composition_cache.erase(oci); + if (ocomp._result != (const TransformState *)NULL && ocomp._result != other) { + cache_unref_delete(ocomp._result); + } + } + } + if (comp._result != (const TransformState *)NULL && comp._result != this) { + cache_unref_delete(comp._result); + } + } +} + //////////////////////////////////////////////////////////////////// // Function: TransformState::calc_singular // Access: Private diff --git a/panda/src/pgraph/transformState.h b/panda/src/pgraph/transformState.h index 1c49acce86..2c3c432fbf 100644 --- a/panda/src/pgraph/transformState.h +++ b/panda/src/pgraph/transformState.h @@ -21,11 +21,13 @@ #include "pandabase.h" -#include "typedWritableReferenceCount.h" +#include "cachedTypedWritableReferenceCount.h" #include "pointerTo.h" #include "luse.h" #include "pset.h" #include "event.h" +#include "updateSeq.h" +#include "pStatCollector.h" class GraphicsStateGuardianBase; class FactoryParams; @@ -52,7 +54,7 @@ class FactoryParams; // instead of modifying a TransformState object, create a // new one. //////////////////////////////////////////////////////////////////// -class EXPCL_PANDA TransformState : public TypedWritableReferenceCount { +class EXPCL_PANDA TransformState : public CachedTypedWritableReferenceCount { protected: TransformState(); @@ -125,6 +127,8 @@ PUBLISHED: CPT(TransformState) compose(const TransformState *other) const; CPT(TransformState) invert_compose(const TransformState *other) const; + int unref() const; + void output(ostream &out) const; void write(ostream &out, int indent_level) const; @@ -147,16 +151,16 @@ private: bool _inverted; }; typedef pvector CompositionCycleDesc; - typedef pset VisitedStates; static CPT(TransformState) return_new(TransformState *state); CPT(TransformState) do_compose(const TransformState *other) const; CPT(TransformState) do_invert_compose(const TransformState *other) const; static bool r_detect_cycles(const TransformState *start_state, const TransformState *current_state, - int length, - VisitedStates &visited_this_cycle, - CompositionCycleDesc &cycle_desc); + int length, UpdateSeq this_seq, + CompositionCycleDesc *cycle_desc); + + void remove_cache_pointers(); private: typedef phash_set > States; @@ -191,6 +195,12 @@ private: CompositionCache _composition_cache; CompositionCache _invert_composition_cache; + // This is used to mark nodes as we visit them to detect cycles. + UpdateSeq _cycle_detect; + static UpdateSeq _last_cycle_detect; + + static PStatCollector _cache_update_pcollector; + private: // This is the actual data within the TransformState. INLINE void check_singular() const; @@ -248,9 +258,9 @@ public: return _type_handle; } static void init_type() { - TypedWritableReferenceCount::init_type(); + CachedTypedWritableReferenceCount::init_type(); register_type(_type_handle, "TransformState", - TypedWritableReferenceCount::get_class_type()); + CachedTypedWritableReferenceCount::get_class_type()); } virtual TypeHandle get_type() const { return get_class_type(); diff --git a/panda/src/putil/Sources.pp b/panda/src/putil/Sources.pp index aa76e3faba..82ee7cfd2c 100644 --- a/panda/src/putil/Sources.pp +++ b/panda/src/putil/Sources.pp @@ -13,6 +13,7 @@ bitMask.h \ buttonHandle.I \ buttonHandle.h buttonRegistry.I buttonRegistry.h \ + cachedTypedWritableReferenceCount.h cachedTypedWritableReferenceCount.I \ collideMask.h \ portalMask.h \ compareTo.I compareTo.h \ @@ -55,6 +56,7 @@ #define INCLUDED_SOURCES \ bamReader.cxx bamReaderParam.cxx bamWriter.cxx bitMask.cxx \ buttonHandle.cxx buttonRegistry.cxx \ + cachedTypedWritableReferenceCount.cxx \ config_util.cxx configurable.cxx \ cycleData.cxx \ cycleDataReader.cxx \ @@ -85,7 +87,9 @@ bam.h bamReader.I bamReader.h bamReaderParam.I bamReaderParam.h \ bamWriter.I bamWriter.h bitMask.I bitMask.h \ buttonHandle.I buttonHandle.h buttonRegistry.I \ - buttonRegistry.h collideMask.h portalMask.h \ + buttonRegistry.h \ + cachedTypedWritableReferenceCount.h cachedTypedWritableReferenceCount.I \ + collideMask.h portalMask.h \ compareTo.I compareTo.h \ config_util.h configurable.h factory.I factory.h \ cycleData.h cycleData.I \ diff --git a/panda/src/putil/bamReader.cxx b/panda/src/putil/bamReader.cxx index 4f92530bfb..bcbea131ca 100644 --- a/panda/src/putil/bamReader.cxx +++ b/panda/src/putil/bamReader.cxx @@ -420,9 +420,11 @@ read_handle(DatagramIterator &scan) { TypeRegistry::ptr()->record_derivation(type, parent_type); } else { if (type.get_parent_towards(parent_type) != parent_type) { - bam_cat.warning() - << "Bam file indicates a derivation of " << type - << " from " << parent_type << " which is no longer true.\n"; + if (bam_cat.is_debug()) { + bam_cat.debug() + << "Bam file indicates a derivation of " << type + << " from " << parent_type << " which is no longer true.\n"; + } } } } diff --git a/panda/src/putil/cachedTypedWritableReferenceCount.I b/panda/src/putil/cachedTypedWritableReferenceCount.I new file mode 100644 index 0000000000..2e72403387 --- /dev/null +++ b/panda/src/putil/cachedTypedWritableReferenceCount.I @@ -0,0 +1,295 @@ +// Filename: cachedTypedWritableReferenceCount.I +// Created by: drose (25Jan05) +// +//////////////////////////////////////////////////////////////////// +// +// PANDA 3D SOFTWARE +// Copyright (c) 2001 - 2004, Disney Enterprises, Inc. All rights reserved +// +// All use of this software is subject to the terms of the Panda 3d +// Software license. You should have received a copy of this license +// along with this source code; you will also find a current copy of +// the license at http://etc.cmu.edu/panda3d/docs/license/ . +// +// To contact the maintainers of this program write to +// panda3d-general@lists.sourceforge.net . +// +//////////////////////////////////////////////////////////////////// + + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::Constructor +// Access: Protected +// Description: The ReferenceCount constructor is protected because +// you almost never want to create just a ReferenceCount +// object by itself, and it's probably a mistake if you +// try. +// +// ReferenceCount doesn't store any useful information +// in its own right; its only purpose is to add +// reference-counting to some other class via +// inheritance. +//////////////////////////////////////////////////////////////////// +INLINE CachedTypedWritableReferenceCount:: +CachedTypedWritableReferenceCount() { + _cache_ref_count = 0; +} + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::Copy Constructor +// Access: Protected +// Description: The copies of reference-counted objects do not +// themselves inherit the reference count! +// +// This copy constructor is protected because you almost +// never want to create just a ReferenceCount object by +// itself, and it's probably a mistake if you try. +//////////////////////////////////////////////////////////////////// +INLINE CachedTypedWritableReferenceCount:: +CachedTypedWritableReferenceCount(const CachedTypedWritableReferenceCount ©) : TypedWritableReferenceCount(copy) { + _cache_ref_count = 0; +} + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::Copy Assignment Operator +// Access: Protected +// Description: The copies of reference-counted objects do not +// themselves inherit the reference count! +// +// This copy assignment operator is protected because +// you almost never want to copy just a ReferenceCount +// object by itself, and it's probably a mistake if you +// try. Instead, this should only be called from a +// derived class that implements this operator and then +// calls up the inheritance chain. +//////////////////////////////////////////////////////////////////// +INLINE void CachedTypedWritableReferenceCount:: +operator = (const CachedTypedWritableReferenceCount ©) { + nassertv(this != NULL); + + // If this assertion fails, our own pointer was recently deleted. + // Possibly you used a real pointer instead of a PointerTo at some + // point, and the object was deleted when the PointerTo went out of + // scope. Maybe you tried to create an automatic (local variable) + // instance of a class that derives from ReferenceCount. Or maybe + // your headers are out of sync, and you need to make clean in + // direct or some higher tree. + nassertv(_cache_ref_count != -100); + + TypedWritableReferenceCount::operator = (copy); +} + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::Destructor +// Access: Protected +// Description: The ReferenceCount destructor is protected to +// discourage users from accidentally trying to delete a +// ReferenceCount pointer directly. This is almost +// always a bad idea, since the destructor is not +// virtual, and you've almost certainly got some pointer +// to something that inherits from ReferenceCount, not +// just a plain old ReferenceCount object. +//////////////////////////////////////////////////////////////////// +INLINE CachedTypedWritableReferenceCount:: +~CachedTypedWritableReferenceCount() { + nassertv(this != NULL); + + // If this assertion fails, we're trying to delete an object that + // was just deleted. Possibly you used a real pointer instead of a + // PointerTo at some point, and the object was deleted when the + // PointerTo went out of scope. Maybe you tried to create an + // automatic (local variable) instance of a class that derives from + // ReferenceCount. Or maybe your headers are out of sync, and you + // need to make clean in direct or some higher tree. + nassertv(_cache_ref_count != -100); + + // If this assertion fails, the reference counts are all screwed + // up altogether. Maybe some errant code stomped all over memory + // somewhere. + nassertv(_cache_ref_count >= 0); + + // If this assertion fails, someone tried to delete this object + // while its reference count was still positive. Maybe you tried + // to point a PointerTo at a static object (a local variable, + // instead of one allocated via new)? The test below against 0x7f + // is supposed to check for that, but it's a pretty hokey test. + + // Another possibility is you inadvertently omitted a copy + // constructor for a ReferenceCount object, and then bitwise + // copied a dynamically allocated value--reference count and + // all--onto a locally allocated one. + nassertv(_cache_ref_count == 0); + +#ifndef NDEBUG + // Ok, all clear to delete. Now set the reference count to -100, + // so we'll have a better chance of noticing if we happen to have + // a stray pointer to it still out there. + _cache_ref_count = -100; +#endif +} + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::get_cache_ref_count +// Access: Public +// Description: Returns the current reference count. +//////////////////////////////////////////////////////////////////// +INLINE int CachedTypedWritableReferenceCount:: +get_cache_ref_count() const { +#ifndef NDEBUG + test_cache_ref_count_integrity(); +#endif + return _cache_ref_count; +} + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::cache_ref +// Access: Public +// Description: Explicitly increments the reference count. +// +// This function is const, even though it changes the +// object, because generally fiddling with an object's +// reference count isn't considered part of fiddling +// with the object. An object might be const in other +// ways, but we still need to accurately count the +// number of references to it. +// +// The return value is the new reference count. +//////////////////////////////////////////////////////////////////// +INLINE int CachedTypedWritableReferenceCount:: +cache_ref() const { + nassertr(this != NULL, 0); + + // If this assertion fails, we're trying to delete an object that + // was just deleted. Possibly you used a real pointer instead of a + // PointerTo at some point, and the object was deleted when the + // PointerTo went out of scope. Maybe you tried to create an + // automatic (local variable) instance of a class that derives from + // ReferenceCount. Or maybe your headers are out of sync, and you + // need to make clean in direct or some higher tree. + nassertr(_cache_ref_count != -100, 0); + + // If this assertion fails, the reference counts are all screwed + // up altogether. Maybe some errant code stomped all over memory + // somewhere. + nassertr(_cache_ref_count >= 0, 0); + + ref(); + return AtomicAdjust::inc(((CachedTypedWritableReferenceCount *)this)->_cache_ref_count); +} + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::cache_unref +// Access: Public +// Description: Explicitly decrements the reference count. Note that +// the object will not be implicitly deleted by unref() +// simply because the reference count drops to zero. +// (Having a member function delete itself is +// problematic; plus, we don't have a virtual destructor +// anyway.) However, see the helper function +// unref_delete(). +// +// User code should avoid using ref() and unref() +// directly, which can result in missed reference +// counts. Instead, let a PointerTo object manage the +// reference counting automatically. +// +// This function is const, even though it changes the +// object, because generally fiddling with an object's +// reference count isn't considered part of fiddling +// with the object. An object might be const in other +// ways, but we still need to accurately count the +// number of references to it. +// +// The return value is the new reference count. +//////////////////////////////////////////////////////////////////// +INLINE int CachedTypedWritableReferenceCount:: +cache_unref() const { + nassertr(this != NULL, false); + + // If this assertion fails, we're trying to delete an object that + // was just deleted. Possibly you used a real pointer instead of a + // PointerTo at some point, and the object was deleted when the + // PointerTo went out of scope. Maybe you tried to create an + // automatic (local variable) instance of a class that derives from + // ReferenceCount. Or maybe your headers are out of sync, and you + // need to make clean in direct or some higher tree. + nassertr(_cache_ref_count != -100, false); + + // If this assertion fails, the reference counts are all screwed + // up altogether. Maybe some errant code stomped all over memory + // somewhere. + nassertr(_cache_ref_count >= 0, false); + + // If this assertion fails, you tried to unref an object with a + // zero reference count. Are you using ref() and unref() + // directly? Are you sure you can't use PointerTo's? + nassertr(_cache_ref_count > 0, false); + + unref(); + return AtomicAdjust::dec(((CachedTypedWritableReferenceCount *)this)->_cache_ref_count); +} + + +//////////////////////////////////////////////////////////////////// +// Function: CachedTypedWritableReferenceCount::test_cache_ref_count_integrity +// Access: Public +// Description: Does some easy checks to make sure that the reference +// count isn't completely bogus. +//////////////////////////////////////////////////////////////////// +INLINE void CachedTypedWritableReferenceCount:: +test_cache_ref_count_integrity() const { +#ifndef NDEBUG + nassertv(this != NULL); + + // If this assertion fails, we're trying to delete an object that + // was just deleted. Possibly you used a real pointer instead of a + // PointerTo at some point, and the object was deleted when the + // PointerTo went out of scope. Maybe you tried to create an + // automatic (local variable) instance of a class that derives from + // ReferenceCount. Or maybe your headers are out of sync, and you + // need to make clean in direct or some higher tree. + nassertv(_cache_ref_count != -100); + + // If this assertion fails, the reference counts are all screwed + // up altogether. Maybe some errant code stomped all over memory + // somewhere. + nassertv(_cache_ref_count >= 0); + + test_ref_count_integrity(); +#endif +} + +//////////////////////////////////////////////////////////////////// +// Function: cache_unref_delete +// Description: This global helper function will unref the given +// ReferenceCount object, and if the reference count +// reaches zero, automatically delete it. It can't be a +// member function because it's usually a bad idea to +// delete an object from within its own member function. +// It's a template function so the destructor doesn't +// have to be virtual. +//////////////////////////////////////////////////////////////////// +template +INLINE void +cache_unref_delete(RefCountType *ptr) { + ptr->cache_unref(); + if (ptr->get_ref_count() == 0) { +#ifndef NDEBUG + if (get_leak_memory()) { + // In leak-memory mode, we don't actually delete the pointer, + // although we do call the destructor explicitly. This has + // exactly the same effect as deleting it, without actually + // freeing up the memory it uses. + + // Furthermore, if we have never-destruct set, we don't even + // call the destructor. + if (!get_never_destruct()) { + ptr->~RefCountType(); + } + return; + } +#endif + delete ptr; + } +} + diff --git a/panda/src/putil/cachedTypedWritableReferenceCount.cxx b/panda/src/putil/cachedTypedWritableReferenceCount.cxx new file mode 100644 index 0000000000..2672f3fe84 --- /dev/null +++ b/panda/src/putil/cachedTypedWritableReferenceCount.cxx @@ -0,0 +1,21 @@ +// Filename: cachedTypedWritableReferenceCount.cxx +// Created by: drose (25Jan05) +// +//////////////////////////////////////////////////////////////////// +// +// PANDA 3D SOFTWARE +// Copyright (c) 2001 - 2004, Disney Enterprises, Inc. All rights reserved +// +// All use of this software is subject to the terms of the Panda 3d +// Software license. You should have received a copy of this license +// along with this source code; you will also find a current copy of +// the license at http://etc.cmu.edu/panda3d/docs/license/ . +// +// To contact the maintainers of this program write to +// panda3d-general@lists.sourceforge.net . +// +//////////////////////////////////////////////////////////////////// + +#include "cachedTypedWritableReferenceCount.h" + +TypeHandle CachedTypedWritableReferenceCount::_type_handle; diff --git a/panda/src/putil/cachedTypedWritableReferenceCount.h b/panda/src/putil/cachedTypedWritableReferenceCount.h new file mode 100644 index 0000000000..7e7db7612d --- /dev/null +++ b/panda/src/putil/cachedTypedWritableReferenceCount.h @@ -0,0 +1,80 @@ +// Filename: cachedTypedWritableReferenceCount.h +// Created by: drose (25Jan05) +// +//////////////////////////////////////////////////////////////////// +// +// PANDA 3D SOFTWARE +// Copyright (c) 2001 - 2004, Disney Enterprises, Inc. All rights reserved +// +// All use of this software is subject to the terms of the Panda 3d +// Software license. You should have received a copy of this license +// along with this source code; you will also find a current copy of +// the license at http://etc.cmu.edu/panda3d/docs/license/ . +// +// To contact the maintainers of this program write to +// panda3d-general@lists.sourceforge.net . +// +//////////////////////////////////////////////////////////////////// + +#ifndef CACHEDTYPEDWRITABLEREFERENCECOUNT_H +#define CACHEDTYPEDWRITABLEREFERENCECOUNT_H + +#include "pandabase.h" + +#include "typedWritableReferenceCount.h" + +//////////////////////////////////////////////////////////////////// +// Class : CachedTypedWritableReferenceCount +// Description : This is a special extension to ReferenceCount that +// includes dual reference counts: the standard +// reference count number, which includes all references +// to the object, and a separate number (the cache +// reference count) that counts the number of references +// to the object just within its cache alone. When +// get_ref_count() == get_cache_ref_count(), the object +// is not referenced outside the cache. +// +// The cache refs must be explicitly maintained; there +// is no PointerTo<> class to maintain the cache +// reference counts automatically. The cache reference +// count is automatically included in the overall +// reference count: calling cache_ref() and +// cache_unref() automatically calls ref() and unref(). +//////////////////////////////////////////////////////////////////// +class EXPCL_PANDA CachedTypedWritableReferenceCount : public TypedWritableReferenceCount { +protected: + INLINE CachedTypedWritableReferenceCount(); + INLINE CachedTypedWritableReferenceCount(const CachedTypedWritableReferenceCount ©); + INLINE void operator = (const CachedTypedWritableReferenceCount ©); + INLINE ~CachedTypedWritableReferenceCount(); + +PUBLISHED: + INLINE int get_cache_ref_count() const; + INLINE int cache_ref() const; + INLINE int cache_unref() const; + INLINE void test_cache_ref_count_integrity() const; + +private: + int _cache_ref_count; + +public: + static TypeHandle get_class_type() { + return _type_handle; + } + + static void init_type() { + TypedWritableReferenceCount::init_type(); + register_type(_type_handle, "CachedTypedWritableReferenceCount", + TypedWritableReferenceCount::get_class_type()); + } + +private: + static TypeHandle _type_handle; +}; + +template +INLINE void cache_unref_delete(RefCountType *ptr); + +#include "cachedTypedWritableReferenceCount.I" + +#endif diff --git a/panda/src/putil/config_util.cxx b/panda/src/putil/config_util.cxx index 492e98fbef..c9556d7f1e 100644 --- a/panda/src/putil/config_util.cxx +++ b/panda/src/putil/config_util.cxx @@ -18,6 +18,7 @@ #include "config_util.h" #include "bamReaderParam.h" +#include "cachedTypedWritableReferenceCount.h" #include "clockObject.h" #include "configurable.h" #include "datagram.h" @@ -57,6 +58,7 @@ ConfigVariableSearchPath sound_path ConfigureFn(config_util) { BamReaderParam::init_type(); + CachedTypedWritableReferenceCount::init_type(); Configurable::init_type(); Datagram::init_type(); FactoryParam::init_type(); diff --git a/panda/src/putil/putil_composite1.cxx b/panda/src/putil/putil_composite1.cxx index c9e585db24..a0d2c1563c 100644 --- a/panda/src/putil/putil_composite1.cxx +++ b/panda/src/putil/putil_composite1.cxx @@ -4,6 +4,7 @@ #include "bitMask.cxx" #include "buttonHandle.cxx" #include "buttonRegistry.cxx" +#include "cachedTypedWritableReferenceCount.cxx" #include "config_util.cxx" #include "configurable.cxx" #include "cycleData.cxx" diff --git a/panda/src/text/textNode.cxx b/panda/src/text/textNode.cxx index 5f5deb16e2..6d7bdbd349 100644 --- a/panda/src/text/textNode.cxx +++ b/panda/src/text/textNode.cxx @@ -139,6 +139,24 @@ calc_width(int character) const { return _assembler.calc_width(character, *this); } +//////////////////////////////////////////////////////////////////// +// Function: TextNode::output +// Access: Public, Virtual +// Description: +//////////////////////////////////////////////////////////////////// +void TextNode:: +output(ostream &out) const { + PandaNode::output(out); + + check_rebuild(); + int geom_count = 0; + if (_internal_geom != (PandaNode *)NULL) { + geom_count = count_geoms(_internal_geom); + } + + out << " (" << geom_count << " geoms)"; +} + //////////////////////////////////////////////////////////////////// // Function: TextNode::write // Access: Published, Virtual @@ -182,7 +200,8 @@ PT(PandaNode) TextNode:: generate() { if (text_cat.is_debug()) { text_cat.debug() - << "Rebuilding " << *this << " with '" << get_text() << "'\n"; + << "Rebuilding " << get_type() << " " << get_name() + << " with '" << get_text() << "'\n"; } // The strategy here will be to assemble together a bunch of @@ -774,3 +793,27 @@ make_card_with_border() { return card_geode.p(); } + +//////////////////////////////////////////////////////////////////// +// Function: TextNode::count_geoms +// Access: Private, Static +// Description: Recursively counts the number of Geoms at the +// indicated node and below. Strictly for reporting +// this count on output. +//////////////////////////////////////////////////////////////////// +int TextNode:: +count_geoms(PandaNode *node) { + int num_geoms = 0; + + if (node->is_geom_node()) { + GeomNode *geom_node = DCAST(GeomNode, node); + num_geoms += geom_node->get_num_geoms(); + } + + Children children = node->get_children(); + for (int i = 0; i < children.get_num_children(); ++i) { + num_geoms += count_geoms(children.get_child(i)); + } + + return num_geoms; +} diff --git a/panda/src/text/textNode.h b/panda/src/text/textNode.h index 49bb511abd..cda98cc714 100644 --- a/panda/src/text/textNode.h +++ b/panda/src/text/textNode.h @@ -187,6 +187,7 @@ PUBLISHED: float calc_width(int character) const; INLINE float calc_width(const string &line) const; + virtual void output(ostream &out) const; virtual void write(ostream &out, int indent_level = 0) const; // The following functions return information about the text that @@ -243,6 +244,8 @@ private: PT(PandaNode) make_card(); PT(PandaNode) make_card_with_border(); + static int count_geoms(PandaNode *node); + PT(PandaNode) _internal_geom; PT(Texture) _card_texture;