From b21e8fdf3216b9a18d7f2edef9fc5ce5590f9969 Mon Sep 17 00:00:00 2001 From: rdb Date: Sun, 11 Dec 2016 15:22:40 +0100 Subject: [PATCH] COW performance tweaks; also somehow fixes Bullet soft body issue --- .../putil/cachedTypedWritableReferenceCount.I | 19 +++- .../putil/cachedTypedWritableReferenceCount.h | 3 +- panda/src/putil/copyOnWriteObject.cxx | 17 ++++ panda/src/putil/copyOnWriteObject.h | 3 + panda/src/putil/copyOnWritePointer.I | 97 ++++++++++++++++--- panda/src/putil/copyOnWritePointer.cxx | 28 +++--- panda/src/putil/copyOnWritePointer.h | 14 ++- 7 files changed, 145 insertions(+), 36 deletions(-) diff --git a/panda/src/putil/cachedTypedWritableReferenceCount.I b/panda/src/putil/cachedTypedWritableReferenceCount.I index 050c95eb3f..1d60f9a614 100644 --- a/panda/src/putil/cachedTypedWritableReferenceCount.I +++ b/panda/src/putil/cachedTypedWritableReferenceCount.I @@ -126,7 +126,7 @@ cache_ref() const { #endif ref(); - AtomicAdjust::inc(((CachedTypedWritableReferenceCount *)this)->_cache_ref_count); + AtomicAdjust::inc(_cache_ref_count); } /** @@ -147,7 +147,7 @@ cache_unref() const { // you can't use PointerTo's? nassertr(_cache_ref_count > 0, 0); - AtomicAdjust::dec(((CachedTypedWritableReferenceCount *)this)->_cache_ref_count); + AtomicAdjust::dec(_cache_ref_count); return ReferenceCount::unref(); } @@ -164,6 +164,19 @@ test_ref_count_integrity() const { #endif } +/** + * Decrements the cache reference count without affecting the normal reference + * count. Don't use this. + */ +INLINE void CachedTypedWritableReferenceCount:: +cache_ref_only() const { +#ifdef _DEBUG + nassertv(test_ref_count_integrity()); +#endif + + AtomicAdjust::inc(_cache_ref_count); +} + /** * Decrements the cache reference count without affecting the normal reference * count. Intended to be called by derived classes only, presumably to @@ -180,7 +193,7 @@ cache_unref_only() const { // you can't use PointerTo's? nassertv(_cache_ref_count > 0); - AtomicAdjust::dec(((CachedTypedWritableReferenceCount *)this)->_cache_ref_count); + AtomicAdjust::dec(_cache_ref_count); } /** diff --git a/panda/src/putil/cachedTypedWritableReferenceCount.h b/panda/src/putil/cachedTypedWritableReferenceCount.h index 15772048dd..7420253fcb 100644 --- a/panda/src/putil/cachedTypedWritableReferenceCount.h +++ b/panda/src/putil/cachedTypedWritableReferenceCount.h @@ -47,11 +47,12 @@ PUBLISHED: MAKE_PROPERTY(cache_ref_count, get_cache_ref_count); protected: + INLINE void cache_ref_only() const; INLINE void cache_unref_only() const; bool do_test_ref_count_integrity() const; private: - AtomicAdjust::Integer _cache_ref_count; + mutable AtomicAdjust::Integer _cache_ref_count; public: static TypeHandle get_class_type() { diff --git a/panda/src/putil/copyOnWriteObject.cxx b/panda/src/putil/copyOnWriteObject.cxx index 0faa2f23ce..172f354319 100644 --- a/panda/src/putil/copyOnWriteObject.cxx +++ b/panda/src/putil/copyOnWriteObject.cxx @@ -35,4 +35,21 @@ unref() const { } return is_zero; } + +/** + * Explicitly increments the cache reference count only. Don't use this. + * + * In the case of a CopyOnWriteObject, when the reference count decrements + * down to the cache reference count, the object is implicitly unlocked. + */ +void CopyOnWriteObject:: +cache_ref_only() const { + MutexHolder holder(_lock_mutex); + CachedTypedWritableReferenceCount::cache_ref_only(); + if (get_ref_count() == get_cache_ref_count()) { + ((CopyOnWriteObject *)this)->_lock_status = LS_unlocked; + ((CopyOnWriteObject *)this)->_locking_thread = NULL; + ((CopyOnWriteObject *)this)->_lock_cvar.notify(); + } +} #endif // COW_THREADED diff --git a/panda/src/putil/copyOnWriteObject.h b/panda/src/putil/copyOnWriteObject.h index 8cada6bfd7..12ea5baf24 100644 --- a/panda/src/putil/copyOnWriteObject.h +++ b/panda/src/putil/copyOnWriteObject.h @@ -49,6 +49,9 @@ PUBLISHED: virtual bool unref() const; INLINE void cache_ref() const; INLINE bool cache_unref() const; + +public: + void cache_ref_only() const; #endif // COW_THREADED protected: diff --git a/panda/src/putil/copyOnWritePointer.I b/panda/src/putil/copyOnWritePointer.I index 98a0987791..a44731bcbc 100644 --- a/panda/src/putil/copyOnWritePointer.I +++ b/panda/src/putil/copyOnWritePointer.I @@ -74,23 +74,58 @@ INLINE CopyOnWritePointer:: * */ INLINE CopyOnWritePointer:: -CopyOnWritePointer(CopyOnWritePointer &&move) NOEXCEPT : - _cow_object(move._cow_object) +CopyOnWritePointer(CopyOnWritePointer &&from) NOEXCEPT : + _cow_object(from._cow_object) { // Steal the other's reference count. - move._cow_object = (CopyOnWriteObject *)NULL; + from._cow_object = (CopyOnWriteObject *)NULL; +} + +/** + * + */ +INLINE CopyOnWritePointer:: +CopyOnWritePointer(PointerTo &&from) NOEXCEPT : + _cow_object(from.p()) +{ + // Steal the other's reference count, but because it is a regular pointer, + // we do need to include the cache reference count. + if (_cow_object != (CopyOnWriteObject *)NULL) { + _cow_object->cache_ref_only(); + } + from.cheat() = NULL; } /** * */ INLINE void CopyOnWritePointer:: -operator = (CopyOnWritePointer &&move) NOEXCEPT { +operator = (CopyOnWritePointer &&from) NOEXCEPT { // Protect against self-move-assignment. - if (move._cow_object != _cow_object) { + if (from._cow_object != _cow_object) { CopyOnWriteObject *old_object = _cow_object; - _cow_object = move._cow_object; - move._cow_object = NULL; + _cow_object = from._cow_object; + from._cow_object = NULL; + + if (old_object != (CopyOnWriteObject *)NULL) { + cache_unref_delete(old_object); + } + } +} + +/** + * + */ +INLINE void CopyOnWritePointer:: +operator = (PointerTo &&from) NOEXCEPT { + if (from.p() != _cow_object) { + CopyOnWriteObject *old_object = _cow_object; + + // Steal the other's reference count, but because it is a regular pointer, + // we do need to include the cache reference count. + _cow_object = from.p(); + _cow_object->cache_ref_only(); + from.cheat() = NULL; if (old_object != (CopyOnWriteObject *)NULL) { cache_unref_delete(old_object); @@ -262,20 +297,60 @@ operator = (To *object) { */ template INLINE CopyOnWritePointerTo:: -CopyOnWritePointerTo(CopyOnWritePointerTo &&move) NOEXCEPT : - CopyOnWritePointer((CopyOnWritePointer &&)move) +CopyOnWritePointerTo(CopyOnWritePointerTo &&from) NOEXCEPT : + CopyOnWritePointer((CopyOnWritePointer &&)from) { } #endif // CPPPARSER +#ifndef CPPPARSER +/** + * + */ +template +INLINE CopyOnWritePointerTo:: +CopyOnWritePointerTo(PointerTo &&from) NOEXCEPT { + // Steal the other's reference count, but because it is a regular pointer, + // we do need to include the cache reference count. + _cow_object = from.p(); + if (_cow_object != (CopyOnWriteObject *)NULL) { + _cow_object->cache_ref_only(); + } + from.cheat() = NULL; +} +#endif // CPPPARSER + #ifndef CPPPARSER /** * */ template INLINE void CopyOnWritePointerTo:: -operator = (CopyOnWritePointerTo &&move) NOEXCEPT { - CopyOnWritePointer::operator = ((CopyOnWritePointer &&)move); +operator = (CopyOnWritePointerTo &&from) NOEXCEPT { + CopyOnWritePointer::operator = ((CopyOnWritePointer &&)from); +} +#endif // CPPPARSER + +#ifndef CPPPARSER +/** + * + */ +template +INLINE void CopyOnWritePointerTo:: +operator = (PointerTo &&from) NOEXCEPT { + if (from.p() != _cow_object) { + CopyOnWriteObject *old_object = _cow_object; + + // Steal the other's reference count, but because it is a regular pointer, + // we do need to include the cache reference count. + _cow_object = from.p(); + _cow_object->cache_ref_only(); + from.cheat() = NULL; + + if (old_object != (CopyOnWriteObject *)NULL) { + cache_unref_delete(old_object); + } + } } #endif // CPPPARSER #endif // USE_MOVE_SEMANTICS diff --git a/panda/src/putil/copyOnWritePointer.cxx b/panda/src/putil/copyOnWritePointer.cxx index de0a2e8abf..e2396e84a9 100644 --- a/panda/src/putil/copyOnWritePointer.cxx +++ b/panda/src/putil/copyOnWritePointer.cxx @@ -90,19 +90,17 @@ get_write_pointer() { } PT(CopyOnWriteObject) new_object = _cow_object->make_cow_copy(); + _cow_object->CachedTypedWritableReferenceCount::cache_unref(); + _cow_object->_lock_mutex.release(); - // We can't call cache_unref_delete, because we hold the lock. - if (!_cow_object->CachedTypedWritableReferenceCount::cache_unref()) { - _cow_object->_lock_mutex.release(); - delete _cow_object; - } else { - _cow_object->_lock_mutex.release(); - } + MutexHolder holder(new_object->_lock_mutex); _cow_object = new_object; - _cow_object->cache_ref(); + _cow_object->CachedTypedWritableReferenceCount::cache_ref(); _cow_object->_lock_status = CopyOnWriteObject::LS_locked_write; _cow_object->_locking_thread = current_thread; + return new_object; + } else if (_cow_object->get_cache_ref_count() > 1) { // No one else has it specifically read-locked, but there are other // CopyOnWritePointers holding the same object, so we should make our own @@ -115,19 +113,17 @@ get_write_pointer() { } PT(CopyOnWriteObject) new_object = _cow_object->make_cow_copy(); + _cow_object->CachedTypedWritableReferenceCount::cache_unref(); + _cow_object->_lock_mutex.release(); - // We can't call cache_unref_delete, because we hold the lock. - if (!_cow_object->CachedTypedWritableReferenceCount::cache_unref()) { - _cow_object->_lock_mutex.release(); - delete _cow_object; - } else { - _cow_object->_lock_mutex.release(); - } + MutexHolder holder(new_object->_lock_mutex); _cow_object = new_object; - _cow_object->cache_ref(); + _cow_object->CachedTypedWritableReferenceCount::cache_ref(); _cow_object->_lock_status = CopyOnWriteObject::LS_locked_write; _cow_object->_locking_thread = current_thread; + return new_object; + } else { // No other thread has the pointer locked, and we're the only // CopyOnWritePointer with this object. We can safely write to it without diff --git a/panda/src/putil/copyOnWritePointer.h b/panda/src/putil/copyOnWritePointer.h index c40c432670..1a7f0e099d 100644 --- a/panda/src/putil/copyOnWritePointer.h +++ b/panda/src/putil/copyOnWritePointer.h @@ -37,8 +37,10 @@ public: INLINE ~CopyOnWritePointer(); #ifdef USE_MOVE_SEMANTICS - INLINE CopyOnWritePointer(CopyOnWritePointer &&move) NOEXCEPT; - INLINE void operator = (CopyOnWritePointer &&move) NOEXCEPT; + INLINE CopyOnWritePointer(CopyOnWritePointer &&from) NOEXCEPT; + INLINE CopyOnWritePointer(PointerTo &&from) NOEXCEPT; + INLINE void operator = (CopyOnWritePointer &&from) NOEXCEPT; + INLINE void operator = (PointerTo &&from) NOEXCEPT; #endif INLINE bool operator == (const CopyOnWritePointer &other) const; @@ -61,7 +63,7 @@ public: INLINE bool test_ref_count_integrity() const; INLINE bool test_ref_count_nonzero() const; -private: +protected: CopyOnWriteObject *_cow_object; }; @@ -84,8 +86,10 @@ public: INLINE void operator = (To *object); #ifdef USE_MOVE_SEMANTICS - INLINE CopyOnWritePointerTo(CopyOnWritePointerTo &&move) NOEXCEPT; - INLINE void operator = (CopyOnWritePointerTo &&move) NOEXCEPT; + INLINE CopyOnWritePointerTo(CopyOnWritePointerTo &&from) NOEXCEPT; + INLINE CopyOnWritePointerTo(PointerTo &&from) NOEXCEPT; + INLINE void operator = (CopyOnWritePointerTo &&from) NOEXCEPT; + INLINE void operator = (PointerTo &&from) NOEXCEPT; #endif #ifdef COW_THREADED