From c3d8f815818330a6fdc5890647483cd3598e15f5 Mon Sep 17 00:00:00 2001 From: David Rose Date: Sun, 4 Sep 2011 00:11:57 +0000 Subject: [PATCH] progress on multithreaded Panda --- panda/src/pgraph/config_pgraph.cxx | 8 +- panda/src/pgraph/config_pgraph.h | 1 + panda/src/pgraph/renderAttrib.cxx | 5 + panda/src/pgraph/renderState.cxx | 11 +- panda/src/pgraph/transformState.cxx | 311 ++++++++++++++++++---------- panda/src/pgraph/transformState.h | 2 + panda/src/testbed/pgrid.cxx | 36 ++-- 7 files changed, 238 insertions(+), 136 deletions(-) diff --git a/panda/src/pgraph/config_pgraph.cxx b/panda/src/pgraph/config_pgraph.cxx index a0d88b0fbe..574405fb7c 100644 --- a/panda/src/pgraph/config_pgraph.cxx +++ b/panda/src/pgraph/config_pgraph.cxx @@ -188,7 +188,13 @@ ConfigVariableBool auto_break_cycles PRC_DESC("Set this true to automatically detect and break reference-count " "cycles in the TransformState and RenderState caches. When this " "is false, you must explicitly call TransformState.clear_cache() " - "from time to time to prevent gradual memory bloat.")); + "from time to time to prevent gradual memory bloat. This has " + "no meaning when garbage-collect-states is true.")); + +ConfigVariableBool garbage_collect_states +("garbage-collect-states", false, + PRC_DESC("This temporary config variable is used for development only. " + "Do not set!")); ConfigVariableBool transform_cache ("transform-cache", true, diff --git a/panda/src/pgraph/config_pgraph.h b/panda/src/pgraph/config_pgraph.h index 41061e4c19..6d36f3782c 100644 --- a/panda/src/pgraph/config_pgraph.h +++ b/panda/src/pgraph/config_pgraph.h @@ -44,6 +44,7 @@ extern ConfigVariableBool compose_componentwise; extern ConfigVariableBool uniquify_matrix; extern ConfigVariableBool paranoid_const; extern ConfigVariableBool auto_break_cycles; +extern ConfigVariableBool garbage_collect_states; extern ConfigVariableBool transform_cache; extern ConfigVariableBool state_cache; extern ConfigVariableBool uniquify_transforms; diff --git a/panda/src/pgraph/renderAttrib.cxx b/panda/src/pgraph/renderAttrib.cxx index f8d1d30911..02d9f0a0ab 100644 --- a/panda/src/pgraph/renderAttrib.cxx +++ b/panda/src/pgraph/renderAttrib.cxx @@ -144,6 +144,11 @@ cull_callback(CullTraverser *, const CullTraverserData &) const { //////////////////////////////////////////////////////////////////// bool RenderAttrib:: unref() const { + // This is flawed, but this is development only. + if (garbage_collect_states) { + return ReferenceCount::unref(); + } + // 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. LightReMutexHolder holder(*_attribs_lock); diff --git a/panda/src/pgraph/renderState.cxx b/panda/src/pgraph/renderState.cxx index 14ef516b04..dec515beaf 100644 --- a/panda/src/pgraph/renderState.cxx +++ b/panda/src/pgraph/renderState.cxx @@ -419,11 +419,9 @@ compose(const RenderState *other) const { return this; } -#ifndef NDEBUG if (!state_cache) { return do_compose(other); } -#endif // NDEBUG LightReMutexHolder holder(*_states_lock); @@ -516,11 +514,9 @@ invert_compose(const RenderState *other) const { return make_empty(); } -#ifndef NDEBUG if (!state_cache) { return do_invert_compose(other); } -#endif // NDEBUG LightReMutexHolder holder(*_states_lock); @@ -706,6 +702,11 @@ adjust_all_priorities(int adjustment) const { //////////////////////////////////////////////////////////////////// bool RenderState:: unref() const { + // This is flawed, but this is development only. + if (garbage_collect_states) { + return ReferenceCount::unref(); + } + // 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. LightReMutexHolder holder(*_states_lock); @@ -1460,11 +1461,9 @@ CPT(RenderState) RenderState:: return_unique(RenderState *state) { nassertr(state != (RenderState *)NULL, state); -#ifndef NDEBUG if (!state_cache) { return state; } -#endif #ifndef NDEBUG if (paranoid_const) { diff --git a/panda/src/pgraph/transformState.cxx b/panda/src/pgraph/transformState.cxx index 5d90849609..90b52da6d0 100644 --- a/panda/src/pgraph/transformState.cxx +++ b/panda/src/pgraph/transformState.cxx @@ -579,10 +579,6 @@ set_shear2d(float shear) const { //////////////////////////////////////////////////////////////////// CPT(TransformState) TransformState:: compose(const TransformState *other) const { - // This method isn't strictly const, because it updates the cache, - // but we pretend that it is because it's only a cache which is - // transparent to the rest of the interface. - // We handle identity as a trivial special case. if (is_identity()) { return other; @@ -599,72 +595,37 @@ compose(const TransformState *other) const { return other; } -#ifndef NDEBUG if (!transform_cache) { return do_compose(other); } -#endif // NDEBUG - - LightReMutexHolder holder(*_states_lock); // Is this composition already cached? - int index = _composition_cache.find(other); - if (index != -1) { - Composition &comp = ((TransformState *)this)->_composition_cache.modify_data(index); - if (comp._result == (const TransformState *)NULL) { - // Well, it wasn't cached already, but we already had an entry - // (probably created for the reverse direction), so use the same - // entry to store the new result. - CPT(TransformState) result = do_compose(other); - comp._result = result; - - if (result != (const TransformState *)this) { - // See the comments below about the need to up the reference - // count only when the result is not the same as this. - result->cache_ref(); - } + CPT(TransformState) result; + { + LightReMutexHolder holder(*_states_lock); + int index = _composition_cache.find(other); + if (index != -1) { + const Composition &comp = _composition_cache.get_data(index); + result = comp._result; + } + if (result != (TransformState *)NULL) { + _cache_stats.inc_hits(); } - // Here's the cache! - _cache_stats.inc_hits(); - return comp._result; - } - _cache_stats.inc_misses(); - - // We need to make a new cache entry, both in this object and in the - // other object. We make both records so the other TransformState - // object will know to delete the entry from this object when it - // destructs, and vice-versa. - - // The cache entry in this object is the only one that indicates the - // result; the other will be NULL for now. - CPT(TransformState) result = do_compose(other); - - _cache_stats.add_total_size(1); - _cache_stats.inc_adds(_composition_cache.get_size() == 0); - - ((TransformState *)this)->_composition_cache[other]._result = result; - - if (other != this) { - _cache_stats.add_total_size(1); - _cache_stats.inc_adds(other->_composition_cache.get_size() == 0); - ((TransformState *)other)->_composition_cache[this]._result = NULL; } - if (result != (const TransformState *)this) { - // If the result of compose() is something other than this, - // explicitly increment the reference count. We have to be sure - // to decrement it again later, when the composition entry is - // removed from the cache. - result->cache_ref(); - - // (If the result was just this again, we still store the - // result, but we don't increment the reference count, since - // that would be a self-referential leak.) + if (result != (TransformState *)NULL) { + // Success! + return result; } - _cache_stats.maybe_report("TransformState"); + // Not in the cache. Compute a new result. It's important that we + // don't hold the lock while we do this, or we lose the benefit of + // parallelization. + result = do_compose(other); - return result; + // It's OK to cast away the constness of this pointer, because the + // cache is a transparent property of the class. + return ((TransformState *)this)->store_compose(other, result); } //////////////////////////////////////////////////////////////////// @@ -704,69 +665,38 @@ invert_compose(const TransformState *other) const { return make_identity(); } -#ifndef NDEBUG if (!transform_cache) { return do_invert_compose(other); } -#endif // NDEBUG LightReMutexHolder holder(*_states_lock); - // Is this composition already cached? - int index = _invert_composition_cache.find(other); - if (index != -1) { - Composition &comp = ((TransformState *)this)->_invert_composition_cache.modify_data(index); - if (comp._result == (const TransformState *)NULL) { - // Well, it wasn't cached already, but we already had an entry - // (probably created for the reverse direction), so use the same - // entry to store the new result. - CPT(TransformState) result = do_invert_compose(other); - comp._result = result; - - if (result != (const TransformState *)this) { - // See the comments below about the need to up the reference - // count only when the result is not the same as this. - result->cache_ref(); - } + CPT(TransformState) result; + { + LightReMutexHolder holder(*_states_lock); + int index = _invert_composition_cache.find(other); + if (index != -1) { + const Composition &comp = _invert_composition_cache.get_data(index); + result = comp._result; + } + if (result != (TransformState *)NULL) { + _cache_stats.inc_hits(); } - // Here's the cache! - _cache_stats.inc_hits(); - return comp._result; - } - _cache_stats.inc_misses(); - - // We need to make a new cache entry, both in this object and in the - // other object. We make both records so the other TransformState - // object will know to delete the entry from this object when it - // destructs, and vice-versa. - - // The cache entry in this object is the only one that indicates the - // result; the other will be NULL for now. - CPT(TransformState) result = do_invert_compose(other); - - _cache_stats.add_total_size(1); - _cache_stats.inc_adds(_invert_composition_cache.get_size() == 0); - ((TransformState *)this)->_invert_composition_cache[other]._result = result; - - if (other != this) { - _cache_stats.add_total_size(1); - _cache_stats.inc_adds(other->_invert_composition_cache.get_size() == 0); - ((TransformState *)other)->_invert_composition_cache[this]._result = NULL; } - if (result != (const TransformState *)this) { - // If the result of compose() is something other than this, - // explicitly increment the reference count. We have to be sure - // to decrement it again later, when the composition entry is - // removed from the cache. - result->cache_ref(); - - // (If the result was just this again, we still store the - // result, but we don't increment the reference count, since - // that would be a self-referential leak.) + if (result != (TransformState *)NULL) { + // Success! + return result; } - return result; + // Not in the cache. Compute a new result. It's important that we + // don't hold the lock while we do this, or we lose the benefit of + // parallelization. + result = do_invert_compose(other); + + // It's OK to cast away the constness of this pointer, because the + // cache is a transparent property of the class. + return ((TransformState *)this)->store_invert_compose(other, result); } //////////////////////////////////////////////////////////////////// @@ -781,6 +711,11 @@ invert_compose(const TransformState *other) const { //////////////////////////////////////////////////////////////////// bool TransformState:: unref() const { + // This is flawed, but this is development only. + if (garbage_collect_states) { + return ReferenceCount::unref(); + } + // 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. LightReMutexHolder holder(*_states_lock); @@ -1526,11 +1461,9 @@ CPT(TransformState) TransformState:: return_unique(TransformState *state) { nassertr(state != (TransformState *)NULL, state); -#ifndef NDEBUG if (!transform_cache) { return state; } -#endif #ifndef NDEBUG if (paranoid_const) { @@ -1647,6 +1580,158 @@ do_compose(const TransformState *other) const { } } +//////////////////////////////////////////////////////////////////// +// Function: TransformState::store_compose +// Access: Private +// Description: Stores the result of a composition in the cache. +// Returns the stored result (it may be a different +// object than the one passed in, due to another thread +// having computed the composition first). +//////////////////////////////////////////////////////////////////// +CPT(TransformState) TransformState:: +store_compose(const TransformState *other, const TransformState *result) { + // Identity should have already been screened. + nassertr(!is_identity(), other); + nassertr(!other->is_identity(), this); + + // So should have validity. + nassertr(!is_invalid(), this); + nassertr(!other->is_invalid(), other); + + LightReMutexHolder holder(*_states_lock); + + // Is this composition already cached? + int index = _composition_cache.find(other); + if (index != -1) { + Composition &comp = _composition_cache.modify_data(index); + if (comp._result == (const TransformState *)NULL) { + // Well, it wasn't cached already, but we already had an entry + // (probably created for the reverse direction), so use the same + // entry to store the new result. + comp._result = result; + + if (result != (const TransformState *)this) { + // See the comments below about the need to up the reference + // count only when the result is not the same as this. + result->cache_ref(); + } + } + // Here's the cache! + _cache_stats.inc_hits(); + return comp._result; + } + _cache_stats.inc_misses(); + + // We need to make a new cache entry, both in this object and in the + // other object. We make both records so the other TransformState + // object will know to delete the entry from this object when it + // destructs, and vice-versa. + + // The cache entry in this object is the only one that indicates the + // result; the other will be NULL for now. + _cache_stats.add_total_size(1); + _cache_stats.inc_adds(_composition_cache.get_size() == 0); + + _composition_cache[other]._result = result; + + if (other != this) { + _cache_stats.add_total_size(1); + _cache_stats.inc_adds(other->_composition_cache.get_size() == 0); + ((TransformState *)other)->_composition_cache[this]._result = NULL; + } + + if (result != (TransformState *)this) { + // If the result of do_compose() is something other than this, + // explicitly increment the reference count. We have to be sure + // to decrement it again later, when the composition entry is + // removed from the cache. + result->cache_ref(); + + // (If the result was just this again, we still store the + // result, but we don't increment the reference count, since + // that would be a self-referential leak.) + } + + _cache_stats.maybe_report("TransformState"); + + return result; +} + +//////////////////////////////////////////////////////////////////// +// Function: TransformState::store_invert_compose +// Access: Private +// Description: Stores the result of a composition in the cache. +// Returns the stored result (it may be a different +// object than the one passed in, due to another thread +// having computed the composition first). +//////////////////////////////////////////////////////////////////// +CPT(TransformState) TransformState:: +store_invert_compose(const TransformState *other, const TransformState *result) { + // Identity should have already been screened. + nassertr(!is_identity(), other); + + // So should have validity. + nassertr(!is_invalid(), this); + nassertr(!other->is_invalid(), other); + + nassertr(other != this, make_identity()); + + LightReMutexHolder holder(*_states_lock); + + // Is this composition already cached? + int index = _invert_composition_cache.find(other); + if (index != -1) { + Composition &comp = ((TransformState *)this)->_invert_composition_cache.modify_data(index); + if (comp._result == (const TransformState *)NULL) { + // Well, it wasn't cached already, but we already had an entry + // (probably created for the reverse direction), so use the same + // entry to store the new result. + comp._result = result; + + if (result != (const TransformState *)this) { + // See the comments below about the need to up the reference + // count only when the result is not the same as this. + result->cache_ref(); + } + } + // Here's the cache! + _cache_stats.inc_hits(); + return comp._result; + } + _cache_stats.inc_misses(); + + // We need to make a new cache entry, both in this object and in the + // other object. We make both records so the other TransformState + // object will know to delete the entry from this object when it + // destructs, and vice-versa. + + // The cache entry in this object is the only one that indicates the + // result; the other will be NULL for now. + _cache_stats.add_total_size(1); + _cache_stats.inc_adds(_invert_composition_cache.get_size() == 0); + _invert_composition_cache[other]._result = result; + + if (other != this) { + _cache_stats.add_total_size(1); + _cache_stats.inc_adds(other->_invert_composition_cache.get_size() == 0); + ((TransformState *)other)->_invert_composition_cache[this]._result = NULL; + } + + if (result != (TransformState *)this) { + // If the result of compose() is something other than this, + // explicitly increment the reference count. We have to be sure + // to decrement it again later, when the composition entry is + // removed from the cache. + result->cache_ref(); + + // (If the result was just this again, we still store the + // result, but we don't increment the reference count, since + // that would be a self-referential leak.) + } + + return result; +} + //////////////////////////////////////////////////////////////////// // Function: TransformState::do_invert_compose // Access: Private diff --git a/panda/src/pgraph/transformState.h b/panda/src/pgraph/transformState.h index dba6cc9f88..943c7939bd 100644 --- a/panda/src/pgraph/transformState.h +++ b/panda/src/pgraph/transformState.h @@ -234,7 +234,9 @@ private: static CPT(TransformState) return_unique(TransformState *state); CPT(TransformState) do_compose(const TransformState *other) const; + CPT(TransformState) store_compose(const TransformState *other, const TransformState *result); CPT(TransformState) do_invert_compose(const TransformState *other) const; + CPT(TransformState) store_invert_compose(const TransformState *other, const TransformState *result); static bool r_detect_cycles(const TransformState *start_state, const TransformState *current_state, int length, UpdateSeq this_seq, diff --git a/panda/src/testbed/pgrid.cxx b/panda/src/testbed/pgrid.cxx index 59ef5630cb..e14e89bdb9 100644 --- a/panda/src/testbed/pgrid.cxx +++ b/panda/src/testbed/pgrid.cxx @@ -213,16 +213,20 @@ load_gridded_models(WindowFramework *window, // Load up all the files indicated in the list of gridded filenames // and store them in the given vector. + Loader loader; + LoaderOptions options; + options.set_flags(options.get_flags() | LoaderOptions::LF_no_ram_cache); + // First, load up each model from disk once, and store them all // separate from the scene graph. Also count up the total number of // models we'll be putting in the grid. int grid_count = 0; - NodePath models("models"); GriddedFilenames::iterator fi; for (fi = filenames.begin(); fi != filenames.end(); ++fi) { GriddedFilename &gf = (*fi); - gf._model = window->load_model(models, gf._filename); - if (!gf._model.is_empty()) { + PT(PandaNode) node = loader.load_sync(gf._filename, options); + if (node != (PandaNode *)NULL) { + gf._model = NodePath(node); grid_count += gf._count; } } @@ -254,7 +258,7 @@ load_gridded_models(WindowFramework *window, int passnum = 0; bool loaded_any; - NodePath render = window->get_render(); + NodePath root = window->get_panda_framework()->get_models(); do { loaded_any = false; @@ -265,9 +269,15 @@ load_gridded_models(WindowFramework *window, // Copy this model into the scene graph, and assign it a // position on the grid. - string model_name = format_string(++model_count); - NodePath model = render.attach_new_node(model_name); - gf._model.copy_to(model); + ++model_count; + PT(PandaNode) node = loader.load_sync(gf._filename, options); + NodePath model; + if (node == (PandaNode *)NULL) { + model = gf._model.copy_to(NodePath()); + } else { + model = NodePath(node); + } + model.reparent_to(root); gridded_file_info info; info.node = model.node(); @@ -369,14 +379,6 @@ load_gridded_models(WindowFramework *window, passnum++; } while (loaded_any); - - // Finally, remove the source models we loaded up. Not a real big deal. - for (fi = filenames.begin(); fi != filenames.end(); ++fi) { - GriddedFilename &gf = (*fi); - if (!gf._model.is_empty()) { - gf._model.remove_node(); - } - } } int @@ -402,13 +404,15 @@ main(int argc, char *argv[]) { window->enable_keyboard(); window->setup_trackball(); - window->load_models(window->get_render(), static_filenames); + window->load_models(framework.get_models(), static_filenames); + framework.get_models().instance_to(window->get_render()); GriddedInfoArray info_arr; load_gridded_models(window, gridded_filenames, info_arr); window->loop_animations(); window->stagger_animations(); + window->center_trackball(framework.get_models()); framework.enable_default_keys();