diff options
author | Uwe Klotz <uklotz@mixxx.org> | 2019-09-30 08:34:16 +0200 |
---|---|---|
committer | Uwe Klotz <uklotz@mixxx.org> | 2019-09-30 08:34:16 +0200 |
commit | 198f2fcf404d54c2aaf40e1fab484b8f519c8f87 (patch) | |
tree | a46025820ce3519fa709e2f50b4a13caad9a4270 /src/track | |
parent | 95388238416f4c4042c8d2f60d39a04e9b5c8278 (diff) | |
parent | 9688e0442c8217f5105c64c8f4b7e375b5d54ee5 (diff) |
Merge branch '2.2' of git@github.com:mixxxdj/mixxx.git
# Conflicts:
# src/track/globaltrackcache.cpp
Diffstat (limited to 'src/track')
-rw-r--r-- | src/track/globaltrackcache.cpp | 169 | ||||
-rw-r--r-- | src/track/globaltrackcache.h | 54 |
2 files changed, 149 insertions, 74 deletions
diff --git a/src/track/globaltrackcache.cpp b/src/track/globaltrackcache.cpp index d608ed269e..1bb0cab342 100644 --- a/src/track/globaltrackcache.cpp +++ b/src/track/globaltrackcache.cpp @@ -36,18 +36,22 @@ TrackRef createTrackRef(const Track& track) { class EvictAndSaveFunctor { public: - explicit EvictAndSaveFunctor(GlobalTrackCacheEntryPointer cacheEntryPtr) + explicit EvictAndSaveFunctor( + GlobalTrackCacheEntryPointer cacheEntryPtr) : m_cacheEntryPtr(std::move(cacheEntryPtr)) { } void operator()(Track* plainPtr) { Q_UNUSED(plainPtr); // only used in DEBUG_ASSERT + DEBUG_ASSERT(m_cacheEntryPtr); DEBUG_ASSERT(plainPtr == m_cacheEntryPtr->getPlainPtr()); // Here we move m_cacheEntryPtr and the owned track out of the // functor and the owning reference counting object. // This is required to break a cycle reference from the weak pointer // inside the cache entry to the same reference counting object. GlobalTrackCache::evictAndSaveCachedTrack(std::move(m_cacheEntryPtr)); + // Verify that this functor is only invoked once + DEBUG_ASSERT(!m_cacheEntryPtr); } const GlobalTrackCacheEntryPointer& getCacheEntryPointer() const { @@ -58,26 +62,6 @@ class EvictAndSaveFunctor { GlobalTrackCacheEntryPointer m_cacheEntryPtr; }; - -void deleteTrack(Track* plainPtr) { - DEBUG_ASSERT(plainPtr); - - // We safely delete the object via the Qt event queue instead - // of using operator delete! Otherwise the deleted track object - // might be accessed when processing cross-thread signals that - // are delayed within a queued connection and may arrive after - // the object has already been deleted. - if (traceLogEnabled()) { - plainPtr->dumpObjectInfo(); - } - if (debugLogEnabled()) { - kLogger.debug() - << "Deleting" - << plainPtr; - } - plainPtr->deleteLater(); -} - } // anonymous namespace GlobalTrackCacheLocker::GlobalTrackCacheLocker() @@ -210,14 +194,21 @@ void GlobalTrackCacheResolver::initTrackIdAndUnlockCache(TrackId trackId) { } //static -void GlobalTrackCache::createInstance(GlobalTrackCacheSaver* pDeleter) { +void GlobalTrackCache::createInstance( + GlobalTrackCacheSaver* pSaver, + deleteTrackFn_t deleteTrackFn) { DEBUG_ASSERT(!s_pInstance); - s_pInstance = new GlobalTrackCache(pDeleter); + s_pInstance = new GlobalTrackCache(pSaver, deleteTrackFn); } //static void GlobalTrackCache::destroyInstance() { DEBUG_ASSERT(s_pInstance); + // Processing all pending events is required to evict all + // remaining references from the cache. + QCoreApplication::processEvents(); + // Now the cache should be empty + DEBUG_ASSERT(GlobalTrackCacheLocker().isEmpty()); GlobalTrackCache* pInstance = s_pInstance; // Reset the static/global pointer before entering the destructor s_pInstance = nullptr; @@ -226,6 +217,32 @@ void GlobalTrackCache::destroyInstance() { pInstance->deleteLater(); } +void GlobalTrackCacheEntry::TrackDeleter::operator()(Track* pTrack) const { + DEBUG_ASSERT(pTrack); + + // We safely delete the object via the Qt event queue instead + // of using operator delete! Otherwise the deleted track object + // might be accessed when processing cross-thread signals that + // are delayed within a queued connection and may arrive after + // the object has already been deleted. + if (traceLogEnabled()) { + pTrack->dumpObjectInfo(); + } + if (debugLogEnabled()) { + kLogger.debug() + << "Deleting" + << pTrack; + } + + if (m_deleteTrackFn) { + // Custom delete function + (*m_deleteTrackFn)(pTrack); + } else { + // Default delete function + pTrack->deleteLater(); + } +} + //static void GlobalTrackCache::evictAndSaveCachedTrack(GlobalTrackCacheEntryPointer cacheEntryPtr) { // Any access to plainPtr before a validity check inside the @@ -254,9 +271,12 @@ void GlobalTrackCache::evictAndSaveCachedTrack(GlobalTrackCacheEntryPointer cach } } -GlobalTrackCache::GlobalTrackCache(GlobalTrackCacheSaver* pSaver) +GlobalTrackCache::GlobalTrackCache( + GlobalTrackCacheSaver* pSaver, + deleteTrackFn_t deleteTrackFn) : m_mutex(QMutex::Recursive), m_pSaver(pSaver), + m_deleteTrackFn(deleteTrackFn), m_tracksById(kUnorderedCollectionMinCapacity, DbId::hash_fun) { DEBUG_ASSERT(m_pSaver); qRegisterMetaType<GlobalTrackCacheEntryPointer>("GlobalTrackCacheEntryPointer"); @@ -321,6 +341,18 @@ void GlobalTrackCache::relocateTracks( m_tracksByCanonicalLocation = std::move(relocatedTracksByCanonicalLocation); } +void GlobalTrackCache::saveEvictedTrack(Track* pEvictedTrack) const { + DEBUG_ASSERT(pEvictedTrack); + // Disconnect all receivers and block signals before saving the + // track. + // NOTE(uklotzde, 2018-02-03): Simply disconnecting all receivers + // doesn't seem to work reliably. Emitting the clean() signal from + // a track that is about to deleted may cause access violations!! + pEvictedTrack->disconnect(); + pEvictedTrack->blockSignals(true); + m_pSaver->saveEvictedTrack(pEvictedTrack); +} + void GlobalTrackCache::deactivate() { // Ideally the cache should be empty when destroyed. // But since this is difficult to achieve all remaining @@ -331,7 +363,7 @@ void GlobalTrackCache::deactivate() { auto i = m_tracksById.begin(); while (i != m_tracksById.end()) { Track* plainPtr= i->second->getPlainPtr(); - m_pSaver->saveCachedTrack(plainPtr); + saveEvictedTrack(plainPtr); m_tracksByCanonicalLocation.erase(plainPtr->getCanonicalLocation()); i = m_tracksById.erase(i); } @@ -339,7 +371,7 @@ void GlobalTrackCache::deactivate() { auto j = m_tracksByCanonicalLocation.begin(); while (j != m_tracksByCanonicalLocation.end()) { Track* plainPtr= j->second->getPlainPtr(); - m_pSaver->saveCachedTrack(plainPtr); + saveEvictedTrack(plainPtr); j = m_tracksByCanonicalLocation.erase(j); } @@ -413,13 +445,14 @@ TrackPointer GlobalTrackCache::lookupByRef( TrackPointer GlobalTrackCache::revive( GlobalTrackCacheEntryPointer entryPtr) { - TrackPointer savingPtr = entryPtr->getSavingWeakPtr().lock(); + TrackPointer savingPtr = entryPtr->lock(); if (savingPtr) { if (traceLogEnabled()) { kLogger.trace() << "Found alive track" << entryPtr->getPlainPtr(); } + DEBUG_ASSERT(!savingPtr->signalsBlocked()); return savingPtr; } @@ -433,11 +466,12 @@ TrackPointer GlobalTrackCache::revive( << "Reviving zombie track" << entryPtr->getPlainPtr(); } - DEBUG_ASSERT(entryPtr->getSavingWeakPtr().expired()); + DEBUG_ASSERT(entryPtr->expired()); savingPtr = TrackPointer(entryPtr->getPlainPtr(), EvictAndSaveFunctor(entryPtr)); - entryPtr->setSavingWeakPtr(savingPtr); + entryPtr->init(savingPtr); + DEBUG_ASSERT(!savingPtr->signalsBlocked()); return savingPtr; } @@ -510,25 +544,19 @@ void GlobalTrackCache::resolve( << "Cache miss - allocating track" << trackRef; } - auto deletingPtr = std::unique_ptr<Track, void (&)(Track*)>( + auto deletingPtr = std::unique_ptr<Track, GlobalTrackCacheEntry::TrackDeleter>( new Track( std::move(fileInfo), std::move(pSecurityToken), std::move(trackId)), - deleteTrack); - // Track objects live together with the cache on the main thread - // and will be deleted later within the event loop. But this - // function might be called from any thread, even from worker - // threads without an event loop. We need to move the newly - // created object to the target thread. - deletingPtr->moveToThread(QApplication::instance()->thread()); + GlobalTrackCacheEntry::TrackDeleter(m_deleteTrackFn)); auto cacheEntryPtr = std::make_shared<GlobalTrackCacheEntry>( std::move(deletingPtr)); auto savingPtr = TrackPointer( cacheEntryPtr->getPlainPtr(), EvictAndSaveFunctor(cacheEntryPtr)); - cacheEntryPtr->setSavingWeakPtr(savingPtr); + cacheEntryPtr->init(savingPtr); if (debugLogEnabled()) { kLogger.debug() @@ -553,6 +581,14 @@ void GlobalTrackCache::resolve( trackRef.getCanonicalLocation(), cacheEntryPtr)); } + + // Track objects live together with the cache on the main thread + // and will be deleted later within the event loop. But this + // function might be called from any thread, even from worker + // threads without an event loop. We need to move the newly + // created object to the main thread. + savingPtr->moveToThread(QApplication::instance()->thread()); + pCacheResolver->initLookupResult( GlobalTrackCacheLookupResult::MISS, std::move(savingPtr), @@ -608,7 +644,7 @@ void GlobalTrackCache::evictAndSave( GlobalTrackCacheLocker cacheLocker; - if (!cacheEntryPtr->getSavingWeakPtr().expired()) { + if (!cacheEntryPtr->expired()) { // We have handed out (revived) this track again after our reference count // drops to zero and before acquiring the lock at the beginning of this function if (debugLogEnabled()) { @@ -619,31 +655,36 @@ void GlobalTrackCache::evictAndSave( return; } - if (!evict(cacheEntryPtr->getPlainPtr())) { + if (!tryEvict(cacheEntryPtr->getPlainPtr())) { // A second deleter has already evicted the track from cache after our // reference count drops to zero and before acquiring the lock at the // beginning of this function if (debugLogEnabled()) { kLogger.debug() - << "Skip to save an already evicted track" + << "Skip to save an already evicted track again" << cacheEntryPtr->getPlainPtr(); } return; } - DEBUG_ASSERT(isEvicted(cacheEntryPtr->getPlainPtr())); - m_pSaver->saveCachedTrack(cacheEntryPtr->getPlainPtr()); + DEBUG_ASSERT(!isCached(cacheEntryPtr->getPlainPtr())); + saveEvictedTrack(cacheEntryPtr->getPlainPtr()); - // here the cacheEntryPtr goes out of scope, the cache is deleted - // including the owned track + // here the cacheEntryPtr goes out of scope, the cache entry is + // deleted including the owned track } -bool GlobalTrackCache::evict(Track* plainPtr) { +bool GlobalTrackCache::tryEvict(Track* plainPtr) { DEBUG_ASSERT(plainPtr); // Make the cached track object invisible to avoid reusing // it before starting to save it. This is achieved by // removing it from both cache indices. + // Due to expected race conditions pointers might be evicted + // multiple times. Therefore we need to check the stored + // pointers to avoid evicting a new cached track object instead + // of the given plainPtr!! bool evicted = false; + bool notEvicted = false; const auto trackRef = createTrackRef(*plainPtr); if (debugLogEnabled()) { kLogger.debug() @@ -654,39 +695,43 @@ bool GlobalTrackCache::evict(Track* plainPtr) { if (trackRef.hasId()) { const auto trackById = m_tracksById.find(trackRef.getId()); if (trackById != m_tracksById.end()) { - DEBUG_ASSERT(trackById->second->getPlainPtr() == plainPtr); - m_tracksById.erase(trackById); - evicted = true; + if (trackById->second->getPlainPtr() == plainPtr) { + m_tracksById.erase(trackById); + evicted = true; + } else { + notEvicted = true; + } } } if (trackRef.hasCanonicalLocation()) { const auto trackByCanonicalLocation( m_tracksByCanonicalLocation.find(trackRef.getCanonicalLocation())); if (m_tracksByCanonicalLocation.end() != trackByCanonicalLocation) { - DEBUG_ASSERT(trackByCanonicalLocation->second->getPlainPtr() == plainPtr); - m_tracksByCanonicalLocation.erase( - trackByCanonicalLocation); - evicted = true; + if (trackByCanonicalLocation->second->getPlainPtr() == plainPtr) { + m_tracksByCanonicalLocation.erase( + trackByCanonicalLocation); + evicted = true; + } else { + notEvicted = true; + } } } - DEBUG_ASSERT(isEvicted(plainPtr)); - // Don't erase the pointer from m_cachedTracks here, because - // this function is invoked from 2 different contexts. The - // caller is responsible for doing this. Until then the cache - // is inconsistent and verifyConsistency() is expected to fail. + DEBUG_ASSERT(!isCached(plainPtr)); + Q_UNUSED(notEvicted); // only used in debug assertion + DEBUG_ASSERT(!(evicted && notEvicted)); return evicted; } -bool GlobalTrackCache::isEvicted(Track* plainPtr) const { +bool GlobalTrackCache::isCached(Track* plainPtr) const { for (auto&& entry: m_tracksById) { if (entry.second->getPlainPtr() == plainPtr) { - return false; + return true; } } for (auto&& entry: m_tracksByCanonicalLocation) { if (entry.second->getPlainPtr() == plainPtr) { - return false; + return true; } } - return true; + return false; } diff --git a/src/track/globaltrackcache.h b/src/track/globaltrackcache.h index 87945d1ee9..4cabed9209 100644 --- a/src/track/globaltrackcache.h +++ b/src/track/globaltrackcache.h @@ -32,6 +32,8 @@ protected: virtual ~GlobalTrackCacheRelocator() {} }; +typedef void (*deleteTrackFn_t)(Track*); + class GlobalTrackCacheEntry final { // We need to hold two shared pointers, the deletingPtr is // responsible for the lifetime of the Track object itself. @@ -39,25 +41,44 @@ class GlobalTrackCacheEntry final { // is not longer referenced, the track is saved and evicted // from the cache. public: + class TrackDeleter { + public: + explicit TrackDeleter(deleteTrackFn_t deleteTrackFn = nullptr) + : m_deleteTrackFn(deleteTrackFn) { + } + + void operator()(Track* pTrack) const; + + private: + deleteTrackFn_t m_deleteTrackFn; + }; + explicit GlobalTrackCacheEntry( - std::unique_ptr<Track, void (&)(Track*)> deletingPtr) + std::unique_ptr<Track, TrackDeleter> deletingPtr) : m_deletingPtr(std::move(deletingPtr)) { } - GlobalTrackCacheEntry(const GlobalTrackCacheEntry& other) = delete; + GlobalTrackCacheEntry(GlobalTrackCacheEntry&&) = default; + + void init(TrackWeakPointer savingWeakPtr) { + // Uninitialized or expired + DEBUG_ASSERT(!m_savingWeakPtr.lock()); + m_savingWeakPtr = std::move(savingWeakPtr); + } Track* getPlainPtr() const { return m_deletingPtr.get(); } - const TrackWeakPointer& getSavingWeakPtr() const { - return m_savingWeakPtr; + + TrackPointer lock() const { + return m_savingWeakPtr.lock(); } - void setSavingWeakPtr(TrackWeakPointer savingWeakPtr) { - m_savingWeakPtr = std::move(savingWeakPtr); + bool expired() const { + return m_savingWeakPtr.expired(); } private: - std::unique_ptr<Track, void (&)(Track*)> m_deletingPtr; + std::unique_ptr<Track, TrackDeleter> m_deletingPtr; TrackWeakPointer m_savingWeakPtr; }; @@ -155,7 +176,7 @@ private: class /*interface*/ GlobalTrackCacheSaver { private: friend class GlobalTrackCache; - virtual void saveCachedTrack(Track* pTrack) noexcept = 0; + virtual void saveEvictedTrack(Track* pEvictedTrack) noexcept = 0; protected: virtual ~GlobalTrackCacheSaver() {} @@ -165,7 +186,10 @@ class GlobalTrackCache : public QObject { Q_OBJECT public: - static void createInstance(GlobalTrackCacheSaver* pDeleter); + static void createInstance( + GlobalTrackCacheSaver* pSaver, + // A custom deleter is only needed for tests without an event loop! + deleteTrackFn_t deleteTrackFn = nullptr); // NOTE(uklotzde, 2018-02-20): We decided not to destroy the singular // instance during shutdown, because we are not able to guarantee that // all track references have been released before. Instead the singular @@ -184,7 +208,9 @@ private: friend class GlobalTrackCacheLocker; friend class GlobalTrackCacheResolver; - explicit GlobalTrackCache(GlobalTrackCacheSaver* pDeleter); + GlobalTrackCache( + GlobalTrackCacheSaver* pSaver, + deleteTrackFn_t deleteTrackFn); ~GlobalTrackCache(); void relocateTracks( @@ -210,18 +236,22 @@ private: void purgeTrackId(TrackId trackId); - bool evict(Track* plainPtr); - bool isEvicted(Track* plainPtr) const; + bool tryEvict(Track* plainPtr); + bool isCached(Track* plainPtr) const; bool isEmpty() const; void deactivate(); + void saveEvictedTrack(Track* pEvictedTrack) const; + // Managed by GlobalTrackCacheLocker mutable QMutex m_mutex; GlobalTrackCacheSaver* m_pSaver; + deleteTrackFn_t m_deleteTrackFn; + // This caches the unsaved Tracks by ID typedef std::unordered_map<TrackId, GlobalTrackCacheEntryPointer, TrackId::hash_fun_t> TracksById; TracksById m_tracksById; |