From cf44a95d843b85568cbb99df0cd260e22a7c7907 Mon Sep 17 00:00:00 2001 From: David Rose Date: Mon, 13 Sep 2004 16:29:57 +0000 Subject: [PATCH] fix leak due to cyclic RenderState reference counts --- panda/src/pgraph/colorScaleAttrib.cxx | 12 + panda/src/pgraph/config_pgraph.cxx | 6 + panda/src/pgraph/config_pgraph.h | 1 + panda/src/pgraph/renderAttrib.I | 31 +- panda/src/pgraph/renderAttrib.cxx | 70 +++++ panda/src/pgraph/renderAttrib.h | 6 +- panda/src/pgraph/renderEffect.I | 2 +- panda/src/pgraph/renderEffect.cxx | 94 +++++- panda/src/pgraph/renderEffect.h | 10 +- panda/src/pgraph/renderEffects.cxx | 134 ++++++-- panda/src/pgraph/renderEffects.h | 10 +- panda/src/pgraph/renderState.I | 283 +++++++++-------- panda/src/pgraph/renderState.cxx | 413 +++++++++++++++++++------ panda/src/pgraph/renderState.h | 52 +++- panda/src/pgraph/transformState.I | 81 +++-- panda/src/pgraph/transformState.cxx | 427 ++++++++++++++++++++------ panda/src/pgraph/transformState.h | 46 ++- 17 files changed, 1277 insertions(+), 401 deletions(-) diff --git a/panda/src/pgraph/colorScaleAttrib.cxx b/panda/src/pgraph/colorScaleAttrib.cxx index ab6dde81ec..65de44c552 100644 --- a/panda/src/pgraph/colorScaleAttrib.cxx +++ b/panda/src/pgraph/colorScaleAttrib.cxx @@ -118,9 +118,21 @@ compare_to_impl(const RenderAttrib *other) const { DCAST_INTO_R(ta, other, 0); if (is_off() != ta->is_off()) { + if (pgraph_cat.is_spam()) { + pgraph_cat.spam() + << "Comparing " << (int)is_off() << " to " << (int)ta->is_off() << " result = " + << (int)is_off() - (int)ta->is_off() << "\n"; + } + return (int)is_off() - (int)ta->is_off(); } + if (pgraph_cat.is_spam()) { + pgraph_cat.spam() + << "Comparing " << _scale << " to " << ta->_scale << " result = " + << _scale.compare_to(ta->_scale) << "\n"; + } + return _scale.compare_to(ta->_scale); } diff --git a/panda/src/pgraph/config_pgraph.cxx b/panda/src/pgraph/config_pgraph.cxx index 8206bef13b..cb55858b0c 100644 --- a/panda/src/pgraph/config_pgraph.cxx +++ b/panda/src/pgraph/config_pgraph.cxx @@ -127,6 +127,12 @@ const bool paranoid_compose = config_pgraph.GetBool("paranoid-compose", false); // by matrix. const bool compose_componentwise = config_pgraph.GetBool("compose-componentwise", true); +// Set this true to double-check that nothing is inappropriately +// modifying the supposedly const structures like RenderState, +// RenderAttrib, TransformState, and RenderEffect. This has no effect +// if NDEBUG is defined. +const bool paranoid_const = config_pgraph.GetBool("paranoid-const", false); + // Set this true to view some info statements regarding the polylight // It is helpful for debugging const bool polylight_info = config_pgraph.GetBool("polylight-info", false); diff --git a/panda/src/pgraph/config_pgraph.h b/panda/src/pgraph/config_pgraph.h index 2c4017d91b..1d8f8c1156 100644 --- a/panda/src/pgraph/config_pgraph.h +++ b/panda/src/pgraph/config_pgraph.h @@ -35,6 +35,7 @@ extern const bool unambiguous_graph; extern const bool allow_unrelated_wrt; extern const bool paranoid_compose; extern const bool compose_componentwise; +extern const bool paranoid_const; extern const bool polylight_info; diff --git a/panda/src/pgraph/renderAttrib.I b/panda/src/pgraph/renderAttrib.I index bb43313796..fc48ba9701 100644 --- a/panda/src/pgraph/renderAttrib.I +++ b/panda/src/pgraph/renderAttrib.I @@ -68,8 +68,22 @@ make_default() const { } //////////////////////////////////////////////////////////////////// -// Function: RenderAttrib::compare_to +// Function: RenderAttrib::always_reissue // Access: Public +// Description: Returns true if the RenderAttrib should be reissued +// to the GSG with every state change, even if it is the +// same pointer as it was before; or false for the +// normal case, to reissue only when the RenderAttrib +// pointer changes. +//////////////////////////////////////////////////////////////////// +INLINE bool RenderAttrib:: +always_reissue() const { + return _always_reissue; +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderAttrib::compare_to +// Access: Published // Description: Provides an arbitrary ordering among all unique // RenderAttribs, so we can store the essentially // different ones in a big set and throw away the rest. @@ -92,18 +106,3 @@ compare_to(const RenderAttrib &other) const { // We only call compare_to_impl() if they have the same type. return compare_to_impl(&other); } - -//////////////////////////////////////////////////////////////////// -// Function: RenderAttrib::always_reissue -// Access: Public -// Description: Returns true if the RenderAttrib should be reissued -// to the GSG with every state change, even if it is the -// same pointer as it was before; or false for the -// normal case, to reissue only when the RenderAttrib -// pointer changes. -//////////////////////////////////////////////////////////////////// -INLINE bool RenderAttrib:: -always_reissue() const { - return _always_reissue; -} - diff --git a/panda/src/pgraph/renderAttrib.cxx b/panda/src/pgraph/renderAttrib.cxx index 9abde7e05d..68a325ae18 100644 --- a/panda/src/pgraph/renderAttrib.cxx +++ b/panda/src/pgraph/renderAttrib.cxx @@ -124,6 +124,70 @@ write(ostream &out, int indent_level) const { indent(out, indent_level) << *this << "\n"; } +//////////////////////////////////////////////////////////////////// +// Function: RenderAttrib::get_num_attribs +// Access: Published, Static +// Description: Returns the total number of unique RenderAttrib +// objects allocated in the world. This will go up and +// down during normal operations. +//////////////////////////////////////////////////////////////////// +int RenderAttrib:: +get_num_attribs() { + if (_attribs == (Attribs *)NULL) { + return 0; + } + return _attribs->size(); +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderAttrib::list_attribs +// Access: Published, Static +// Description: Lists all of the RenderAttribs in the cache to the +// output stream, one per line. This can be quite a lot +// of output if the cache is large, so be prepared. +//////////////////////////////////////////////////////////////////// +void RenderAttrib:: +list_attribs(ostream &out) { + out << _attribs->size() << " attribs:\n"; + Attribs::const_iterator si; + for (si = _attribs->begin(); si != _attribs->end(); ++si) { + const RenderAttrib *attrib = (*si); + attrib->write(out, 2); + } +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderAttrib::validate_attribs +// Access: Published, Static +// Description: Ensures that the cache is still stored in sorted +// order. Returns true if so, false if there is a +// problem (which implies someone has modified one of +// the supposedly-const RenderAttrib objects). +//////////////////////////////////////////////////////////////////// +bool RenderAttrib:: +validate_attribs() { + if (_attribs->empty()) { + return true; + } + + Attribs::const_iterator si = _attribs->begin(); + Attribs::const_iterator snext = si; + ++snext; + while (snext != _attribs->end()) { + if ((*si)->compare_to(*(*snext)) >= 0) { + pgraph_cat.error() + << "RenderAttribs out of order!\n"; + (*si)->write(pgraph_cat.error(false), 2); + (*snext)->write(pgraph_cat.error(false), 2); + return false; + } + si = snext; + ++snext; + } + + return true; +} + //////////////////////////////////////////////////////////////////// // Function: RenderAttrib::return_new // Access: Protected, Static @@ -146,6 +210,12 @@ return_new(RenderAttrib *attrib) { // for anything else. nassertr(attrib->_saved_entry == _attribs->end(), attrib); +#ifndef NDEBUG + if (paranoid_const) { + nassertr(validate_attribs(), attrib); + } +#endif + // Save the attrib in a local PointerTo so that it will be freed at // the end of this function if no one else uses it. CPT(RenderAttrib) pt_attrib = attrib; diff --git a/panda/src/pgraph/renderAttrib.h b/panda/src/pgraph/renderAttrib.h index fc015cf2a1..0be3d77335 100644 --- a/panda/src/pgraph/renderAttrib.h +++ b/panda/src/pgraph/renderAttrib.h @@ -68,15 +68,19 @@ public: INLINE CPT(RenderAttrib) compose(const RenderAttrib *other) const; INLINE CPT(RenderAttrib) invert_compose(const RenderAttrib *other) const; INLINE CPT(RenderAttrib) make_default() const; - INLINE int compare_to(const RenderAttrib &other) const; virtual void issue(GraphicsStateGuardianBase *gsg) const; INLINE bool always_reissue() const; PUBLISHED: + INLINE int compare_to(const RenderAttrib &other) const; virtual void output(ostream &out) const; virtual void write(ostream &out, int indent_level) const; + static int get_num_attribs(); + static void list_attribs(ostream &out); + static bool validate_attribs(); + enum PandaCompareFunc { // intentionally defined to match D3DCMPFUNC M_none=0, // alpha-test disabled (always-draw) M_never, // Never draw. diff --git a/panda/src/pgraph/renderEffect.I b/panda/src/pgraph/renderEffect.I index 258081cb25..6e702b742f 100644 --- a/panda/src/pgraph/renderEffect.I +++ b/panda/src/pgraph/renderEffect.I @@ -19,7 +19,7 @@ //////////////////////////////////////////////////////////////////// // Function: RenderEffect::compare_to -// Access: Public +// Access: Published // Description: Provides an arbitrary ordering among all unique // RenderEffects, so we can store the essentially // different ones in a big set and throw away the rest. diff --git a/panda/src/pgraph/renderEffect.cxx b/panda/src/pgraph/renderEffect.cxx index bd4e1ddd67..b99b1eeaee 100644 --- a/panda/src/pgraph/renderEffect.cxx +++ b/panda/src/pgraph/renderEffect.cxx @@ -20,7 +20,7 @@ #include "bamReader.h" #include "indent.h" -RenderEffect::Effects RenderEffect::_effects; +RenderEffect::Effects *RenderEffect::_effects = NULL; TypeHandle RenderEffect::_type_handle; //////////////////////////////////////////////////////////////////// @@ -30,7 +30,15 @@ TypeHandle RenderEffect::_type_handle; //////////////////////////////////////////////////////////////////// RenderEffect:: RenderEffect() { - _saved_entry = _effects.end(); + if (_effects == (Effects *)NULL) { + // Make sure the global _effects map is allocated. This only has + // to be done once. We could make this map static, but then we + // run into problems if anyone creates a RenderState object at + // static init time; it also seems to cause problems when the + // Panda shared library is unloaded at application exit time. + _effects = new Effects; + } + _saved_entry = _effects->end(); } //////////////////////////////////////////////////////////////////// @@ -61,13 +69,13 @@ operator = (const RenderEffect &) { //////////////////////////////////////////////////////////////////// RenderEffect:: ~RenderEffect() { - if (_saved_entry != _effects.end()) { + if (_saved_entry != _effects->end()) { // We cannot make this assertion, because the RenderEffect has // already partially destructed--this means we cannot look up the // object in the map. In fact, the map is temporarily invalid // until we finish destructing, since we screwed up the ordering // when we changed the return value of get_type(). - // nassertv(_effects.find(this) == _saved_entry); + // nassertv(_effects->find(this) == _saved_entry); // Note: this isn't thread-safe, because once the derived class // destructor exits and before this destructor completes, the map @@ -76,8 +84,8 @@ RenderEffect:: // functionality to a separate method, that is to be called from // *each* derived class's destructor (and then we can put the // above assert back in). - _effects.erase(_saved_entry); - _saved_entry = _effects.end(); + _effects->erase(_saved_entry); + _saved_entry = _effects->end(); } } @@ -172,6 +180,70 @@ write(ostream &out, int indent_level) const { indent(out, indent_level) << *this << "\n"; } +//////////////////////////////////////////////////////////////////// +// Function: RenderEffect::get_num_effects +// Access: Published, Static +// Description: Returns the total number of unique RenderEffect +// objects allocated in the world. This will go up and +// down during normal operations. +//////////////////////////////////////////////////////////////////// +int RenderEffect:: +get_num_effects() { + if (_effects == (Effects *)NULL) { + return 0; + } + return _effects->size(); +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderEffect::list_effects +// Access: Published, Static +// Description: Lists all of the RenderEffects in the cache to the +// output stream, one per line. This can be quite a lot +// of output if the cache is large, so be prepared. +//////////////////////////////////////////////////////////////////// +void RenderEffect:: +list_effects(ostream &out) { + out << _effects->size() << " effects:\n"; + Effects::const_iterator si; + for (si = _effects->begin(); si != _effects->end(); ++si) { + const RenderEffect *effect = (*si); + effect->write(out, 2); + } +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderEffect::validate_effects +// Access: Published, Static +// Description: Ensures that the cache is still stored in sorted +// order. Returns true if so, false if there is a +// problem (which implies someone has modified one of +// the supposedly-const RenderEffect objects). +//////////////////////////////////////////////////////////////////// +bool RenderEffect:: +validate_effects() { + if (_effects->empty()) { + return true; + } + + Effects::const_iterator si = _effects->begin(); + Effects::const_iterator snext = si; + ++snext; + while (snext != _effects->end()) { + if ((*si)->compare_to(*(*snext)) >= 0) { + pgraph_cat.error() + << "RenderEffects out of order!\n"; + (*si)->write(pgraph_cat.error(false), 2); + (*snext)->write(pgraph_cat.error(false), 2); + return false; + } + si = snext; + ++snext; + } + + return true; +} + //////////////////////////////////////////////////////////////////// // Function: RenderEffect::return_new // Access: Protected, Static @@ -192,13 +264,19 @@ return_new(RenderEffect *effect) { // This should be a newly allocated pointer, not one that was used // for anything else. - nassertr(effect->_saved_entry == _effects.end(), effect); + nassertr(effect->_saved_entry == _effects->end(), effect); + +#ifndef NDEBUG + if (paranoid_const) { + nassertr(validate_effects(), effect); + } +#endif // Save the effect in a local PointerTo so that it will be freed at // the end of this function if no one else uses it. CPT(RenderEffect) pt_effect = effect; - pair result = _effects.insert(effect); + pair result = _effects->insert(effect); if (result.second) { // The effect was inserted; save the iterator and return the // input effect. diff --git a/panda/src/pgraph/renderEffect.h b/panda/src/pgraph/renderEffect.h index abf5a604d7..d59657f7ad 100644 --- a/panda/src/pgraph/renderEffect.h +++ b/panda/src/pgraph/renderEffect.h @@ -67,8 +67,6 @@ private: public: virtual ~RenderEffect(); - INLINE int compare_to(const RenderEffect &other) const; - virtual bool safe_to_transform() const; virtual bool safe_to_combine() const; virtual CPT(RenderEffect) xform(const LMatrix4f &mat) const; @@ -79,9 +77,15 @@ public: CPT(RenderState) &node_state) const; PUBLISHED: + INLINE int compare_to(const RenderEffect &other) const; + virtual void output(ostream &out) const; virtual void write(ostream &out, int indent_level) const; + static int get_num_effects(); + static void list_effects(ostream &out); + static bool validate_effects(); + protected: static CPT(RenderEffect) return_new(RenderEffect *effect); @@ -89,7 +93,7 @@ protected: private: typedef pset > Effects; - static Effects _effects; + static Effects *_effects; Effects::iterator _saved_entry; diff --git a/panda/src/pgraph/renderEffects.cxx b/panda/src/pgraph/renderEffects.cxx index 6d59415726..c121b07ce8 100644 --- a/panda/src/pgraph/renderEffects.cxx +++ b/panda/src/pgraph/renderEffects.cxx @@ -29,7 +29,7 @@ #include "indent.h" #include "compareTo.h" -RenderEffects::States RenderEffects::_states; +RenderEffects::States *RenderEffects::_states = NULL; CPT(RenderEffects) RenderEffects::_empty_state; TypeHandle RenderEffects::_type_handle; @@ -42,7 +42,15 @@ TypeHandle RenderEffects::_type_handle; //////////////////////////////////////////////////////////////////// RenderEffects:: RenderEffects() { - _saved_entry = _states.end(); + if (_states == (States *)NULL) { + // Make sure the global _states map is allocated. This only has + // to be done once. We could make this map static, but then we + // run into problems if anyone creates a RenderState object at + // static init time; it also seems to cause problems when the + // Panda shared library is unloaded at application exit time. + _states = new States; + } + _saved_entry = _states->end(); _flags = 0; } @@ -75,33 +83,12 @@ operator = (const RenderEffects &) { RenderEffects:: ~RenderEffects() { // Remove the deleted RenderEffects object from the global pool. - if (_saved_entry != _states.end()) { - _states.erase(_saved_entry); - _saved_entry = _states.end(); + if (_saved_entry != _states->end()) { + _states->erase(_saved_entry); + _saved_entry = _states->end(); } } -//////////////////////////////////////////////////////////////////// -// Function: RenderEffects::operator < -// Access: Public -// Description: Provides an arbitrary ordering among all unique -// RenderEffectss, so we can store the essentially -// different ones in a big set and throw away the rest. -// -// This method is not needed outside of the RenderEffects -// class because all equivalent RenderEffects objects are -// guaranteed to share the same pointer; thus, a pointer -// comparison is always sufficient. -//////////////////////////////////////////////////////////////////// -bool RenderEffects:: -operator < (const RenderEffects &other) const { - // We must compare all the properties of the effects, not just - // the type; thus, we compare them one at a time using compare_to(). - return lexicographical_compare(_effects.begin(), _effects.end(), - other._effects.begin(), other._effects.end(), - CompareTo()); -} - //////////////////////////////////////////////////////////////////// // Function: RenderEffects::safe_to_transform // Access: Public @@ -171,6 +158,27 @@ xform(const LMatrix4f &mat) const { return return_new(new_state); } +//////////////////////////////////////////////////////////////////// +// Function: RenderEffects::operator < +// Access: Published +// Description: Provides an arbitrary ordering among all unique +// RenderEffectss, so we can store the essentially +// different ones in a big set and throw away the rest. +// +// This method is not needed outside of the RenderEffects +// class because all equivalent RenderEffects objects are +// guaranteed to share the same pointer; thus, a pointer +// comparison is always sufficient. +//////////////////////////////////////////////////////////////////// +bool RenderEffects:: +operator < (const RenderEffects &other) const { + // We must compare all the properties of the effects, not just + // the type; thus, we compare them one at a time using compare_to(). + return lexicographical_compare(_effects.begin(), _effects.end(), + other._effects.begin(), other._effects.end(), + CompareTo()); +} + //////////////////////////////////////////////////////////////////// // Function: RenderEffects::find_effect @@ -412,6 +420,70 @@ cull_callback(CullTraverser *trav, CullTraverserData &data, } } +//////////////////////////////////////////////////////////////////// +// Function: RenderEffects::get_num_states +// Access: Published, Static +// Description: Returns the total number of unique RenderEffects +// objects allocated in the world. This will go up and +// down during normal operations. +//////////////////////////////////////////////////////////////////// +int RenderEffects:: +get_num_states() { + if (_states == (States *)NULL) { + return 0; + } + return _states->size(); +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderEffects::list_states +// Access: Published, Static +// Description: Lists all of the RenderEffectss in the cache to the +// output stream, one per line. This can be quite a lot +// of output if the cache is large, so be prepared. +//////////////////////////////////////////////////////////////////// +void RenderEffects:: +list_states(ostream &out) { + out << _states->size() << " states:\n"; + States::const_iterator si; + for (si = _states->begin(); si != _states->end(); ++si) { + const RenderEffects *state = (*si); + state->write(out, 2); + } +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderEffects::validate_states +// Access: Published, Static +// Description: Ensures that the cache is still stored in sorted +// order. Returns true if so, false if there is a +// problem (which implies someone has modified one of +// the supposedly-const RenderEffects objects). +//////////////////////////////////////////////////////////////////// +bool RenderEffects:: +validate_states() { + if (_states->empty()) { + return true; + } + + States::const_iterator si = _states->begin(); + States::const_iterator snext = si; + ++snext; + while (snext != _states->end()) { + if (!(*(*si) < *(*snext))) { + pgraph_cat.error() + << "RenderEffectss out of order!\n"; + (*si)->write(pgraph_cat.error(false), 2); + (*snext)->write(pgraph_cat.error(false), 2); + return false; + } + si = snext; + ++snext; + } + + return true; +} + //////////////////////////////////////////////////////////////////// // Function: RenderEffects::return_new // Access: Private, Static @@ -430,13 +502,19 @@ return_new(RenderEffects *state) { // This should be a newly allocated pointer, not one that was used // for anything else. - nassertr(state->_saved_entry == _states.end(), state); + nassertr(state->_saved_entry == _states->end(), state); + +#ifndef NDEBUG + if (paranoid_const) { + nassertr(validate_states(), state); + } +#endif // Save the state in a local PointerTo so that it will be freed at // the end of this function if no one else uses it. CPT(RenderEffects) pt_state = state; - pair result = _states.insert(state); + pair result = _states->insert(state); if (result.second) { // The state was inserted; save the iterator and return the // input state. diff --git a/panda/src/pgraph/renderEffects.h b/panda/src/pgraph/renderEffects.h index 108822d819..791dde786f 100644 --- a/panda/src/pgraph/renderEffects.h +++ b/panda/src/pgraph/renderEffects.h @@ -57,13 +57,13 @@ private: public: virtual ~RenderEffects(); - bool operator < (const RenderEffects &other) const; - bool safe_to_transform() const; bool safe_to_combine() const; CPT(RenderEffects) xform(const LMatrix4f &mat) const; PUBLISHED: + bool operator < (const RenderEffects &other) const; + INLINE bool is_empty() const; INLINE int get_num_effects() const; INLINE const RenderEffect *get_effect(int n) const; @@ -90,6 +90,10 @@ PUBLISHED: void output(ostream &out) const; void write(ostream &out, int indent_level) const; + static int get_num_states(); + static void list_states(ostream &out); + static bool validate_states(); + public: INLINE bool has_decal() const; INLINE bool has_show_bounds() const; @@ -107,7 +111,7 @@ private: private: typedef pset > States; - static States _states; + static States *_states; static CPT(RenderEffects) _empty_state; // This iterator records the entry corresponding to this RenderEffects diff --git a/panda/src/pgraph/renderState.I b/panda/src/pgraph/renderState.I index f40eb23f72..37890d1bed 100644 --- a/panda/src/pgraph/renderState.I +++ b/panda/src/pgraph/renderState.I @@ -17,127 +17,6 @@ //////////////////////////////////////////////////////////////////// -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Composition::Constructor -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE RenderState::Composition:: -Composition() { -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Composition::Copy Constructor -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE RenderState::Composition:: -Composition(const RenderState::Composition ©) : - _result(copy._result) -{ -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::Constructor -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE RenderState::Attribute:: -Attribute(const RenderAttrib *attrib, int override) : - _type(attrib->get_type()), - _attrib(attrib), - _override(override) -{ -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::Constructor -// Access: Public -// Description: This constructor is only used when reading the -// RenderState from a bam file. At this point, the -// attribute pointer is unknown. -//////////////////////////////////////////////////////////////////// -INLINE RenderState::Attribute:: -Attribute(int override) : - _override(override) -{ -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::Constructor -// Access: Public -// Description: This constructor makes an invalid Attribute with no -// RenderAttrib pointer; its purpose is just to make an -// object we can use to look up a particular type in the -// Attribute set. -//////////////////////////////////////////////////////////////////// -INLINE RenderState::Attribute:: -Attribute(TypeHandle type) : - _type(type), - _attrib(NULL), - _override(0) -{ -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::Copy Constructor -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE RenderState::Attribute:: -Attribute(const Attribute ©) : - _type(copy._type), - _attrib(copy._attrib), - _override(copy._override) -{ -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::Copy Assignment Operator -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE void RenderState::Attribute:: -operator = (const Attribute ©) { - _type = copy._type; - _attrib = copy._attrib; - _override = copy._override; -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::operator < -// Access: Public -// Description: This is used by the Attributes set to uniquify -// RenderAttributes by type. Only one RenderAttrib of a -// given type is allowed in the set. This ordering must -// also match the ordering reported by compare_to(). -//////////////////////////////////////////////////////////////////// -INLINE bool RenderState::Attribute:: -operator < (const Attribute &other) const { - return _type < other._type; -} - -//////////////////////////////////////////////////////////////////// -// Function: RenderState::Attribute::compare_to -// Access: Public -// Description: Provides an indication of whether a particular -// attribute is equivalent to another one, for purposes -// of generating unique RenderStates. This should -// compare all properties of the Attribute, but it is -// important that the type is compared first, to be -// consistent with the ordering defined by operator <. -//////////////////////////////////////////////////////////////////// -INLINE int RenderState::Attribute:: -compare_to(const Attribute &other) const { - if (_type != other._type) { - return _type.get_index() - other._type.get_index(); - } - if (_attrib != other._attrib) { - return _attrib < other._attrib ? -1 : 1; - } - return _override - other._override; -} - //////////////////////////////////////////////////////////////////// // Function: RenderState::is_empty // Access: Published @@ -308,3 +187,165 @@ is_destructing() const { return false; #endif } + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Composition::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::Composition:: +Composition() { +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Composition::Copy Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::Composition:: +Composition(const RenderState::Composition ©) : + _result(copy._result) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::Attribute:: +Attribute(const RenderAttrib *attrib, int override) : + _type(attrib->get_type()), + _attrib(attrib), + _override(override) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::Constructor +// Access: Public +// Description: This constructor is only used when reading the +// RenderState from a bam file. At this point, the +// attribute pointer is unknown. +//////////////////////////////////////////////////////////////////// +INLINE RenderState::Attribute:: +Attribute(int override) : + _override(override) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::Constructor +// Access: Public +// Description: This constructor makes an invalid Attribute with no +// RenderAttrib pointer; its purpose is just to make an +// object we can use to look up a particular type in the +// Attribute set. +//////////////////////////////////////////////////////////////////// +INLINE RenderState::Attribute:: +Attribute(TypeHandle type) : + _type(type), + _attrib(NULL), + _override(0) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::Copy Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::Attribute:: +Attribute(const Attribute ©) : + _type(copy._type), + _attrib(copy._attrib), + _override(copy._override) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::Copy Assignment Operator +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE void RenderState::Attribute:: +operator = (const Attribute ©) { + _type = copy._type; + _attrib = copy._attrib; + _override = copy._override; +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::operator < +// Access: Public +// Description: This is used by the Attributes set to uniquify +// RenderAttributes by type. Only one RenderAttrib of a +// given type is allowed in the set. This ordering must +// also match the ordering reported by compare_to(). +//////////////////////////////////////////////////////////////////// +INLINE bool RenderState::Attribute:: +operator < (const Attribute &other) const { + return _type < other._type; +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::Attribute::compare_to +// Access: Public +// Description: Provides an indication of whether a particular +// attribute is equivalent to another one, for purposes +// of generating unique RenderStates. This should +// compare all properties of the Attribute, but it is +// important that the type is compared first, to be +// consistent with the ordering defined by operator <. +//////////////////////////////////////////////////////////////////// +INLINE int RenderState::Attribute:: +compare_to(const Attribute &other) const { + if (_type != other._type) { + return _type.get_index() - other._type.get_index(); + } + if (_attrib != other._attrib) { + return _attrib < other._attrib ? -1 : 1; + } + return _override - other._override; +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::CompositionCycleDescEntry::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::CompositionCycleDescEntry:: +CompositionCycleDescEntry(const RenderState *obj, + const RenderState *result, + bool inverted) : + _obj(obj), + _result(result), + _inverted(inverted) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::CycleChain::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::CycleChain:: +CycleChain(const RenderState *state) : + _state(state), + _prev(NULL), + _length(1) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::CycleChain::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE RenderState::CycleChain:: +CycleChain(CycleChain *prev, const RenderState *state) : + _state(state), + _prev(prev), + _length(prev->_length + 1) +{ +} diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index ec799000c5..757da1c347 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -52,7 +52,6 @@ RenderState() { _states = new States; } _saved_entry = _states->end(); - _self_compose = (RenderState *)NULL; _flags = 0; } @@ -127,13 +126,8 @@ RenderState:: // objects destruct, there will be no reason for that one to. RenderState *other = (RenderState *)(*ci).first; - // We should never have a reflexive entry in this map. If we - // do, something got screwed up elsewhere. - nassertv(other != this); - - // We hold a copy of the composition result to ensure that the - // result RenderState object (if there is one) doesn't - // destruct. + // 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, @@ -142,25 +136,33 @@ RenderState:: // is still valid. _composition_cache.erase(ci); - CompositionCache::iterator oci = other->_composition_cache.find(this); + 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; - - // Now we're holding a reference count to both computed - // results, so no objects will be tempted to destruct while we - // erase the other cache entry. - other->_composition_cache.erase(oci); + // 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. This may - // have cascading effects as other RenderState objects are - // destructed, but there will be no harm done if they destruct - // now. + // 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. @@ -170,25 +172,30 @@ RenderState:: nassertv(other != this); Composition comp = (*ci).second; _invert_composition_cache.erase(ci); - 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 (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); } } - // Also, if we called compose(this) at some point and the return - // value was something other than this, we need to decrement the - // associated reference count. - if (_self_compose != (RenderState *)NULL && _self_compose != this) { - unref_delete((RenderState *)_self_compose); - } + // If this was true at the beginning of the destructor, but is no + // longer true now, probably we've been double-deleted. + nassertv(get_ref_count() == 0); } //////////////////////////////////////////////////////////////////// // Function: RenderState::operator < -// Access: Public +// Access: Published // Description: Provides an arbitrary ordering among all unique // RenderStates, so we can store the essentially // different ones in a big set and throw away the rest. @@ -350,28 +357,6 @@ compose(const RenderState *other) const { return this; } - if (other == this) { - // compose(this) has to be handled as a special case, because the - // caching problem is so different. - if (_self_compose != (RenderState *)NULL) { - return _self_compose; - } - CPT(RenderState) result = do_compose(this); - ((RenderState *)this)->_self_compose = result; - - if (result != (const RenderState *)this) { - // If the result of compose(this) is something other than this, - // explicitly increment the reference count. We have to be sure - // to decrement it again later, in our destructor. - _self_compose->ref(); - - // (If the result was just this again, we still store the - // result, but we don't increment the reference count, since - // that would be a self-referential leak. What a mess this is.) - } - return _self_compose; - } - // Is this composition already cached? CompositionCache::const_iterator ci = _composition_cache.find(other); if (ci != _composition_cache.end()) { @@ -380,7 +365,14 @@ compose(const RenderState *other) const { // Well, it wasn't cached already, but we already had an entry // (probably created for the reverse direction), so use the same // entry to store the new result. - ((Composition &)comp)._result = do_compose(other); + CPT(RenderState) result = do_compose(other); + ((Composition &)comp)._result = result; + + 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(); + } } // Here's the cache! return comp._result; @@ -395,9 +387,22 @@ compose(const RenderState *other) const { // result; the other will be NULL for now. CPT(RenderState) result = do_compose(other); + // Order is important here, in case other == this. ((RenderState *)other)->_composition_cache[this]._result = NULL; ((RenderState *)this)->_composition_cache[other]._result = result; + if (result != (const RenderState *)this) { + // If the result of compose() is something other than this, + // 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(); + + // (If the result was just this again, we still store the + // result, but we don't increment the reference count, since + // that would be a self-referential leak.) + } + return result; } @@ -438,7 +443,14 @@ invert_compose(const RenderState *other) const { // Well, it wasn't cached already, but we already had an entry // (probably created for the reverse direction), so use the same // entry to store the new result. - ((Composition &)comp)._result = do_invert_compose(other); + CPT(RenderState) result = do_invert_compose(other); + ((Composition &)comp)._result = result; + + 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(); + } } // Here's the cache! return comp._result; @@ -456,6 +468,18 @@ invert_compose(const RenderState *other) const { ((RenderState *)other)->_invert_composition_cache[this]._result = NULL; ((RenderState *)this)->_invert_composition_cache[other]._result = result; + if (result != (const RenderState *)this) { + // If the result of compose() is something other than this, + // 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(); + + // (If the result was just this again, we still store the + // result, but we don't increment the reference count, since + // that would be a self-referential leak.) + } + return result; } @@ -658,7 +682,18 @@ get_num_states() { // Access: Published, Static // Description: Returns the total number of RenderState objects that // have been allocated but have no references outside of -// the internal RenderState map. +// the internal RenderState cache. +// +// A nonzero return value is not necessarily indicative +// of leaked references; it is normal for two +// RenderState objects, both of which have references +// held outside the cache, to have to result of their +// composition stored within the cache. This result +// will be retained within the cache until one of the +// base RenderStates is released. +// +// Use list_cycles() to get an idea of the number of +// actual "leaked" RenderState objects. //////////////////////////////////////////////////////////////////// int RenderState:: get_num_unused_states() { @@ -666,8 +701,6 @@ get_num_unused_states() { return 0; } - int num_unused = 0; - // First, we need to count the number of times each RenderState // object is recorded in the cache. typedef pmap StateCount; @@ -682,7 +715,7 @@ get_num_unused_states() { ci != state->_composition_cache.end(); ++ci) { const RenderState *result = (*ci).second._result; - if (result != (const RenderState *)NULL) { + if (result != (const RenderState *)NULL && result != state) { // Here's a RenderState that's recorded in the cache. // Count it. pair ir = @@ -698,7 +731,7 @@ get_num_unused_states() { ci != state->_invert_composition_cache.end(); ++ci) { const RenderState *result = (*ci).second._result; - if (result != (const RenderState *)NULL) { + if (result != (const RenderState *)NULL && result != state) { pair ir = state_count.insert(StateCount::value_type(result, 1)); if (!ir.second) { @@ -706,26 +739,13 @@ get_num_unused_states() { } } } - - // Finally, check the self_compose field, which might be reference - // counted too. - if (state->_self_compose != (const RenderState *)NULL && - state->_self_compose != state) { - const RenderState *result = state->_self_compose; - if (result != (const RenderState *)NULL) { - pair ir = - state_count.insert(StateCount::value_type(result, 1)); - if (!ir.second) { - (*(ir.first)).second++; - } - } - } - } // Now that we have the appearance count of each RenderState // object, we can tell which ones are unreferenced outside of the // RenderState cache, by comparing these to the reference counts. + int num_unused = 0; + StateCount::iterator sci; for (sci = state_count.begin(); sci != state_count.end(); ++sci) { const RenderState *state = (*sci).first; @@ -733,6 +753,13 @@ get_num_unused_states() { nassertr(count <= state->get_ref_count(), num_unused); if (count == state->get_ref_count()) { num_unused++; + + if (pgraph_cat.is_debug()) { + pgraph_cat.debug() + << "Unused state: " << (void *)state << ":" + << state->get_ref_count() << " =\n"; + state->write(pgraph_cat.debug(false), 2); + } } } @@ -790,13 +817,29 @@ clear_cache() { TempStates::iterator ti; for (ti = temp_states.begin(); ti != temp_states.end(); ++ti) { RenderState *state = (RenderState *)(*ti).p(); - state->_composition_cache.clear(); - state->_invert_composition_cache.clear(); - if (state->_self_compose != (RenderState *)NULL && - state->_self_compose != state) { - unref_delete((RenderState *)state->_self_compose); - state->_self_compose = (RenderState *)NULL; + + CompositionCache::const_iterator ci; + for (ci = state->_composition_cache.begin(); + ci != state->_composition_cache.end(); + ++ci) { + const RenderState *result = (*ci).second._result; + if (result != (const RenderState *)NULL && result != state) { + result->unref(); + nassertr(result->get_ref_count() > 0, 0); + } } + state->_composition_cache.clear(); + + for (ci = state->_invert_composition_cache.begin(); + ci != state->_invert_composition_cache.end(); + ++ci) { + const RenderState *result = (*ci).second._result; + if (result != (const RenderState *)NULL && result != state) { + result->unref(); + nassertr(result->get_ref_count() > 0, 0); + } + } + state->_invert_composition_cache.clear(); } // Once this block closes and the temp_states object goes away, @@ -808,6 +851,120 @@ clear_cache() { return orig_size - new_size; } +//////////////////////////////////////////////////////////////////// +// Function: RenderState::list_cycles +// Access: Published, Static +// Description: Detects all of the reference-count cycles in the +// 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. +// +// The cycles listed here are not leaks in the strictest +// sense of the word, since they can be reclaimed by a +// call to clear_cache(); but they will not be reclaimed +// automatically. +//////////////////////////////////////////////////////////////////// +void RenderState:: +list_cycles(ostream &out) { + VisitedStates visited; + CompositionCycleDesc cycle_desc; + + States::iterator si; + for (si = _states->begin(); si != _states->end(); ++si) { + const RenderState *state = (*si); + + bool inserted = visited.insert(state).second; + if (inserted) { + VisitedStates visited_this_cycle; + CycleChain chain(state); + if (r_detect_cycles(state, visited_this_cycle, &chain, cycle_desc)) { + // This state begins a cycle. + CompositionCycleDesc::reverse_iterator csi; + + out << "\nCycle detected of length " << cycle_desc.size() + 1 << ":\n" + << "state " << (void *)state << ":" << state->get_ref_count() + << " =\n"; + state->write(out, 2); + for (csi = cycle_desc.rbegin(); csi != cycle_desc.rend(); ++csi) { + const CompositionCycleDescEntry &entry = (*csi); + if (entry._inverted) { + out << "invert composed with "; + } else { + out << "composed with "; + } + out << (const void *)entry._obj << ":" << entry._obj->get_ref_count() + << " " << *entry._obj << "\n" + << "produces " << (const void *)entry._result << ":" + << entry._result->get_ref_count() << " =\n"; + entry._result->write(out, 2); + visited.insert(entry._result); + } + + cycle_desc.clear(); + } + } + } +} + + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::list_states +// Access: Published, Static +// Description: Lists all of the RenderStates in the cache to the +// output stream, one per line. This can be quite a lot +// of output if the cache is large, so be prepared. +//////////////////////////////////////////////////////////////////// +void RenderState:: +list_states(ostream &out) { + out << _states->size() << " states:\n"; + States::const_iterator si; + for (si = _states->begin(); si != _states->end(); ++si) { + const RenderState *state = (*si); + state->write(out, 2); + } +} + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::validate_states +// Access: Published, Static +// Description: Ensures that the cache is still stored in sorted +// order, and that none of the cache elements have been +// inadvertently deleted. Returns true if so, false if +// there is a problem (which implies someone has +// modified one of the supposedly-const RenderState +// objects). +//////////////////////////////////////////////////////////////////// +bool RenderState:: +validate_states() { + if (_states->empty()) { + return true; + } + + States::const_iterator si = _states->begin(); + States::const_iterator snext = si; + ++snext; + nassertr((*si)->get_ref_count() > 0, false); + while (snext != _states->end()) { + if (!(*(*si) < *(*snext))) { + pgraph_cat.error() + << "RenderStates out of order!\n"; + (*si)->write(pgraph_cat.error(false), 2); + (*snext)->write(pgraph_cat.error(false), 2); + return false; + } + si = snext; + ++snext; + nassertr((*si)->get_ref_count() > 0, false); + } + + return true; +} + //////////////////////////////////////////////////////////////////// // Function: RenderState::issue_delta_modify // Access: Public @@ -982,6 +1139,12 @@ return_new(RenderState *state) { // for anything else. nassertr(state->_saved_entry == _states->end(), state); +#ifndef NDEBUG + if (paranoid_const) { + nassertr(validate_states(), state); + } +#endif + // Save the state in a local PointerTo so that it will be freed at // the end of this function if no one else uses it. CPT(RenderState) pt_state = state; @@ -1120,6 +1283,67 @@ do_invert_compose(const RenderState *other) const { return return_new(new_state); } +//////////////////////////////////////////////////////////////////// +// Function: RenderState::r_detect_cycles +// Access: Private, Static +// 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. +//////////////////////////////////////////////////////////////////// +bool RenderState:: +r_detect_cycles(const RenderState *state, + RenderState::VisitedStates &visited_this_cycle, + RenderState::CycleChain *chain, + RenderState::CompositionCycleDesc &cycle_desc) { + bool inserted = visited_this_cycle.insert(state).second; + if (!inserted && chain->has_result(state)) { + // We've already seen this state; therefore, we've found a cycle. + + // However, we only care about cycles that involve more than two + // steps. If only one or two nodes are involved, it doesn't + // represent a memory leak, so no problem there. + return (chain->_length > 2); + } + + CompositionCache::const_iterator ci; + for (ci = state->_composition_cache.begin(); + ci != state->_composition_cache.end(); + ++ci) { + const RenderState *result = (*ci).second._result; + if (result != (const RenderState *)NULL) { + CycleChain next_chain(chain, result); + if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) { + // Cycle detected. + CompositionCycleDescEntry entry((*ci).first, result, false); + cycle_desc.push_back(entry); + return true; + } + } + } + + for (ci = state->_invert_composition_cache.begin(); + ci != state->_invert_composition_cache.end(); + ++ci) { + const RenderState *result = (*ci).second._result; + if (result != (const RenderState *)NULL) { + CycleChain next_chain(chain, result); + if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) { + // Cycle detected. + CompositionCycleDescEntry entry((*ci).first, result, true); + cycle_desc.push_back(entry); + return true; + } + } + } + + // No cycle detected. + return false; +} + + //////////////////////////////////////////////////////////////////// // Function: RenderState::determine_bin_index // Access: Private @@ -1375,3 +1599,20 @@ fillin(DatagramIterator &scan, BamReader *manager) { _attributes.push_back(Attribute(override)); } } + +//////////////////////////////////////////////////////////////////// +// Function: RenderState::CycleChain::has_result +// Access: Public +// Description: Returns true if the indicated state has been reached +// in this chain previously, false otherwise. +//////////////////////////////////////////////////////////////////// +bool RenderState::CycleChain:: +has_result(const RenderState *state) const { + if (_state == state) { + return true; + } + if (_prev != NULL) { + return _prev->has_result(state); + } + return false; +} diff --git a/panda/src/pgraph/renderState.h b/panda/src/pgraph/renderState.h index 17d6859401..eadc47fc92 100644 --- a/panda/src/pgraph/renderState.h +++ b/panda/src/pgraph/renderState.h @@ -56,9 +56,9 @@ private: public: virtual ~RenderState(); +PUBLISHED: bool operator < (const RenderState &other) const; -PUBLISHED: INLINE bool is_empty() const; INLINE int get_num_attribs() const; INLINE const RenderAttrib *get_attrib(int n) const; @@ -99,6 +99,9 @@ PUBLISHED: static int get_num_states(); static int get_num_unused_states(); static int clear_cache(); + static void list_cycles(ostream &out); + static void list_states(ostream &out); + static bool validate_states(); PUBLISHED: // These methods are intended for use by low-level code, but they're @@ -118,9 +121,38 @@ public: static void bin_removed(int bin_index); private: + class CompositionCycleDescEntry { + public: + INLINE CompositionCycleDescEntry(const RenderState *obj, + const RenderState *result, + bool inverted); + + const RenderState *_obj; + const RenderState *_result; + bool _inverted; + }; + typedef pvector CompositionCycleDesc; + typedef pset VisitedStates; + class CycleChain { + public: + INLINE CycleChain(const RenderState *state); + INLINE CycleChain(CycleChain *prev, const RenderState *state); + + bool has_result(const RenderState *state) const; + + const RenderState *_state; + CycleChain *_prev; + int _length; + }; + 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 *state, + VisitedStates &visited_this_cycle, + CycleChain *chain, + CompositionCycleDesc &cycle_desc); + void determine_bin_index(); void determine_fog(); void determine_bin(); @@ -149,21 +181,19 @@ private: INLINE Composition(); INLINE Composition(const Composition ©); - CPT(RenderState) _result; + // _result is reference counted if and only if it is not the same + // pointer as this. + const RenderState *_result; }; - + + // The first element of the map is the object we compose with. This + // is not reference counted within this map; instead we store a + // companion pointer in the other object, and remove the references + // explicitly when either object destructs. typedef pmap CompositionCache; CompositionCache _composition_cache; CompositionCache _invert_composition_cache; - // Thise pointer is used to cache the result of compose(this). This - // has to be a special case, because we have to handle the reference - // counts carefully so that we don't leak. Most of the time, the - // result of compose(this) is this, which should not be reference - // counted, but other times the result is something else (which - // should be). - const RenderState *_self_compose; - private: // This is the actual data within the RenderState: a set of // RenderAttribs. diff --git a/panda/src/pgraph/transformState.I b/panda/src/pgraph/transformState.I index e14deef19b..b93fb61d47 100644 --- a/panda/src/pgraph/transformState.I +++ b/panda/src/pgraph/transformState.I @@ -17,26 +17,6 @@ //////////////////////////////////////////////////////////////////// -//////////////////////////////////////////////////////////////////// -// Function: TransformState::Composition::Constructor -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE TransformState::Composition:: -Composition() { -} - -//////////////////////////////////////////////////////////////////// -// Function: TransformState::Composition::Copy Constructor -// Access: Public -// Description: -//////////////////////////////////////////////////////////////////// -INLINE TransformState::Composition:: -Composition(const TransformState::Composition ©) : - _result(copy._result) -{ -} - //////////////////////////////////////////////////////////////////// // Function: TransformState::make_pos // Access: Published, Static @@ -582,3 +562,64 @@ is_destructing() const { return false; #endif } + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::Composition::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE TransformState::Composition:: +Composition() { +} + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::Composition::Copy Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE TransformState::Composition:: +Composition(const TransformState::Composition ©) : + _result(copy._result) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::CompositionCycleDescEntry::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE TransformState::CompositionCycleDescEntry:: +CompositionCycleDescEntry(const TransformState *obj, + const TransformState *result, + bool inverted) : + _obj(obj), + _result(result), + _inverted(inverted) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::CycleChain::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE TransformState::CycleChain:: +CycleChain(const TransformState *state) : + _state(state), + _prev(NULL), + _length(1) +{ +} + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::CycleChain::Constructor +// Access: Public +// Description: +//////////////////////////////////////////////////////////////////// +INLINE TransformState::CycleChain:: +CycleChain(CycleChain *prev, const TransformState *state) : + _state(state), + _prev(prev), + _length(prev->_length + 1) +{ +} diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index 70ac082976..f30a694dc5 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -47,7 +47,6 @@ TransformState() { _states = new States; } _saved_entry = _states->end(); - _self_compose = (TransformState *)NULL; _flags = F_is_identity | F_singular_known; _inv_mat = (LMatrix4f *)NULL; } @@ -83,14 +82,13 @@ TransformState:: // We'd better not call the destructor twice on a particular object. nassertv(!is_destructing()); set_destructing(); - + // Free the inverse matrix computation, if it has been stored. if (_inv_mat != (LMatrix4f *)NULL) { delete _inv_mat; _inv_mat = (LMatrix4f *)NULL; } - // Remove the deleted TransformState object from the global pool. if (_saved_entry != _states->end()) { nassertv(_states->find(this) == _saved_entry); _states->erase(_saved_entry); @@ -130,13 +128,8 @@ TransformState:: // objects destruct, there will be no reason for that one to. TransformState *other = (TransformState *)(*ci).first; - // We should never have a reflexive entry in this map. If we - // do, something got screwed up elsewhere. - nassertv(other != this); - - // We hold a copy of the composition result to ensure that the - // result TransformState object (if there is one) doesn't - // destruct. + // 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, @@ -145,25 +138,33 @@ TransformState:: // is still valid. _composition_cache.erase(ci); - CompositionCache::iterator oci = other->_composition_cache.find(this); + 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; - - // Now we're holding a reference count to both computed - // results, so no objects will be tempted to destruct while we - // erase the other cache entry. - other->_composition_cache.erase(oci); + // 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. This may - // have cascading effects as other TransformState objects are - // destructed, but there will be no harm done if they destruct - // now. + // 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. @@ -173,19 +174,20 @@ TransformState:: nassertv(other != this); Composition comp = (*ci).second; _invert_composition_cache.erase(ci); - 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 (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); } - } - - // Also, if we called compose(this) at some point and the return - // value was something other than this, we need to decrement the - // associated reference count. - if (_self_compose != (TransformState *)NULL && _self_compose != this) { - unref_delete((TransformState *)_self_compose); } // If this was true at the beginning of the destructor, but is no @@ -195,7 +197,7 @@ TransformState:: //////////////////////////////////////////////////////////////////// // Function: TransformState::operator < -// Access: Public +// Access: Published // Description: Provides an arbitrary ordering among all unique // TransformStates, so we can store the essentially // different ones in a big set and throw away the rest. @@ -483,7 +485,7 @@ compose(const TransformState *other) const { if (other->is_identity()) { return this; } - + // If either transform is invalid, the result is invalid. if (is_invalid()) { return this; @@ -492,28 +494,6 @@ compose(const TransformState *other) const { return other; } - if (other == this) { - // compose(this) has to be handled as a special case, because the - // caching problem is so different. - if (_self_compose != (TransformState *)NULL) { - return _self_compose; - } - CPT(TransformState) result = do_compose(this); - ((TransformState *)this)->_self_compose = result; - - if (result != (const TransformState *)this) { - // If the result of compose(this) is something other than this, - // explicitly increment the reference count. We have to be sure - // to decrement it again later, in our destructor. - _self_compose->ref(); - - // (If the result was just this again, we still store the - // result, but we don't increment the reference count, since - // that would be a self-referential leak. What a mess this is.) - } - return _self_compose; - } - // Is this composition already cached? CompositionCache::const_iterator ci = _composition_cache.find(other); if (ci != _composition_cache.end()) { @@ -522,7 +502,14 @@ compose(const TransformState *other) const { // Well, it wasn't cached already, but we already had an entry // (probably created for the reverse direction), so use the same // entry to store the new result. - ((Composition &)comp)._result = do_compose(other); + CPT(TransformState) result = do_compose(other); + ((Composition &)comp)._result = result; + + 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(); + } } // Here's the cache! return comp._result; @@ -537,9 +524,22 @@ compose(const TransformState *other) const { // result; the other will be NULL for now. CPT(TransformState) result = do_compose(other); + // Order is important here, in case other == this. ((TransformState *)other)->_composition_cache[this]._result = NULL; ((TransformState *)this)->_composition_cache[other]._result = result; + if (result != (const TransformState *)this) { + // If the result of compose() is something other than this, + // 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(); + + // (If the result was just this again, we still store the + // result, but we don't increment the reference count, since + // that would be a self-referential leak.) + } + return result; } @@ -588,7 +588,14 @@ invert_compose(const TransformState *other) const { // Well, it wasn't cached already, but we already had an entry // (probably created for the reverse direction), so use the same // entry to store the new result. - ((Composition &)comp)._result = do_invert_compose(other); + CPT(TransformState) result = do_invert_compose(other); + ((Composition &)comp)._result = result; + + 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(); + } } // Here's the cache! return comp._result; @@ -606,6 +613,18 @@ invert_compose(const TransformState *other) const { ((TransformState *)other)->_invert_composition_cache[this]._result = NULL; ((TransformState *)this)->_invert_composition_cache[other]._result = result; + if (result != (const TransformState *)this) { + // If the result of compose() is something other than this, + // 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(); + + // (If the result was just this again, we still store the + // result, but we don't increment the reference count, since + // that would be a self-referential leak.) + } + return result; } @@ -702,9 +721,20 @@ get_num_states() { //////////////////////////////////////////////////////////////////// // Function: TransformState::get_num_unused_states // Access: Published, Static -// Description: Returns the total number of TransformState objects -// that have been allocated but have no references -// outside of the internal TransformState map. +// Description: Returns the total number of TransformState objects that +// have been allocated but have no references outside of +// the internal TransformState cache. +// +// A nonzero return value is not necessarily indicative +// of leaked references; it is normal for two +// TransformState objects, both of which have references +// held outside the cache, to have to result of their +// composition stored within the cache. This result +// will be retained within the cache until one of the +// base TransformStates is released. +// +// Use list_cycles() to get an idea of the number of +// actual "leaked" TransformState objects. //////////////////////////////////////////////////////////////////// int TransformState:: get_num_unused_states() { @@ -712,8 +742,6 @@ get_num_unused_states() { return 0; } - int num_unused = 0; - // First, we need to count the number of times each TransformState // object is recorded in the cache. typedef pmap StateCount; @@ -728,7 +756,7 @@ get_num_unused_states() { ci != state->_composition_cache.end(); ++ci) { const TransformState *result = (*ci).second._result; - if (result != (const TransformState *)NULL) { + if (result != (const TransformState *)NULL && result != state) { // Here's a TransformState that's recorded in the cache. // Count it. pair ir = @@ -744,7 +772,7 @@ get_num_unused_states() { ci != state->_invert_composition_cache.end(); ++ci) { const TransformState *result = (*ci).second._result; - if (result != (const TransformState *)NULL) { + if (result != (const TransformState *)NULL && result != state) { pair ir = state_count.insert(StateCount::value_type(result, 1)); if (!ir.second) { @@ -752,26 +780,13 @@ get_num_unused_states() { } } } - - // Finally, check the self_compose field, which might be reference - // counted too. - if (state->_self_compose != (const TransformState *)NULL && - state->_self_compose != state) { - const TransformState *result = state->_self_compose; - if (result != (const TransformState *)NULL) { - pair ir = - state_count.insert(StateCount::value_type(result, 1)); - if (!ir.second) { - (*(ir.first)).second++; - } - } - } - } // Now that we have the appearance count of each TransformState // object, we can tell which ones are unreferenced outside of the // TransformState cache, by comparing these to the reference counts. + int num_unused = 0; + StateCount::iterator sci; for (sci = state_count.begin(); sci != state_count.end(); ++sci) { const TransformState *state = (*sci).first; @@ -779,6 +794,13 @@ get_num_unused_states() { nassertr(count <= state->get_ref_count(), num_unused); if (count == state->get_ref_count()) { num_unused++; + + if (pgraph_cat.is_debug()) { + pgraph_cat.debug() + << "Unused state: " << (void *)state << ":" + << state->get_ref_count() << " =\n"; + state->write(pgraph_cat.debug(false), 2); + } } } @@ -818,10 +840,10 @@ clear_cache() { int orig_size = _states->size(); - // First, we need to copy the entire set of transforms to a - // temporary vector, reference-counting each object. That way we - // can walk through the copy, without fear of dereferencing (and - // deleting) the objects in the map as we go. + // First, we need to copy the entire set of states to a temporary + // vector, reference-counting each object. That way we can walk + // through the copy, without fear of dereferencing (and deleting) + // the objects in the map as we go. { typedef pvector< CPT(TransformState) > TempStates; TempStates temp_states; @@ -836,13 +858,29 @@ clear_cache() { TempStates::iterator ti; for (ti = temp_states.begin(); ti != temp_states.end(); ++ti) { TransformState *state = (TransformState *)(*ti).p(); - state->_composition_cache.clear(); - state->_invert_composition_cache.clear(); - if (state->_self_compose != (TransformState *)NULL && - state->_self_compose != state) { - unref_delete((TransformState *)state->_self_compose); - state->_self_compose = (TransformState *)NULL; + + CompositionCache::const_iterator ci; + for (ci = state->_composition_cache.begin(); + ci != state->_composition_cache.end(); + ++ci) { + const TransformState *result = (*ci).second._result; + if (result != (const TransformState *)NULL && result != state) { + result->unref(); + nassertr(result->get_ref_count() > 0, 0); + } } + state->_composition_cache.clear(); + + for (ci = state->_invert_composition_cache.begin(); + ci != state->_invert_composition_cache.end(); + ++ci) { + const TransformState *result = (*ci).second._result; + if (result != (const TransformState *)NULL && result != state) { + result->unref(); + nassertr(result->get_ref_count() > 0, 0); + } + } + state->_invert_composition_cache.clear(); } // Once this block closes and the temp_states object goes away, @@ -854,6 +892,120 @@ clear_cache() { return orig_size - new_size; } +//////////////////////////////////////////////////////////////////// +// Function: TransformState::list_cycles +// Access: Published, Static +// Description: Detects all of the reference-count cycles in the +// 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. +// +// The cycles listed here are not leaks in the strictest +// sense of the word, since they can be reclaimed by a +// call to clear_cache(); but they will not be reclaimed +// automatically. +//////////////////////////////////////////////////////////////////// +void TransformState:: +list_cycles(ostream &out) { + VisitedStates visited; + CompositionCycleDesc cycle_desc; + + States::iterator si; + for (si = _states->begin(); si != _states->end(); ++si) { + const TransformState *state = (*si); + + bool inserted = visited.insert(state).second; + if (inserted) { + VisitedStates visited_this_cycle; + CycleChain chain(state); + if (r_detect_cycles(state, visited_this_cycle, &chain, cycle_desc)) { + // This state begins a cycle. + CompositionCycleDesc::reverse_iterator csi; + + out << "\nCycle detected of length " << cycle_desc.size() + 1 << ":\n" + << "state " << (void *)state << ":" << state->get_ref_count() + << " =\n"; + state->write(out, 2); + for (csi = cycle_desc.rbegin(); csi != cycle_desc.rend(); ++csi) { + const CompositionCycleDescEntry &entry = (*csi); + if (entry._inverted) { + out << "invert composed with "; + } else { + out << "composed with "; + } + out << (const void *)entry._obj << ":" << entry._obj->get_ref_count() + << " " << *entry._obj << "\n" + << "produces " << (const void *)entry._result << ":" + << entry._result->get_ref_count() << " =\n"; + entry._result->write(out, 2); + visited.insert(entry._result); + } + + cycle_desc.clear(); + } + } + } +} + + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::list_states +// Access: Published, Static +// Description: Lists all of the TransformStates in the cache to the +// output stream, one per line. This can be quite a lot +// of output if the cache is large, so be prepared. +//////////////////////////////////////////////////////////////////// +void TransformState:: +list_states(ostream &out) { + out << _states->size() << " states:\n"; + States::const_iterator si; + for (si = _states->begin(); si != _states->end(); ++si) { + const TransformState *state = (*si); + state->write(out, 2); + } +} + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::validate_states +// Access: Published, Static +// Description: Ensures that the cache is still stored in sorted +// order, and that none of the cache elements have been +// inadvertently deleted. Returns true if so, false if +// there is a problem (which implies someone has +// modified one of the supposedly-const TransformState +// objects). +//////////////////////////////////////////////////////////////////// +bool TransformState:: +validate_states() { + if (_states->empty()) { + return true; + } + + States::const_iterator si = _states->begin(); + States::const_iterator snext = si; + ++snext; + nassertr((*si)->get_ref_count() > 0, false); + while (snext != _states->end()) { + if (!(*(*si) < *(*snext))) { + pgraph_cat.error() + << "TransformStates out of order!\n"; + (*si)->write(pgraph_cat.error(false), 2); + (*snext)->write(pgraph_cat.error(false), 2); + return false; + } + si = snext; + ++snext; + nassertr((*si)->get_ref_count() > 0, false); + } + + return true; +} + //////////////////////////////////////////////////////////////////// // Function: TransformState::return_new // Access: Private, Static @@ -874,6 +1026,12 @@ return_new(TransformState *state) { // for anything else. nassertr(state->_saved_entry == _states->end(), state); +#ifndef NDEBUG + if (paranoid_const) { + nassertr(validate_states(), state); + } +#endif + // Save the state in a local PointerTo so that it will be freed at // the end of this function if no one else uses it. CPT(TransformState) pt_state = state; @@ -1030,6 +1188,66 @@ do_invert_compose(const TransformState *other) const { } } +//////////////////////////////////////////////////////////////////// +// Function: TransformState::r_detect_cycles +// Access: Private, Static +// 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. +//////////////////////////////////////////////////////////////////// +bool TransformState:: +r_detect_cycles(const TransformState *state, + TransformState::VisitedStates &visited_this_cycle, + TransformState::CycleChain *chain, + TransformState::CompositionCycleDesc &cycle_desc) { + bool inserted = visited_this_cycle.insert(state).second; + if (!inserted && chain->has_result(state)) { + // We've already seen this state; therefore, we've found a cycle. + + // However, we only care about cycles that involve more than two + // steps. If only one or two nodes are involved, it doesn't + // represent a memory leak, so no problem there. + return (chain->_length > 2); + } + + CompositionCache::const_iterator ci; + for (ci = state->_composition_cache.begin(); + ci != state->_composition_cache.end(); + ++ci) { + const TransformState *result = (*ci).second._result; + if (result != (const TransformState *)NULL) { + CycleChain next_chain(chain, result); + if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) { + // Cycle detected. + CompositionCycleDescEntry entry((*ci).first, result, false); + cycle_desc.push_back(entry); + return true; + } + } + } + + for (ci = state->_invert_composition_cache.begin(); + ci != state->_invert_composition_cache.end(); + ++ci) { + const TransformState *result = (*ci).second._result; + if (result != (const TransformState *)NULL) { + CycleChain next_chain(chain, result); + if (r_detect_cycles(result, visited_this_cycle, &next_chain, cycle_desc)) { + // Cycle detected. + CompositionCycleDescEntry entry((*ci).first, result, true); + cycle_desc.push_back(entry); + return true; + } + } + } + + // No cycle detected. + return false; +} + //////////////////////////////////////////////////////////////////// // Function: TransformState::calc_singular // Access: Private @@ -1326,3 +1544,20 @@ fillin(DatagramIterator &scan, BamReader *manager) { _mat.read_datagram(scan); } } + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::CycleChain::has_result +// Access: Public +// Description: Returns true if the indicated state has been reached +// in this chain previously, false otherwise. +//////////////////////////////////////////////////////////////////// +bool TransformState::CycleChain:: +has_result(const TransformState *state) const { + if (_state == state) { + return true; + } + if (_prev != NULL) { + return _prev->has_result(state); + } + return false; +} diff --git a/panda/src/pgraph/transformState.h b/panda/src/pgraph/transformState.h index 495017b562..07bfb29d68 100644 --- a/panda/src/pgraph/transformState.h +++ b/panda/src/pgraph/transformState.h @@ -64,9 +64,9 @@ private: public: virtual ~TransformState(); +PUBLISHED: bool operator < (const TransformState &other) const; -PUBLISHED: static CPT(TransformState) make_identity(); static CPT(TransformState) make_invalid(); INLINE static CPT(TransformState) make_pos(const LVecBase3f &pos); @@ -131,11 +131,42 @@ PUBLISHED: static int get_num_states(); static int get_num_unused_states(); static int clear_cache(); + static void list_cycles(ostream &out); + static void list_states(ostream &out); + static bool validate_states(); private: + class CompositionCycleDescEntry { + public: + INLINE CompositionCycleDescEntry(const TransformState *obj, + const TransformState *result, + bool inverted); + + const TransformState *_obj; + const TransformState *_result; + bool _inverted; + }; + typedef pvector CompositionCycleDesc; + typedef pset VisitedStates; + class CycleChain { + public: + INLINE CycleChain(const TransformState *state); + INLINE CycleChain(CycleChain *prev, const TransformState *state); + + bool has_result(const TransformState *state) const; + + const TransformState *_state; + CycleChain *_prev; + int _length; + }; + 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 *state, + VisitedStates &visited_this_cycle, + CycleChain *chain, + CompositionCycleDesc &cycle_desc); private: typedef pset > States; @@ -157,18 +188,19 @@ private: INLINE Composition(); INLINE Composition(const Composition ©); - CPT(TransformState) _result; + // _result is reference counted if and only if it is not the same + // pointer as this. + const TransformState *_result; }; + // The first element of the map is the object we compose with. This + // is not reference counted within this map; instead we store a + // companion pointer in the other object, and remove the references + // explicitly when either object destructs. typedef pmap CompositionCache; CompositionCache _composition_cache; CompositionCache _invert_composition_cache; - // Thise pointer is used to cache the result of compose(this). This - // has to be a special case, because we have to handle the reference - // counts carefully so that we don't leak. - const TransformState *_self_compose; - private: // This is the actual data within the TransformState. INLINE void check_singular() const;