Browse Source

pgraph: Change TransformState hashing not to require grabbing lock

Instead, set it atomically, with a special hash value used instead of a flag for indicating that the hash is not known.
rdb 4 years ago
parent
commit
9ebfef4b83

+ 2 - 11
panda/src/pgraph/transformState.I

@@ -856,8 +856,8 @@ INLINE void TransformState::
 check_hash() const {
 check_hash() const {
   // This pretends to be a const function, even though it's not, because it
   // This pretends to be a const function, even though it's not, because it
   // only updates a transparent cache value.
   // only updates a transparent cache value.
-  if ((_flags & F_hash_known) == 0) {
-    ((TransformState *)this)->calc_hash();
+  if (_hash == H_unknown) {
+    calc_hash();
   }
   }
 }
 }
 
 
@@ -937,15 +937,6 @@ check_mat() const {
   }
   }
 }
 }
 
 
-/**
- * Computes the hash value.
- */
-INLINE void TransformState::
-calc_hash() {
-  LightMutexHolder holder(_lock);
-  do_calc_hash();
-}
-
 /**
 /**
  * Derives the components from the matrix, if possible.
  * Derives the components from the matrix, if possible.
  */
  */

+ 33 - 26
panda/src/pgraph/transformState.cxx

@@ -1406,10 +1406,10 @@ init_states() {
                     0.0f, 0.0f, 1.0f, 0.0f,
                     0.0f, 0.0f, 1.0f, 0.0f,
                     0.0f, 0.0f, 0.0f, 1.0f);
                     0.0f, 0.0f, 0.0f, 1.0f);
     state->_inv_mat = new LMatrix4(state->_mat);
     state->_inv_mat = new LMatrix4(state->_mat);
-    state->_hash = int_hash::add_hash(0, F_is_invalid | F_is_identity | F_is_2d);
+    state->_hash = H_identity;
     state->_flags = F_is_identity | F_singular_known | F_components_known
     state->_flags = F_is_identity | F_singular_known | F_components_known
                   | F_has_components | F_mat_known | F_quat_known | F_hpr_known
                   | F_has_components | F_mat_known | F_quat_known | F_hpr_known
-                  | F_uniform_scale | F_identity_scale | F_is_2d | F_hash_known
+                  | F_uniform_scale | F_identity_scale | F_is_2d
                   | F_norm_quat_known;
                   | F_norm_quat_known;
     state->cache_ref();
     state->cache_ref();
     state->_saved_entry = _states.store(state, nullptr);
     state->_saved_entry = _states.store(state, nullptr);
@@ -1417,9 +1417,9 @@ init_states() {
   }
   }
   {
   {
     TransformState *state = new TransformState;
     TransformState *state = new TransformState;
-    state->_hash = int_hash::add_hash(0, F_is_invalid);
-    state->_flags = F_is_singular | F_singular_known  | F_components_known
-                  | F_mat_known | F_is_invalid | F_hash_known;
+    state->_hash = H_invalid;
+    state->_flags = F_is_singular | F_singular_known | F_components_known
+                  | F_mat_known | F_is_invalid;
     state->cache_ref();
     state->cache_ref();
     state->_saved_entry = _states.store(state, nullptr);
     state->_saved_entry = _states.store(state, nullptr);
     _invalid_state = state;
     _invalid_state = state;
@@ -2002,15 +2002,18 @@ remove_cache_pointers() {
  * Computes a suitable hash value for phash_map.
  * Computes a suitable hash value for phash_map.
  */
  */
 void TransformState::
 void TransformState::
-do_calc_hash() {
+calc_hash() const {
+  // It's OK not to grab the lock here, because (1) this does not depend on
+  // cached values (only given components are considered), and (2) the hash
+  // itself is set atomically.
   PStatTimer timer(_transform_hash_pcollector);
   PStatTimer timer(_transform_hash_pcollector);
-  _hash = 0;
+  AtomicAdjust::Integer hash = 0;
 
 
   static const int significant_flags =
   static const int significant_flags =
     (F_is_invalid | F_is_identity | F_components_given | F_hpr_given | F_is_2d);
     (F_is_invalid | F_is_identity | F_components_given | F_hpr_given | F_is_2d);
 
 
   int flags = (_flags & significant_flags);
   int flags = (_flags & significant_flags);
-  _hash = int_hash::add_hash(_hash, flags);
+  hash = int_hash::add_hash(hash, flags);
 
 
   nassertv((flags & (F_is_invalid | F_is_identity)) == 0);
   nassertv((flags & (F_is_invalid | F_is_identity)) == 0);
 
 
@@ -2019,37 +2022,41 @@ do_calc_hash() {
 
 
   if ((_flags & F_components_given) != 0) {
   if ((_flags & F_components_given) != 0) {
     // If the transform was specified componentwise, hash it componentwise.
     // If the transform was specified componentwise, hash it componentwise.
-    _hash = _pos.add_hash(_hash);
+    hash = _pos.add_hash(hash);
     if ((_flags & F_hpr_given) != 0) {
     if ((_flags & F_hpr_given) != 0) {
-      _hash = _hpr.add_hash(_hash);
-
-    } else if ((_flags & F_quat_given) != 0) {
-      _hash = _quat.add_hash(_hash);
+      hash = _hpr.add_hash(hash);
+    }
+    else if ((_flags & F_quat_given) != 0) {
+      hash = _quat.add_hash(hash);
     }
     }
 
 
-    _hash = _scale.add_hash(_hash);
-    _hash = _shear.add_hash(_hash);
-
-  } else {
+    hash = _scale.add_hash(hash);
+    hash = _shear.add_hash(hash);
+  }
+  else {
     // Otherwise, hash the matrix . . .
     // Otherwise, hash the matrix . . .
     if (_uniquify_matrix) {
     if (_uniquify_matrix) {
       // . . . but only if the user thinks that's worthwhile.
       // . . . but only if the user thinks that's worthwhile.
-      if ((_flags & F_mat_known) == 0) {
-        // Calculate the matrix without doubly-locking.
-        do_calc_mat();
-      }
-      _hash = _mat.add_hash(_hash);
-
-    } else {
+      check_mat();
+      hash = _mat.add_hash(hash);
+    }
+    else {
       // Otherwise, hash the pointer only--any two different matrix-based
       // Otherwise, hash the pointer only--any two different matrix-based
       // TransformStates are considered to be different, even if their
       // TransformStates are considered to be different, even if their
       // matrices have the same values.
       // matrices have the same values.
 
 
-      _hash = pointer_hash::add_hash(_hash, this);
+      hash = pointer_hash::add_hash(hash, this);
     }
     }
   }
   }
 
 
-  _flags |= F_hash_known;
+  if (hash == H_unknown || hash == H_identity || hash == H_invalid) {
+    // Arbitrarily offset the hash not to conflict with these special values.
+    hash += 0x10000;
+  }
+
+  // We don't care if some other thread set this in the meantime, since every
+  // thread should have computed the same hash.
+  AtomicAdjust::set(_hash, hash);
 }
 }
 
 
 /**
 /**

+ 8 - 4
panda/src/pgraph/transformState.h

@@ -326,8 +326,7 @@ private:
   INLINE void check_quat() const;
   INLINE void check_quat() const;
   INLINE void check_norm_quat() const;
   INLINE void check_norm_quat() const;
   INLINE void check_mat() const;
   INLINE void check_mat() const;
-  INLINE void calc_hash();
-  void do_calc_hash();
+  void calc_hash() const;
   void calc_singular();
   void calc_singular();
   INLINE void calc_components();
   INLINE void calc_components();
   void do_calc_components();
   void do_calc_components();
@@ -365,7 +364,6 @@ private:
     F_has_nonzero_shear  = 0x00004000,
     F_has_nonzero_shear  = 0x00004000,
     F_is_destructing     = 0x00008000,
     F_is_destructing     = 0x00008000,
     F_is_2d              = 0x00010000,
     F_is_2d              = 0x00010000,
-    F_hash_known         = 0x00020000,
     F_norm_quat_known    = 0x00040000,
     F_norm_quat_known    = 0x00040000,
   };
   };
   LPoint3 _pos;
   LPoint3 _pos;
@@ -373,7 +371,13 @@ private:
   LQuaternion _quat, _norm_quat;
   LQuaternion _quat, _norm_quat;
   LMatrix4 _mat;
   LMatrix4 _mat;
   LMatrix4 *_inv_mat = nullptr;
   LMatrix4 *_inv_mat = nullptr;
-  size_t _hash;
+
+  enum HashValue : AtomicAdjust::Integer {
+    H_unknown = 0,
+    H_identity = 1,
+    H_invalid = 2,
+  };
+  mutable AtomicAdjust::Integer _hash = H_unknown;
 
 
   unsigned int _flags;
   unsigned int _flags;