From 8567173ece9d13ad524f624d9fcf3bceae91d186 Mon Sep 17 00:00:00 2001 From: David Rose Date: Mon, 16 Jan 2006 15:42:28 +0000 Subject: [PATCH] more threading issues --- panda/src/display/graphicsThreadingModel.cxx | 2 +- panda/src/display/graphicsWindow.cxx | 24 +-- panda/src/display/graphicsWindow.h | 3 +- panda/src/express/pmutex.cxx | 2 +- panda/src/express/reMutex.I | 14 ++ panda/src/express/reMutex.h | 4 + panda/src/pgraph/config_pgraph.cxx | 7 + panda/src/pgraph/renderEffects.cxx | 170 +++++++++++-------- panda/src/pgraph/renderEffects.h | 7 + panda/src/pgraph/renderState.cxx | 88 +++++++--- panda/src/pgraph/renderState.h | 8 + panda/src/pgraph/transformState.cxx | 82 +++++++-- panda/src/pgraph/transformState.h | 8 + panda/src/pstatclient/pStatClient.I | 7 + panda/src/pstatclient/pStatClient.cxx | 32 ++++ panda/src/pstatclient/pStatClient.h | 6 + panda/src/windisplay/winGraphicsWindow.cxx | 32 ++-- 17 files changed, 359 insertions(+), 137 deletions(-) diff --git a/panda/src/display/graphicsThreadingModel.cxx b/panda/src/display/graphicsThreadingModel.cxx index 6b05b12579..92dc0a799d 100644 --- a/panda/src/display/graphicsThreadingModel.cxx +++ b/panda/src/display/graphicsThreadingModel.cxx @@ -32,7 +32,7 @@ // Thus, for example, "cull/draw" indicates that the // window will be culled in a thread called "cull", and // drawn in a separate thread called "draw". -// "draw/draw" or simply "draw/" indicates the window +// "draw/draw" or simply "draw" indicates the window // will be culled and drawn in the same thread, "draw". // On the other hand, "/draw" indicates the thread will // be culled in the main, or app thread, and drawn in a diff --git a/panda/src/display/graphicsWindow.cxx b/panda/src/display/graphicsWindow.cxx index ba3ad9f8c5..8d1c514f78 100644 --- a/panda/src/display/graphicsWindow.cxx +++ b/panda/src/display/graphicsWindow.cxx @@ -22,6 +22,7 @@ #include "mouseButton.h" #include "keyboardButton.h" #include "mutexHolder.h" +#include "reMutexHolder.h" #include "throw_event.h" TypeHandle GraphicsWindow::_type_handle; @@ -77,7 +78,7 @@ WindowProperties GraphicsWindow:: get_properties() const { WindowProperties result; { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); result = _properties; } return result; @@ -95,7 +96,7 @@ WindowProperties GraphicsWindow:: get_requested_properties() const { WindowProperties result; { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); result = _requested_properties; } return result; @@ -109,7 +110,7 @@ get_requested_properties() const { //////////////////////////////////////////////////////////////////// void GraphicsWindow:: clear_rejected_properties() { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); _rejected_properties.clear(); } @@ -126,7 +127,7 @@ WindowProperties GraphicsWindow:: get_rejected_properties() const { WindowProperties result; { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); result = _rejected_properties; } return result; @@ -145,7 +146,7 @@ get_rejected_properties() const { //////////////////////////////////////////////////////////////////// void GraphicsWindow:: request_properties(const WindowProperties &requested_properties) { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); _requested_properties.add_properties(requested_properties); if (!_has_size && _requested_properties.has_size()) { @@ -186,7 +187,7 @@ is_active() const { //////////////////////////////////////////////////////////////////// void GraphicsWindow:: set_window_event(const string &window_event) { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); _window_event = window_event; } @@ -200,7 +201,7 @@ set_window_event(const string &window_event) { string GraphicsWindow:: get_window_event() const { string result; - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); result = _window_event; return result; } @@ -231,7 +232,7 @@ get_window_event() const { //////////////////////////////////////////////////////////////////// void GraphicsWindow:: set_close_request_event(const string &close_request_event) { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); _close_request_event = close_request_event; } @@ -247,7 +248,7 @@ set_close_request_event(const string &close_request_event) { string GraphicsWindow:: get_close_request_event() const { string result; - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); result = _close_request_event; return result; } @@ -495,7 +496,7 @@ process_events() { // bitmask after all. WindowProperties properties; { - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); properties = _requested_properties; _requested_properties.clear(); @@ -534,6 +535,7 @@ set_properties_now(WindowProperties &properties) { properties.get_open() != _properties.get_open()) { // Open or close a new window. In this case we can get all of the // properties at once. + _properties.add_properties(properties); properties.clear(); @@ -687,7 +689,7 @@ system_changed_properties(const WindowProperties &properties) { << "system_changed_properties(" << properties << ")\n"; } - MutexHolder holder(_properties_lock); + ReMutexHolder holder(_properties_lock); if (properties.has_size()) { system_changed_size(properties.get_x_size(), properties.get_y_size()); diff --git a/panda/src/display/graphicsWindow.h b/panda/src/display/graphicsWindow.h index 08b7f70f3e..10680e1cd1 100644 --- a/panda/src/display/graphicsWindow.h +++ b/panda/src/display/graphicsWindow.h @@ -29,6 +29,7 @@ #include "buttonEvent.h" #include "notify.h" #include "pmutex.h" +#include "remutex.h" #include "pvector.h" //////////////////////////////////////////////////////////////////// @@ -110,7 +111,7 @@ protected: WindowProperties _properties; private: - Mutex _properties_lock; + ReMutex _properties_lock; // protects _requested_properties, _rejected_properties, and // _window_event. diff --git a/panda/src/express/pmutex.cxx b/panda/src/express/pmutex.cxx index 67130e093b..46340d9e01 100644 --- a/panda/src/express/pmutex.cxx +++ b/panda/src/express/pmutex.cxx @@ -32,7 +32,7 @@ bool Mutex:: debug_is_locked() const { return (_locking_thread == Thread::get_current_thread()); } -#endif +#endif // NDEBUG //////////////////////////////////////////////////////////////////// // Function: Mutex::do_lock diff --git a/panda/src/express/reMutex.I b/panda/src/express/reMutex.I index 332874e4b1..2b4a224929 100644 --- a/panda/src/express/reMutex.I +++ b/panda/src/express/reMutex.I @@ -121,3 +121,17 @@ get_lock_count() const { } return 0; } + +#ifndef NDEBUG +//////////////////////////////////////////////////////////////////// +// Function: ReMutex::debug_is_locked +// Access: Public +// Description: The same as is_locked(). This method name is +// provided just to maintain symmetry with Mutex (which +// doesn't provide an is_locked() method). +//////////////////////////////////////////////////////////////////// +INLINE bool ReMutex:: +debug_is_locked() const { + return is_locked(); +} +#endif // NDEBUG diff --git a/panda/src/express/reMutex.h b/panda/src/express/reMutex.h index 52dedf60ba..a585f3ab0b 100644 --- a/panda/src/express/reMutex.h +++ b/panda/src/express/reMutex.h @@ -48,6 +48,10 @@ public: INLINE bool is_locked() const; INLINE int get_lock_count() const; +#ifndef NDEBUG + INLINE bool debug_is_locked() const; +#endif + private: void do_lock(); void do_release(); diff --git a/panda/src/pgraph/config_pgraph.cxx b/panda/src/pgraph/config_pgraph.cxx index 4f3aa147d3..b4584f63ec 100644 --- a/panda/src/pgraph/config_pgraph.cxx +++ b/panda/src/pgraph/config_pgraph.cxx @@ -397,6 +397,13 @@ init_libpgraph() { TransformState::register_with_read_factory(); TransparencyAttrib::register_with_read_factory(); + // By initializing the _states map up front, we also guarantee that + // the _states_lock mutex gets created before we spawn any threads + // (assuming no one is creating threads at static init time). + TransformState::init_states(); + RenderState::init_states(); + RenderEffects::init_states(); + LoaderFileTypeRegistry *reg = LoaderFileTypeRegistry::get_global_ptr(); reg->register_type(new LoaderFileTypeBam); } diff --git a/panda/src/pgraph/renderEffects.cxx b/panda/src/pgraph/renderEffects.cxx index 35a0c3c1d6..fba5d5680f 100644 --- a/panda/src/pgraph/renderEffects.cxx +++ b/panda/src/pgraph/renderEffects.cxx @@ -28,7 +28,10 @@ #include "datagramIterator.h" #include "indent.h" #include "compareTo.h" - +#include "reMutexHolder.h" +#include "thread.h" + +ReMutex *RenderEffects::_states_lock = NULL; RenderEffects::States *RenderEffects::_states = NULL; CPT(RenderEffects) RenderEffects::_empty_state; TypeHandle RenderEffects::_type_handle; @@ -43,12 +46,7 @@ TypeHandle RenderEffects::_type_handle; RenderEffects:: RenderEffects() { 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; + init_states(); } _saved_entry = _states->end(); _flags = 0; @@ -83,6 +81,7 @@ operator = (const RenderEffects &) { RenderEffects:: ~RenderEffects() { // Remove the deleted RenderEffects object from the global pool. + ReMutexHolder holder(*_states_lock); if (_saved_entry != _states->end()) { _states->erase(_saved_entry); _saved_entry = _states->end(); @@ -423,6 +422,72 @@ write(ostream &out, int indent_level) const { } } +//////////////////////////////////////////////////////////////////// +// 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; + } + ReMutexHolder holder(*_states_lock); + 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; + } + ReMutexHolder holder(*_states_lock); + + 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::cull_callback // Access: Public @@ -460,70 +525,30 @@ adjust_transform(CPT(TransformState) &net_transform, (*ei)._effect->adjust_transform(net_transform, node_transform); } } - + //////////////////////////////////////////////////////////////////// -// 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. +// Function: RenderEffects::init_states +// Access: Public, Static +// Description: 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 RenderEffects object at static init time; +// it also seems to cause problems when the Panda shared +// library is unloaded at application exit time. //////////////////////////////////////////////////////////////////// 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; +init_states() { + _states = new States; + + // TODO: we should have a global Panda mutex to allow us to safely + // create _states_lock without a startup race condition. For the + // meantime, this is OK because we guarantee that this method is + // called at static init time, presumably when there is still only + // one thread in the world. + _states_lock = new ReMutex; + nassertv(Thread::get_current_thread() == Thread::get_main_thread()); } + //////////////////////////////////////////////////////////////////// // Function: RenderEffects::return_new @@ -541,16 +566,18 @@ CPT(RenderEffects) RenderEffects:: return_new(RenderEffects *state) { nassertr(state != (RenderEffects *)NULL, state); - // This should be a newly allocated pointer, not one that was used - // for anything else. - nassertr(state->_saved_entry == _states->end(), state); - #ifndef NDEBUG if (paranoid_const) { nassertr(validate_states(), state); } #endif + ReMutexHolder holder(*_states_lock); + + // This should be a newly allocated pointer, not one that was used + // for anything else. + nassertr(state->_saved_entry == _states->end(), state); + // 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; @@ -689,6 +716,7 @@ complete_pointers(TypedWritable **p_list, BamReader *manager) { nassertr(effect._effect != (RenderEffect *)NULL, pi); effect._type = effect._effect->get_type(); } + ReMutexHolder holder(*_states_lock); // Now make sure the array is properly sorted. (It won't // necessarily preserve its correct sort after being read from bam, diff --git a/panda/src/pgraph/renderEffects.h b/panda/src/pgraph/renderEffects.h index 0291ddcbdb..8cffde9c1e 100644 --- a/panda/src/pgraph/renderEffects.h +++ b/panda/src/pgraph/renderEffects.h @@ -28,6 +28,7 @@ #include "typedWritableReferenceCount.h" #include "pointerTo.h" #include "ordered_vector.h" +#include "reMutex.h" class CullTraverser; class CullTraverserData; @@ -108,6 +109,8 @@ public: virtual void adjust_transform(CPT(TransformState) &net_transform, CPT(TransformState) &node_transform) const; + static void init_states(); + private: static CPT(RenderEffects) return_new(RenderEffects *state); void determine_decal(); @@ -116,6 +119,10 @@ private: void determine_adjust_transform(); private: + // This mutex protects _states. It also protects any modification + // to the cache, which is encoded in _composition_cache and + // _invert_composition_cache. + static ReMutex *_states_lock; typedef pset > States; static States *_states; static CPT(RenderEffects) _empty_state; diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index d4bb360524..ad9ea01b72 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -35,7 +35,10 @@ #include "datagramIterator.h" #include "indent.h" #include "compareTo.h" - +#include "reMutexHolder.h" +#include "thread.h" + +ReMutex *RenderState::_states_lock = NULL; RenderState::States *RenderState::_states = NULL; CPT(RenderState) RenderState::_empty_state; UpdateSeq RenderState::_last_cycle_detect; @@ -59,12 +62,7 @@ TypeHandle RenderState::_type_handle; RenderState:: RenderState() { 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; + init_states(); } _saved_entry = _states->end(); _flags = 0; @@ -103,12 +101,13 @@ RenderState:: nassertv(!is_destructing()); set_destructing(); + ReMutexHolder holder(*_states_lock); if (_saved_entry != _states->end()) { nassertv(_states->find(this) == _saved_entry); _states->erase(_saved_entry); _saved_entry = _states->end(); } - + remove_cache_pointers(); // If this was true at the beginning of the destructor, but is no @@ -319,6 +318,8 @@ compose(const RenderState *other) const { return this; } + ReMutexHolder holder(*_states_lock); + // Is this composition already cached? CompositionCache::const_iterator ci = _composition_cache.find(other); if (ci != _composition_cache.end()) { @@ -397,6 +398,8 @@ invert_compose(const RenderState *other) const { return make_empty(); } + ReMutexHolder holder(*_states_lock); + // Is this composition already cached? CompositionCache::const_iterator ci = _invert_composition_cache.find(other); if (ci != _invert_composition_cache.end()) { @@ -590,6 +593,8 @@ get_override(TypeHandle type) const { //////////////////////////////////////////////////////////////////// int RenderState:: unref() const { + ReMutexHolder holder(*_states_lock); + if (get_cache_ref_count() > 0 && get_ref_count() == get_cache_ref_count() + 1) { // If we are about to remove the one reference that is not in the @@ -598,11 +603,6 @@ unref() const { // it exists. if (auto_break_cycles) { - // There might be a tiny race condition if multiple different - // threads perform cycle detects on related nodes at the same - // time. But the cost of failing the race condition is low--we - // end up with a tiny leak that may eventually be discovered, big - // deal. ++_last_cycle_detect; if (r_detect_cycles(this, this, 1, _last_cycle_detect, NULL)) { // Ok, we have a cycle. This will be a leak unless we break the @@ -611,6 +611,7 @@ unref() const { pgraph_cat.debug() << "Breaking cycle involving " << (*this) << "\n"; } + ((RenderState *)this)->remove_cache_pointers(); } } @@ -684,6 +685,7 @@ get_num_states() { if (_states == (States *)NULL) { return 0; } + ReMutexHolder holder(*_states_lock); return _states->size(); } @@ -710,6 +712,7 @@ get_num_unused_states() { if (_states == (States *)NULL) { return 0; } + ReMutexHolder holder(*_states_lock); // First, we need to count the number of times each RenderState // object is recorded in the cache. @@ -802,6 +805,7 @@ clear_cache() { if (_states == (States *)NULL) { return 0; } + ReMutexHolder holder(*_states_lock); PStatTimer timer(_cache_update_pcollector); int orig_size = _states->size(); @@ -878,6 +882,11 @@ clear_cache() { //////////////////////////////////////////////////////////////////// void RenderState:: list_cycles(ostream &out) { + if (_states == (States *)NULL) { + return; + } + ReMutexHolder holder(*_states_lock); + typedef pset VisitedStates; VisitedStates visited; CompositionCycleDesc cycle_desc; @@ -928,6 +937,12 @@ list_cycles(ostream &out) { //////////////////////////////////////////////////////////////////// void RenderState:: list_states(ostream &out) { + if (_states == (States *)NULL) { + out << "0 states:\n"; + return; + } + ReMutexHolder holder(*_states_lock); + out << _states->size() << " states:\n"; States::const_iterator si; for (si = _states->begin(); si != _states->end(); ++si) { @@ -948,6 +963,11 @@ list_states(ostream &out) { //////////////////////////////////////////////////////////////////// bool RenderState:: validate_states() { + if (_states == (States *)NULL) { + return true; + } + + ReMutexHolder holder(*_states_lock); if (_states->empty()) { return true; } @@ -1021,7 +1041,7 @@ bin_removed(int bin_index) { // Do something here. nassertv(false); } - + //////////////////////////////////////////////////////////////////// // Function: RenderState::return_new // Access: Private, Static @@ -1038,16 +1058,18 @@ CPT(RenderState) RenderState:: return_new(RenderState *state) { nassertr(state != (RenderState *)NULL, state); - // This should be a newly allocated pointer, not one that was used - // for anything else. - nassertr(state->_saved_entry == _states->end(), state); - #ifndef NDEBUG if (paranoid_const) { nassertr(validate_states(), state); } #endif + ReMutexHolder holder(*_states_lock); + + // This should be a newly allocated pointer, not one that was used + // for anything else. + nassertr(state->_saved_entry == _states->end(), state); + // 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; @@ -1273,12 +1295,13 @@ r_detect_cycles(const RenderState *start_state, // particular RenderState. The pointers to this // object may be scattered around in the various // CompositionCaches from other RenderState objects. +// +// You must already be holding _states_lock before you +// call this method. //////////////////////////////////////////////////////////////////// void RenderState:: remove_cache_pointers() { - // Now make sure we clean up all other floating pointers to the - // RenderState. These may be scattered around in the various - // CompositionCaches from other RenderState objects. + nassertv(_states_lock->debug_is_locked()); // Fortunately, since we added CompositionCache records in pairs, we // know exactly the set of RenderState objects that have us in their @@ -1633,6 +1656,29 @@ update_pstats(int old_referenced_bits, int new_referenced_bits) { } } +//////////////////////////////////////////////////////////////////// +// Function: RenderState::init_states +// Access: Public, Static +// Description: 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. +//////////////////////////////////////////////////////////////////// +void RenderState:: +init_states() { + _states = new States; + + // TODO: we should have a global Panda mutex to allow us to safely + // create _states_lock without a startup race condition. For the + // meantime, this is OK because we guarantee that this method is + // called at static init time, presumably when there is still only + // one thread in the world. + _states_lock = new ReMutex; + nassertv(Thread::get_current_thread() == Thread::get_main_thread()); +} + //////////////////////////////////////////////////////////////////// // Function: RenderState::register_with_read_factory diff --git a/panda/src/pgraph/renderState.h b/panda/src/pgraph/renderState.h index 4e585dbdcb..3ff2b0c5bb 100644 --- a/panda/src/pgraph/renderState.h +++ b/panda/src/pgraph/renderState.h @@ -32,6 +32,7 @@ #include "geomMunger.h" #include "weakPointerTo.h" #include "shaderExpansion.h" +#include "reMutex.h" class GraphicsStateGuardianBase; class FogAttrib; @@ -195,7 +196,14 @@ private: INLINE void consider_update_pstats(int old_referenced_bits) const; static void update_pstats(int old_referenced_bits, int new_referenced_bits); +public: + static void init_states(); + private: + // This mutex protects _states. It also protects any modification + // to the cache, which is encoded in _composition_cache and + // _invert_composition_cache. + static ReMutex *_states_lock; typedef pset > States; static States *_states; static CPT(RenderState) _empty_state; diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index 53ac761ef0..ba600aac9b 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -25,7 +25,10 @@ #include "compareTo.h" #include "pStatTimer.h" #include "config_pgraph.h" +#include "reMutexHolder.h" +#include "thread.h" +ReMutex *TransformState::_states_lock = NULL; TransformState::States *TransformState::_states = NULL; CPT(TransformState) TransformState::_identity_state; UpdateSeq TransformState::_last_cycle_detect; @@ -47,12 +50,7 @@ TypeHandle TransformState::_type_handle; TransformState:: TransformState() { 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 TransformState 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; + init_states(); } _saved_entry = _states->end(); _flags = F_is_identity | F_singular_known | F_is_2d; @@ -97,12 +95,13 @@ TransformState:: _inv_mat = (LMatrix4f *)NULL; } + ReMutexHolder holder(*_states_lock); if (_saved_entry != _states->end()) { nassertv(_states->find(this) == _saved_entry); _states->erase(_saved_entry); _saved_entry = _states->end(); } - + remove_cache_pointers(); // If this was true at the beginning of the destructor, but is no @@ -631,6 +630,8 @@ compose(const TransformState *other) const { return other; } + ReMutexHolder holder(*_states_lock); + // Is this composition already cached? CompositionCache::const_iterator ci = _composition_cache.find(other); if (ci != _composition_cache.end()) { @@ -717,6 +718,8 @@ invert_compose(const TransformState *other) const { return make_identity(); } + ReMutexHolder holder(*_states_lock); + // Is this composition already cached? CompositionCache::const_iterator ci = _invert_composition_cache.find(other); if (ci != _invert_composition_cache.end()) { @@ -784,6 +787,8 @@ invert_compose(const TransformState *other) const { //////////////////////////////////////////////////////////////////// int TransformState:: unref() const { + ReMutexHolder holder(*_states_lock); + if (get_cache_ref_count() > 0 && get_ref_count() == get_cache_ref_count() + 1) { // If we are about to remove the one reference that is not in the @@ -792,11 +797,6 @@ unref() const { // it exists. if (auto_break_cycles) { - // There might be a tiny race condition if multiple different - // threads perform cycle detects on related nodes at the same - // time. But the cost of failing the race condition is low--we - // end up with a tiny leak that may eventually be discovered, big - // deal. ++_last_cycle_detect; if (r_detect_cycles(this, this, 1, _last_cycle_detect, NULL)) { // Ok, we have a cycle. This will be a leak unless we break the @@ -805,6 +805,7 @@ unref() const { pgraph_cat.debug() << "Breaking cycle involving " << (*this) << "\n"; } + ((TransformState *)this)->remove_cache_pointers(); } } @@ -928,6 +929,7 @@ get_num_states() { if (_states == (States *)NULL) { return 0; } + ReMutexHolder holder(*_states_lock); return _states->size(); } @@ -954,6 +956,7 @@ get_num_unused_states() { if (_states == (States *)NULL) { return 0; } + ReMutexHolder holder(*_states_lock); // First, we need to count the number of times each TransformState // object is recorded in the cache. We could just trust @@ -1047,6 +1050,7 @@ clear_cache() { if (_states == (States *)NULL) { return 0; } + ReMutexHolder holder(*_states_lock); PStatTimer timer(_cache_update_pcollector); int orig_size = _states->size(); @@ -1123,6 +1127,11 @@ clear_cache() { //////////////////////////////////////////////////////////////////// void TransformState:: list_cycles(ostream &out) { + if (_states == (States *)NULL) { + return; + } + ReMutexHolder holder(*_states_lock); + typedef pset VisitedStates; VisitedStates visited; CompositionCycleDesc cycle_desc; @@ -1173,6 +1182,12 @@ list_cycles(ostream &out) { //////////////////////////////////////////////////////////////////// void TransformState:: list_states(ostream &out) { + if (_states == (States *)NULL) { + out << "0 states:\n"; + return; + } + ReMutexHolder holder(*_states_lock); + out << _states->size() << " states:\n"; States::const_iterator si; for (si = _states->begin(); si != _states->end(); ++si) { @@ -1193,6 +1208,11 @@ list_states(ostream &out) { //////////////////////////////////////////////////////////////////// bool TransformState:: validate_states() { + if (_states == (States *)NULL) { + return true; + } + + ReMutexHolder holder(*_states_lock); if (_states->empty()) { return true; } @@ -1217,6 +1237,29 @@ validate_states() { return true; } +//////////////////////////////////////////////////////////////////// +// Function: TransformState::init_states +// Access: Public, Static +// Description: 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 TransformState object at static init time; +// it also seems to cause problems when the Panda shared +// library is unloaded at application exit time. +//////////////////////////////////////////////////////////////////// +void TransformState:: +init_states() { + _states = new States; + + // TODO: we should have a global Panda mutex to allow us to safely + // create _states_lock without a startup race condition. For the + // meantime, this is OK because we guarantee that this method is + // called at static init time, presumably when there is still only + // one thread in the world. + _states_lock = new ReMutex; + nassertv(Thread::get_current_thread() == Thread::get_main_thread()); +} + //////////////////////////////////////////////////////////////////// // Function: TransformState::return_new // Access: Private, Static @@ -1233,16 +1276,18 @@ CPT(TransformState) TransformState:: return_new(TransformState *state) { nassertr(state != (TransformState *)NULL, state); - // This should be a newly allocated pointer, not one that was used - // for anything else. - nassertr(state->_saved_entry == _states->end(), state); - #ifndef NDEBUG if (paranoid_const) { nassertr(validate_states(), state); } #endif + ReMutexHolder holder(*_states_lock); + + // This should be a newly allocated pointer, not one that was used + // for anything else. + nassertr(state->_saved_entry == _states->end(), state); + // 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; @@ -1544,9 +1589,14 @@ r_detect_cycles(const TransformState *start_state, // particular TransformState. The pointers to this // object may be scattered around in the various // CompositionCaches from other TransformState objects. +// +// You must already be holding _states_lock before you +// call this method. //////////////////////////////////////////////////////////////////// void TransformState:: remove_cache_pointers() { + nassertv(_states_lock->debug_is_locked()); + // Fortunately, since we added CompositionCache records in pairs, we // know exactly the set of TransformState objects that have us in their // cache: it's the same set of TransformState objects that we have in diff --git a/panda/src/pgraph/transformState.h b/panda/src/pgraph/transformState.h index 90a01373cb..eaccd84100 100644 --- a/panda/src/pgraph/transformState.h +++ b/panda/src/pgraph/transformState.h @@ -29,6 +29,7 @@ #include "updateSeq.h" #include "pStatCollector.h" #include "geomEnums.h" +#include "reMutex.h" class GraphicsStateGuardianBase; class FactoryParams; @@ -182,6 +183,9 @@ PUBLISHED: static void list_states(ostream &out); static bool validate_states(); +public: + static void init_states(); + private: class CompositionCycleDescEntry { public: @@ -206,6 +210,10 @@ private: void remove_cache_pointers(); private: + // This mutex protects _states. It also protects any modification + // to the cache, which is encoded in _composition_cache and + // _invert_composition_cache. + static ReMutex *_states_lock; typedef phash_set > States; static States *_states; static CPT(TransformState) _identity_state; diff --git a/panda/src/pstatclient/pStatClient.I b/panda/src/pstatclient/pStatClient.I index 141e99200a..3c34cc9b08 100644 --- a/panda/src/pstatclient/pStatClient.I +++ b/panda/src/pstatclient/pStatClient.I @@ -79,6 +79,7 @@ get_max_rate() const { //////////////////////////////////////////////////////////////////// INLINE int PStatClient:: get_num_collectors() const { + ReMutexHolder holder(_lock); return _collectors.size(); } @@ -89,6 +90,7 @@ get_num_collectors() const { //////////////////////////////////////////////////////////////////// INLINE PStatCollectorDef *PStatClient:: get_collector_def(int index) const { + ReMutexHolder holder(_lock); nassertr(index >= 0 && index < (int)_collectors.size(), NULL); return _collectors[index].get_def(this, index); @@ -102,6 +104,7 @@ get_collector_def(int index) const { //////////////////////////////////////////////////////////////////// INLINE int PStatClient:: get_num_threads() const { + ReMutexHolder holder(_lock); return _threads.size(); } @@ -112,6 +115,7 @@ get_num_threads() const { //////////////////////////////////////////////////////////////////// INLINE string PStatClient:: get_thread_name(int index) const { + ReMutexHolder holder(_lock); nassertr(index >= 0 && index < (int)_threads.size(), string()); return _threads[index]._name; } @@ -183,6 +187,7 @@ resume_after_pause() { //////////////////////////////////////////////////////////////////// INLINE bool PStatClient:: client_connect(string hostname, int port) { + ReMutexHolder holder(_lock); client_disconnect(); return get_impl()->client_connect(hostname, port); } @@ -194,6 +199,7 @@ client_connect(string hostname, int port) { //////////////////////////////////////////////////////////////////// INLINE bool PStatClient:: client_is_connected() const { + ReMutexHolder holder(_lock); return has_impl() && _impl->client_is_connected(); } @@ -232,6 +238,7 @@ has_impl() const { //////////////////////////////////////////////////////////////////// INLINE PStatClientImpl *PStatClient:: get_impl() { + ReMutexHolder holder(_lock); if (_impl == (PStatClientImpl *)NULL) { _impl = new PStatClientImpl(this); } diff --git a/panda/src/pstatclient/pStatClient.cxx b/panda/src/pstatclient/pStatClient.cxx index e6115824f0..5c5478f49f 100644 --- a/panda/src/pstatclient/pStatClient.cxx +++ b/panda/src/pstatclient/pStatClient.cxx @@ -93,6 +93,7 @@ PStatClient:: //////////////////////////////////////////////////////////////////// PStatCollector PStatClient:: get_collector(int index) const { + ReMutexHolder holder(_lock); nassertr(index >= 0 && index < (int)_collectors.size(), PStatCollector()); return PStatCollector((PStatClient *)this, index); } @@ -104,6 +105,7 @@ get_collector(int index) const { //////////////////////////////////////////////////////////////////// string PStatClient:: get_collector_name(int index) const { + ReMutexHolder holder(_lock); nassertr(index >= 0 && index < (int)_collectors.size(), string()); return _collectors[index].get_name(); @@ -119,6 +121,7 @@ get_collector_name(int index) const { //////////////////////////////////////////////////////////////////// string PStatClient:: get_collector_fullname(int index) const { + ReMutexHolder holder(_lock); nassertr(index >= 0 && index < (int)_collectors.size(), string()); int parent_index = _collectors[index].get_parent_index(); @@ -137,6 +140,7 @@ get_collector_fullname(int index) const { //////////////////////////////////////////////////////////////////// PStatThread PStatClient:: get_thread(int index) const { + ReMutexHolder holder(_lock); nassertr(index >= 0 && index < (int)_threads.size(), PStatThread()); return PStatThread((PStatClient *)this, index); } @@ -189,6 +193,7 @@ main_tick() { //////////////////////////////////////////////////////////////////// void PStatClient:: client_main_tick() { + ReMutexHolder holder(_lock); if (has_impl()) { _impl->client_main_tick(); } @@ -202,6 +207,7 @@ client_main_tick() { //////////////////////////////////////////////////////////////////// void PStatClient:: client_disconnect() { + ReMutexHolder holder(_lock); if (has_impl()) { _impl->client_disconnect(); } @@ -254,6 +260,8 @@ get_global_pstats() { //////////////////////////////////////////////////////////////////// PStatCollector PStatClient:: make_collector_with_relname(int parent_index, string relname) { + ReMutexHolder holder(_lock); + if (relname.empty()) { relname = "Unnamed"; } @@ -292,6 +300,8 @@ make_collector_with_relname(int parent_index, string relname) { //////////////////////////////////////////////////////////////////// PStatCollector PStatClient:: make_collector_with_name(int parent_index, const string &name) { + ReMutexHolder holder(_lock); + nassertr(parent_index >= 0 && parent_index < (int)_collectors.size(), PStatCollector()); @@ -343,6 +353,8 @@ make_collector_with_name(int parent_index, const string &name) { //////////////////////////////////////////////////////////////////// PStatThread PStatClient:: make_thread(const string &name) { + ReMutexHolder holder(_lock); + ThingsByName::const_iterator ni = _threads_by_name.find(name); @@ -388,6 +400,8 @@ make_thread(const string &name) { //////////////////////////////////////////////////////////////////// bool PStatClient:: is_active(int collector_index, int thread_index) const { + ReMutexHolder holder(_lock); + nassertr(collector_index >= 0 && collector_index < (int)_collectors.size(), false); nassertr(thread_index >= 0 && thread_index < (int)_threads.size(), false); @@ -407,6 +421,8 @@ is_active(int collector_index, int thread_index) const { //////////////////////////////////////////////////////////////////// bool PStatClient:: is_started(int collector_index, int thread_index) const { + ReMutexHolder holder(_lock); + nassertr(collector_index >= 0 && collector_index < (int)_collectors.size(), false); nassertr(thread_index >= 0 && thread_index < (int)_threads.size(), false); @@ -424,6 +440,8 @@ is_started(int collector_index, int thread_index) const { //////////////////////////////////////////////////////////////////// void PStatClient:: start(int collector_index, int thread_index) { + ReMutexHolder holder(_lock); + #ifdef _DEBUG nassertv(collector_index >= 0 && collector_index < (int)_collectors.size()); nassertv(thread_index >= 0 && thread_index < (int)_threads.size()); @@ -451,6 +469,8 @@ start(int collector_index, int thread_index) { //////////////////////////////////////////////////////////////////// void PStatClient:: start(int collector_index, int thread_index, float as_of) { + ReMutexHolder holder(_lock); + #ifdef _DEBUG nassertv(collector_index >= 0 && collector_index < (int)_collectors.size()); nassertv(thread_index >= 0 && thread_index < (int)_threads.size()); @@ -477,6 +497,8 @@ start(int collector_index, int thread_index, float as_of) { //////////////////////////////////////////////////////////////////// void PStatClient:: stop(int collector_index, int thread_index) { + ReMutexHolder holder(_lock); + #ifdef _DEBUG nassertv(collector_index >= 0 && collector_index < (int)_collectors.size()); nassertv(thread_index >= 0 && thread_index < (int)_threads.size()); @@ -513,6 +535,8 @@ stop(int collector_index, int thread_index) { //////////////////////////////////////////////////////////////////// void PStatClient:: stop(int collector_index, int thread_index, float as_of) { + ReMutexHolder holder(_lock); + #ifdef _DEBUG nassertv(collector_index >= 0 && collector_index < (int)_collectors.size()); nassertv(thread_index >= 0 && thread_index < (int)_threads.size()); @@ -551,6 +575,8 @@ stop(int collector_index, int thread_index, float as_of) { //////////////////////////////////////////////////////////////////// void PStatClient:: clear_level(int collector_index, int thread_index) { + ReMutexHolder holder(_lock); + _collectors[collector_index]._per_thread[thread_index]._has_level = false; _collectors[collector_index]._per_thread[thread_index]._level = 0.0; } @@ -566,6 +592,8 @@ clear_level(int collector_index, int thread_index) { //////////////////////////////////////////////////////////////////// void PStatClient:: set_level(int collector_index, int thread_index, float level) { + ReMutexHolder holder(_lock); + // We don't want to condition this on whether the client is already // connected or the collector is already active, since we might // connect the client later, and we will want to have an accurate @@ -588,6 +616,8 @@ set_level(int collector_index, int thread_index, float level) { //////////////////////////////////////////////////////////////////// void PStatClient:: add_level(int collector_index, int thread_index, float increment) { + ReMutexHolder holder(_lock); + increment *= get_collector_def(collector_index)->_factor; _collectors[collector_index]._per_thread[thread_index]._has_level = true; _collectors[collector_index]._per_thread[thread_index]._level += increment; @@ -603,6 +633,8 @@ add_level(int collector_index, int thread_index, float increment) { //////////////////////////////////////////////////////////////////// float PStatClient:: get_level(int collector_index, int thread_index) const { + ReMutexHolder holder(_lock); + return _collectors[collector_index]._per_thread[thread_index]._level / get_collector_def(collector_index)->_factor; } diff --git a/panda/src/pstatclient/pStatClient.h b/panda/src/pstatclient/pStatClient.h index d4ae167279..ee3436ee4e 100644 --- a/panda/src/pstatclient/pStatClient.h +++ b/panda/src/pstatclient/pStatClient.h @@ -24,6 +24,8 @@ #include "pStatFrameData.h" #include "pStatClientImpl.h" #include "pStatCollectorDef.h" +#include "reMutex.h" +#include "reMutexHolder.h" #include "luse.h" #include "pmap.h" @@ -113,6 +115,10 @@ private: void add_level(int collector_index, int thread_index, float increment); float get_level(int collector_index, int thread_index) const; +private: + // This mutex protects everything in this class. + ReMutex _lock; + // Not a phash_map, so the threads remain sorted by name. typedef pmap ThingsByName; ThingsByName _threads_by_name; diff --git a/panda/src/windisplay/winGraphicsWindow.cxx b/panda/src/windisplay/winGraphicsWindow.cxx index e2df19f5c1..df2d1f1e99 100644 --- a/panda/src/windisplay/winGraphicsWindow.cxx +++ b/panda/src/windisplay/winGraphicsWindow.cxx @@ -997,22 +997,24 @@ window_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) { set_cursor_out_of_window(); break; - case WM_CREATE: { - track_mouse_leaving(hwnd); - ClearToBlack(hwnd,_properties); - - POINT cpos; - GetCursorPos(&cpos); - ScreenToClient(hwnd,&cpos); - RECT clientRect; - GetClientRect(hwnd, &clientRect); - if(PtInRect(&clientRect,cpos)) - set_cursor_in_window(); // should window focus be true as well? - else set_cursor_out_of_window(); - + case WM_CREATE: + { + track_mouse_leaving(hwnd); + ClearToBlack(hwnd, _properties); + + POINT cpos; + GetCursorPos(&cpos); + ScreenToClient(hwnd, &cpos); + RECT clientRect; + GetClientRect(hwnd, &clientRect); + if (PtInRect(&clientRect,cpos)) { + set_cursor_in_window(); // should window focus be true as well? + } else { + set_cursor_out_of_window(); + } + } break; - } - + case WM_CLOSE: // This is a message from the system indicating that the user // has requested to close the window (e.g. alt-f4).