From 369f93951ffcf63dbadadd437f848883536add96 Mon Sep 17 00:00:00 2001 From: David Rose Date: Thu, 8 Sep 2011 15:02:34 +0000 Subject: [PATCH] fix a memory leak in the garbage-collect-states case --- panda/src/pgraph/renderAttrib.cxx | 44 +++++++++++++---------------- panda/src/pgraph/renderState.cxx | 43 +++++++++++++--------------- panda/src/pgraph/transformState.cxx | 43 +++++++++++++--------------- 3 files changed, 60 insertions(+), 70 deletions(-) diff --git a/panda/src/pgraph/renderAttrib.cxx b/panda/src/pgraph/renderAttrib.cxx index 763543a442..9306ddb76a 100644 --- a/panda/src/pgraph/renderAttrib.cxx +++ b/panda/src/pgraph/renderAttrib.cxx @@ -149,25 +149,15 @@ cull_callback(CullTraverser *, const CullTraverserData &) const { //////////////////////////////////////////////////////////////////// bool RenderAttrib:: unref() const { - if (!state_cache) { - // If we're not using the cache anyway, just allow the pointer to - // unref normally. + if (!state_cache || garbage_collect_states) { + // If we're not using the cache at all, or if we're relying on + // garbage collection, just allow the pointer to unref normally. return ReferenceCount::unref(); } - - if (garbage_collect_states) { - // In the garbage collector case, we don't delete RenderStates - // immediately; instead, we allow them to remain in the cache with - // a ref count of 0, and we delete them later in - // garbage_collect(). - - ReferenceCount::unref(); - // Return true so that it is never deleted here. - return true; - } // Here is the normal refcounting case, with a normal cache, and - // without garbage collection in effect. + // without garbage collection in effect. In this case we will pull + // the object out of the cache when its reference count goes to 0. // We always have to grab the lock, since we will definitely need to // be holding it if we happen to drop the reference count to 0. @@ -256,7 +246,7 @@ list_attribs(ostream &out) { //////////////////////////////////////////////////////////////////// int RenderAttrib:: garbage_collect() { - if (_attribs == (Attribs *)NULL) { + if (_attribs == (Attribs *)NULL || !garbage_collect_states) { return 0; } LightReMutexHolder holder(*_attribs_lock); @@ -281,15 +271,15 @@ garbage_collect() { if (_attribs->has_element(si)) { ++num_elements; RenderAttrib *attrib = (RenderAttrib *)_attribs->get_key(si); - if (attrib->get_ref_count() == 0) { - // This attrib has recently been unreffed to 0, but it hasn't - // been deleted yet (because we have overloaded unref(), - // above, to always return true). Now it's time to delete it. - // This is safe, because we're holding the _attribs_lock, so - // it's not possible for some other thread to find the attrib - // in the cache and ref it while we're doing this. + if (attrib->get_ref_count() == 1) { + // This attrib has recently been unreffed to 1 (the one we + // added when we stored it in the cache). Now it's time to + // delete it. This is safe, because we're holding the + // _attribs_lock, so it's not possible for some other thread to + // find the attrib in the cache and ref it while we're doing + // this. attrib->release_new(); - delete attrib; + unref_delete(attrib); } } @@ -444,6 +434,12 @@ return_unique(RenderAttrib *attrib) { } // Not already in the set; add it. + if (garbage_collect_states) { + // If we'll be garbage collecting attribs explicitly, we'll + // increment the reference count when we store it in the cache, so + // that it won't be deleted while it's in it. + attrib->ref(); + } si = _attribs->store(attrib, Empty()); // Save the index and return the input attrib. diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index 9bf518ddb2..5ce2f0799e 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -682,25 +682,15 @@ adjust_all_priorities(int adjustment) const { //////////////////////////////////////////////////////////////////// bool RenderState:: unref() const { - if (!state_cache) { - // If we're not using the cache anyway, just allow the pointer to - // unref normally. + if (!state_cache || garbage_collect_states) { + // If we're not using the cache at all, or if we're relying on + // garbage collection, just allow the pointer to unref normally. return ReferenceCount::unref(); } - - if (garbage_collect_states) { - // In the garbage collector case, we don't delete RenderStates - // immediately; instead, we allow them to remain in the cache with - // a ref count of 0, and we delete them later in - // garbage_collect(). - - ReferenceCount::unref(); - // Return true so that it is never deleted here. - return true; - } // Here is the normal refcounting case, with a normal cache, and - // without garbage collection in effect. + // without garbage collection in effect. In this case we will pull + // the object out of the cache when its reference count goes to 0. // We always have to grab the lock, since we will definitely need to // be holding it if we happen to drop the reference count to 0. @@ -1143,7 +1133,7 @@ int RenderState:: garbage_collect() { int num_attribs = RenderAttrib::garbage_collect(); - if (_states == (States *)NULL) { + if (_states == (States *)NULL || !garbage_collect_states) { return num_attribs; } LightReMutexHolder holder(*_states_lock); @@ -1177,15 +1167,16 @@ garbage_collect() { } } - if (state->get_ref_count() == 0) { - // This state has recently been unreffed to 0, but it hasn't - // been deleted yet (because we have overloaded unref(), - // above, to always return true). Now it's time to delete it. - // This is safe, because we're holding the _states_lock, so - // it's not possible for some other thread to find the state - // in the cache and ref it while we're doing this. + if (state->get_ref_count() == 1) { + // This state has recently been unreffed to 1 (the one we + // added when we stored it in the cache). Now it's time to + // delete it. This is safe, because we're holding the + // _states_lock, so it's not possible for some other thread to + // find the state in the cache and ref it while we're doing + // this. state->release_new(); state->remove_cache_pointers(); + state->cache_unref(); delete state; } } @@ -1607,6 +1598,12 @@ return_unique(RenderState *state) { } // Not already in the set; add it. + if (garbage_collect_states) { + // If we'll be garbage collecting states explicitly, we'll + // increment the reference count when we store it in the cache, so + // that it won't be deleted while it's in it. + state->cache_ref(); + } si = _states->store(state, Empty()); // Save the index and return the input state. diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index 0833f3e847..300571e434 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -717,25 +717,15 @@ invert_compose(const TransformState *other) const { //////////////////////////////////////////////////////////////////// bool TransformState:: unref() const { - if (!transform_cache) { - // If we're not using the cache anyway, just allow the pointer to - // unref normally. + if (!transform_cache || garbage_collect_states) { + // If we're not using the cache at all, or if we're relying on + // garbage collection, just allow the pointer to unref normally. return ReferenceCount::unref(); } - if (garbage_collect_states) { - // In the garbage collector case, we don't delete TransformStates - // immediately; instead, we allow them to remain in the cache with - // a ref count of 0, and we delete them later in - // garbage_collect(). - - ReferenceCount::unref(); - // Return true so that it is never deleted here. - return true; - } - // Here is the normal refcounting case, with a normal cache, and - // without garbage collection in effect. + // without garbage collection in effect. In this case we will pull + // the object out of the cache when its reference count goes to 0. // We always have to grab the lock, since we will definitely need to // be holding it if we happen to drop the reference count to 0. @@ -1289,7 +1279,7 @@ clear_cache() { //////////////////////////////////////////////////////////////////// int TransformState:: garbage_collect() { - if (_states == (States *)NULL) { + if (_states == (States *)NULL || !garbage_collect_states) { return 0; } LightReMutexHolder holder(*_states_lock); @@ -1323,15 +1313,16 @@ garbage_collect() { } } - if (state->get_ref_count() == 0) { - // This state has recently been unreffed to 0, but it hasn't - // been deleted yet (because we have overloaded unref(), - // above, to always return true). Now it's time to delete it. - // This is safe, because we're holding the _states_lock, so - // it's not possible for some other thread to find the state - // in the cache and ref it while we're doing this. + if (state->get_ref_count() == 1) { + // This state has recently been unreffed to 1 (the one we + // added when we stored it in the cache). Now it's time to + // delete it. This is safe, because we're holding the + // _states_lock, so it's not possible for some other thread to + // find the state in the cache and ref it while we're doing + // this. state->release_new(); state->remove_cache_pointers(); + state->cache_unref(); delete state; } } @@ -1704,6 +1695,12 @@ return_unique(TransformState *state) { } // Not already in the set; add it. + if (garbage_collect_states) { + // If we'll be garbage collecting states explicitly, we'll + // increment the reference count when we store it in the cache, so + // that it won't be deleted while it's in it. + state->cache_ref(); + } si = _states->store(state, Empty()); // Save the index and return the input state.