pgraph: Rewrite inefficient prev_transform tracking mechanism

The previous system was causing a lot of lock contention when transforms are modified in the Cull thread.

The new implementation doesn't use a linked list or lock at all, but a simple atomically incrementing integer that indicates that the prev transforms have changed.  set_transform() reads this and backs up the prev transform the first time a transform is modified after reset_all_prev_transforms() is called.

panda/src/pgraph/pandaNode.I

@@ -326,7 +326,13 @@ clear_transform(Thread *current_thread) {
 INLINE CPT(TransformState) PandaNode::
 get_prev_transform(Thread *current_thread) const {
   CDReader cdata(_cycler, current_thread);
-  return cdata->_prev_transform.p();
+  if (_prev_transform_valid == _reset_prev_transform_seq) {
+    return cdata->_prev_transform.p();
+  } else {
+    // If these values are different, someone called reset_prev_transform(),
+    // and we haven't changed our transform since then.
+    return cdata->_transform.p();
+  }
 }
 
 /**
@@ -334,10 +340,19 @@ get_prev_transform(Thread *current_thread) const {
  * indicates its _prev_transform is different from its _transform value (in
  * pipeline stage 0).  In this case, the node will be visited by
  * reset_prev_transform().
+ *
+ * @deprecated Simply check prev_transform != transform instead.
  */
 INLINE bool PandaNode::
 has_dirty_prev_transform() const {
-  return _dirty_prev_transform;
+  CDStageReader cdata(_cycler, 0);
+  if (_prev_transform_valid == _reset_prev_transform_seq) {
+    return cdata->_prev_transform != cdata->_transform;
+  } else {
+    // If these values are different, someone called reset_prev_transform(),
+    // and we haven't changed our transform since then.
+    return false;
+  }
 }
 
 /**
@@ -701,34 +716,6 @@ verify_child_no_cycles(PandaNode *child_node) {
   return true;
 }
 
-/**
- * Sets the dirty_prev_transform flag, and adds the node to the
- * _dirty_prev_transforms chain.  Assumes _dirty_prev_transforms._lock is
- * already held.
- */
-INLINE void PandaNode::
-do_set_dirty_prev_transform() {
-  nassertv(_dirty_prev_transforms._lock.debug_is_locked());
-  if (!_dirty_prev_transform) {
-    LinkedListNode::insert_before(&_dirty_prev_transforms);
-    _dirty_prev_transform = true;
-  }
-}
-
-/**
- * Clears the dirty_prev_transform flag, and removes the node from the
- * _dirty_prev_transforms chain.  Assumes _dirty_prev_transforms._lock is
- * already held.
- */
-INLINE void PandaNode::
-do_clear_dirty_prev_transform() {
-  nassertv(_dirty_prev_transforms._lock.debug_is_locked());
-  if (_dirty_prev_transform) {
-    LinkedListNode::remove_from_list();
-    _dirty_prev_transform = false;
-  }
-}
-
 /**
  *
  */
@@ -1513,7 +1500,11 @@ get_transform() const {
  */
 const TransformState *PandaNodePipelineReader::
 get_prev_transform() const {
-  return _cdata->_prev_transform;
+  if (_node->_prev_transform_valid == PandaNode::_reset_prev_transform_seq) {
+    return _cdata->_prev_transform;
+  } else {
+    return _cdata->_transform;
+  }
 }
 
 /**

panda/src/pgraph/pandaNode.cxx

@@ -42,10 +42,9 @@ TypeHandle PandaNode::BamReaderAuxDataDown::_type_handle;
 
 PandaNode::SceneRootFunc *PandaNode::_scene_root_func;
 
-PandaNodeChain PandaNode::_dirty_prev_transforms("_dirty_prev_transforms");
+UpdateSeq PandaNode::_reset_prev_transform_seq;
 DrawMask PandaNode::_overall_bit = DrawMask::bit(31);
 
-PStatCollector PandaNode::_reset_prev_pcollector("App:Collisions:Reset");
 PStatCollector PandaNode::_update_bounds_pcollector("*:Bounds");
 
 TypeHandle PandaNode::_type_handle;
@@ -78,7 +77,7 @@ PandaNode::
 PandaNode(const string &name) :
   Namable(name),
   _paths_lock("PandaNode::_paths_lock"),
-  _dirty_prev_transform(false)
+  _prev_transform_valid(_reset_prev_transform_seq)
 {
   if (pgraph_cat.is_debug()) {
     pgraph_cat.debug()
@@ -100,12 +99,6 @@ PandaNode::
       << "Destructing " << (void *)this << ", " << get_name() << "\n";
   }
 
-  if (_dirty_prev_transform) {
-    // Need to have this held before we grab any other locks.
-    LightMutexHolder holder(_dirty_prev_transforms._lock);
-    do_clear_dirty_prev_transform();
-  }
-
   // We shouldn't have any parents left by the time we destruct, or there's a
   // refcount fault somewhere.
 
@@ -133,7 +126,7 @@ PandaNode(const PandaNode &copy) :
   TypedWritableReferenceCount(copy),
   Namable(copy),
   _paths_lock("PandaNode::_paths_lock"),
-  _dirty_prev_transform(false),
+  _prev_transform_valid(_reset_prev_transform_seq),
   _python_tag_data(copy._python_tag_data),
   _unexpected_change_flags(0)
 {
@@ -145,18 +138,16 @@ PandaNode(const PandaNode &copy) :
   MemoryUsage::update_type(this, this);
 #endif
 
-  // Need to have this held before we grab any other locks.
-  LightMutexHolder holder(_dirty_prev_transforms._lock);
-
   // Copy the other node's state.
   {
     CDReader copy_cdata(copy._cycler);
     CDWriter cdata(_cycler, true);
     cdata->_state = copy_cdata->_state;
     cdata->_transform = copy_cdata->_transform;
-    cdata->_prev_transform = copy_cdata->_prev_transform;
+    if (copy._prev_transform_valid == _reset_prev_transform_seq) {
-    if (cdata->_transform != cdata->_prev_transform) {
+      cdata->_prev_transform = copy_cdata->_prev_transform;
-      do_set_dirty_prev_transform();
+    } else {
+      cdata->_prev_transform = copy_cdata->_transform;
     }
 
     cdata->_effects = copy_cdata->_effects;
@@ -1061,24 +1052,23 @@ void PandaNode::
 set_transform(const TransformState *transform, Thread *current_thread) {
   nassertv(!transform->is_invalid());
 
-  // Need to have this held before we grab any other locks.
-  LightMutexHolder holder(_dirty_prev_transforms._lock);
-
   // Apply this operation to the current stage as well as to all upstream
   // stages.
   bool any_changed = false;
   OPEN_ITERATE_CURRENT_AND_UPSTREAM(_cycler, current_thread) {
     CDStageWriter cdata(_cycler, pipeline_stage, current_thread);
     if (cdata->_transform != transform) {
+      if (pipeline_stage == 0) {
+        // Back up the previous transform.
+        if (_prev_transform_valid != _reset_prev_transform_seq) {
+          cdata->_prev_transform = std::move(cdata->_transform);
+          _prev_transform_valid = _reset_prev_transform_seq;
+        }
+      }
+
       cdata->_transform = transform;
       cdata->set_fancy_bit(FB_transform, !transform->is_identity());
       any_changed = true;
-
-      if (pipeline_stage == 0) {
-        if (cdata->_transform != cdata->_prev_transform) {
-          do_set_dirty_prev_transform();
-        }
-      }
     }
   }
   CLOSE_ITERATE_CURRENT_AND_UPSTREAM(_cycler);
@@ -1099,20 +1089,13 @@ void PandaNode::
 set_prev_transform(const TransformState *transform, Thread *current_thread) {
   nassertv(!transform->is_invalid());
 
-  // Need to have this held before we grab any other locks.
-  LightMutexHolder holder(_dirty_prev_transforms._lock);
-
   // Apply this operation to the current stage as well as to all upstream
   // stages.
   OPEN_ITERATE_CURRENT_AND_UPSTREAM(_cycler, current_thread) {
     CDStageWriter cdata(_cycler, pipeline_stage, current_thread);
     cdata->_prev_transform = transform;
     if (pipeline_stage == 0) {
-      if (cdata->_transform != cdata->_prev_transform) {
+      _prev_transform_valid = _reset_prev_transform_seq;
-        do_set_dirty_prev_transform();
-      } else {
-        do_clear_dirty_prev_transform();
-      }
     }
   }
   CLOSE_ITERATE_CURRENT_AND_UPSTREAM(_cycler);
@@ -1126,10 +1109,6 @@ set_prev_transform(const TransformState *transform, Thread *current_thread) {
  */
 void PandaNode::
 reset_prev_transform(Thread *current_thread) {
-  // Need to have this held before we grab any other locks.
-  LightMutexHolder holder(_dirty_prev_transforms._lock);
-  do_clear_dirty_prev_transform();
-
   // Apply this operation to the current stage as well as to all upstream
   // stages.
 
@@ -1138,41 +1117,22 @@ reset_prev_transform(Thread *current_thread) {
     cdata->_prev_transform = cdata->_transform;
   }
   CLOSE_ITERATE_CURRENT_AND_UPSTREAM(_cycler);
-  mark_bam_modified();
 }
 
 /**
- * Visits all nodes in the world with the _dirty_prev_transform flag--that is,
+ * Makes sure that all nodes reset their prev_transform value to be the same as
- * all nodes whose _prev_transform is different from the _transform in
+ * their transform value.  This should be called at the start of each frame.
- * pipeline stage 0--and resets the _prev_transform to be the same as
- * _transform.
  */
 void PandaNode::
 reset_all_prev_transform(Thread *current_thread) {
   nassertv(current_thread->get_pipeline_stage() == 0);
 
-  PStatTimer timer(_reset_prev_pcollector, current_thread);
+  // Rather than keeping a linked list of all nodes that have changed their
-  LightMutexHolder holder(_dirty_prev_transforms._lock);
+  // transform, we simply increment this counter.  All the nodes compare this
-
+  // value to their own _prev_transform_valid value, and if it's different,
-  LinkedListNode *list_node = _dirty_prev_transforms._next;
+  // they should disregard their _prev_transform field and assume it's the same
-  while (list_node != &_dirty_prev_transforms) {
+  // as their _transform.
-    PandaNode *panda_node = (PandaNode *)list_node;
+  ++_reset_prev_transform_seq;
-    nassertv(panda_node->_dirty_prev_transform);
-    panda_node->_dirty_prev_transform = false;
-
-    CDStageWriter cdata(panda_node->_cycler, 0, current_thread);
-    cdata->_prev_transform = cdata->_transform;
-
-    list_node = panda_node->_next;
-#ifndef NDEBUG
-    panda_node->_prev = nullptr;
-    panda_node->_next = nullptr;
-#endif  // NDEBUG
-    panda_node->mark_bam_modified();
-  }
-
-  _dirty_prev_transforms._prev = &_dirty_prev_transforms;
-  _dirty_prev_transforms._next = &_dirty_prev_transforms;
 }
 
 /**
@@ -1345,9 +1305,6 @@ copy_all_properties(PandaNode *other) {
     return;
   }
 
-  // Need to have this held before we grab any other locks.
-  LightMutexHolder holder(_dirty_prev_transforms._lock);
-
   bool any_transform_changed = false;
   bool any_state_changed = false;
   bool any_draw_mask_changed = false;
@@ -1368,7 +1325,11 @@ copy_all_properties(PandaNode *other) {
     }
 
     cdataw->_transform = cdatar->_transform;
-    cdataw->_prev_transform = cdatar->_prev_transform;
+    if (other->_prev_transform_valid == _reset_prev_transform_seq) {
+      cdataw->_prev_transform = cdatar->_prev_transform;
+    } else {
+      cdataw->_prev_transform = cdatar->_transform;
+    }
     cdataw->_state = cdatar->_state;
     cdataw->_effects = cdatar->_effects;
     cdataw->_draw_control_mask = cdatar->_draw_control_mask;
@@ -1390,9 +1351,7 @@ copy_all_properties(PandaNode *other) {
       (cdatar->_fancy_bits & change_bits);
 
     if (pipeline_stage == 0) {
-      if (cdataw->_transform != cdataw->_prev_transform) {
+      _prev_transform_valid = _reset_prev_transform_seq;
-        do_set_dirty_prev_transform();
-      }
     }
   }
   CLOSE_ITERATE_CURRENT_AND_UPSTREAM(_cycler);

panda/src/pgraph/pandaNode.h

@@ -63,7 +63,7 @@ class GraphicsStateGuardianBase;
  * properties.
  */
 class EXPCL_PANDA_PGRAPH PandaNode : public TypedWritableReferenceCount,
-                                     public Namable, public LinkedListNode {
+                                     public Namable {
 PUBLISHED:
   explicit PandaNode(const std::string &name);
   virtual ~PandaNode();
@@ -449,9 +449,6 @@ private:
   void fix_path_lengths(int pipeline_stage, Thread *current_thread);
   void r_list_descendants(std::ostream &out, int indent_level) const;
 
-  INLINE void do_set_dirty_prev_transform();
-  INLINE void do_clear_dirty_prev_transform();
-
 public:
   // This must be declared public so that VC6 will allow the nested CData
   // class to access it.
@@ -540,8 +537,10 @@ private:
   Paths _paths;
   LightReMutex _paths_lock;
 
-  bool _dirty_prev_transform;
+  // This is not part of CData because we only care about modifications to the
-  static PandaNodeChain _dirty_prev_transforms;
+  // transform in the App stage.
+  UpdateSeq _prev_transform_valid;
+  static UpdateSeq _reset_prev_transform_seq;
 
   // This is used to maintain a table of keyed data on each node, for the
   // user's purposes.
@@ -713,7 +712,6 @@ private:
 
   static DrawMask _overall_bit;
 
-  static PStatCollector _reset_prev_pcollector;
   static PStatCollector _update_bounds_pcollector;
 
 PUBLISHED: