fix a memory leak in the garbage-collect-states case

panda/src/pgraph/renderAttrib.cxx

@@ -149,25 +149,15 @@ cull_callback(CullTraverser *, const CullTraverserData &) const {
 ////////////////////////////////////////////////////////////////////
 bool RenderAttrib::
 unref() const {
-  if (!state_cache) {
+  if (!state_cache || garbage_collect_states) {
-    // If we're not using the cache anyway, just allow the pointer to
+    // If we're not using the cache at all, or if we're relying on
-    // unref normally.
+    // 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) {
+      if (attrib->get_ref_count() == 1) {
-        // This attrib has recently been unreffed to 0, but it hasn't
+        // This attrib has recently been unreffed to 1 (the one we
-        // been deleted yet (because we have overloaded unref(),
+        // added when we stored it in the cache).  Now it's time to
-        // above, to always return true).  Now it's time to delete it.
+        // delete it.  This is safe, because we're holding the
-        // This is safe, because we're holding the _attribs_lock, so
+        // _attribs_lock, so it's not possible for some other thread to
-        // it's not possible for some other thread to find the attrib
+        // find the attrib in the cache and ref it while we're doing
-        // in the cache and ref it while we're doing this.
+        // 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.

panda/src/pgraph/renderState.cxx

@@ -682,25 +682,15 @@ adjust_all_priorities(int adjustment) const {
 ////////////////////////////////////////////////////////////////////
 bool RenderState::
 unref() const {
-  if (!state_cache) {
+  if (!state_cache || garbage_collect_states) {
-    // If we're not using the cache anyway, just allow the pointer to
+    // If we're not using the cache at all, or if we're relying on
-    // unref normally.
+    // 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) {
+      if (state->get_ref_count() == 1) {
-        // This state has recently been unreffed to 0, but it hasn't
+        // This state has recently been unreffed to 1 (the one we
-        // been deleted yet (because we have overloaded unref(),
+        // added when we stored it in the cache).  Now it's time to
-        // above, to always return true).  Now it's time to delete it.
+        // delete it.  This is safe, because we're holding the
-        // This is safe, because we're holding the _states_lock, so
+        // _states_lock, so it's not possible for some other thread to
-        // it's not possible for some other thread to find the state
+        // find the state in the cache and ref it while we're doing
-        // in the cache and ref it while we're doing this.
+        // 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.

panda/src/pgraph/transformState.cxx

@@ -717,25 +717,15 @@ invert_compose(const TransformState *other) const {
 ////////////////////////////////////////////////////////////////////
 bool TransformState::
 unref() const {
-  if (!transform_cache) {
+  if (!transform_cache || garbage_collect_states) {
-    // If we're not using the cache anyway, just allow the pointer to
+    // If we're not using the cache at all, or if we're relying on
-    // unref normally.
+    // 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) {
+      if (state->get_ref_count() == 1) {
-        // This state has recently been unreffed to 0, but it hasn't
+        // This state has recently been unreffed to 1 (the one we
-        // been deleted yet (because we have overloaded unref(),
+        // added when we stored it in the cache).  Now it's time to
-        // above, to always return true).  Now it's time to delete it.
+        // delete it.  This is safe, because we're holding the
-        // This is safe, because we're holding the _states_lock, so
+        // _states_lock, so it's not possible for some other thread to
-        // it's not possible for some other thread to find the state
+        // find the state in the cache and ref it while we're doing
-        // in the cache and ref it while we're doing this.
+        // 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.