From kde-kimageshop Mon Jun 04 20:50:08 2007 From: Boudewijn Rempt Date: Mon, 04 Jun 2007 20:50:08 +0000 To: kde-kimageshop Subject: koffice/krita/image/tiles Message-Id: <1180990208.728315.7555.nullmailer () svn ! kde ! org> X-MARC-Message: https://marc.info/?l=kde-kimageshop&m=118099022001215 SVN commit 671476 by rempt: CCMAIL:kimageshop@kde.org Given that everything was serialized anyway because of the BKL, remove the other two mutexes -- this should help a lot with performance, but actually, I think I noticed a degradation. Please test! M +20 -66 kis_tilemanager.cc M +15 -6 kis_tilemanager.h --- trunk/koffice/krita/image/tiles/kis_tilemanager.cc #671475:671476 @@ -48,7 +48,9 @@ // ### use QMutexLocker for the all mutexes -KisTileManager::KisTileManager() { +KisTileManager::KisTileManager() + : m_bigKritaLock( QMutex::Recursive ) +{ Q_ASSERT(KisTileManager::m_singleton == 0); KisTileManager::m_singleton = this; @@ -79,13 +81,6 @@ } counter = 0; - - // XXX Why pointers to mutexes, and not as member objects? - m_poolMutex = new QMutex(QMutex::Recursive); - m_swapMutex = new QMutex(QMutex::Recursive); - // Catchall until this is code is audited for threading. - // Actually, why have more than one lock at all here? Much safer with less locks - m_bigKritaLock = new QMutex(QMutex::Recursive); } KisTileManager::~KisTileManager() { @@ -117,10 +112,6 @@ delete [] m_poolPixelSizes; delete [] m_pools; - - delete m_poolMutex; - delete m_swapMutex; - delete m_bigKritaLock; // Where did this go to? delete [] m_poolFreeList; } @@ -135,8 +126,7 @@ void KisTileManager::registerTile(KisTile* tile) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); TileInfo* info = new TileInfo(); info->tile = tile; @@ -162,15 +152,13 @@ if (++counter % 50 == 0) printInfo(); - m_swapMutex->unlock(); } void KisTileManager::deregisterTile(KisTile* tile) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); + if (!m_tileMap.contains(tile)) { - m_swapMutex->unlock(); return; } // Q_ASSERT(m_tileMap.contains(tile)); @@ -217,13 +205,11 @@ doSwapping(); - m_swapMutex->unlock(); } void KisTileManager::ensureTileLoaded(const KisTile* tile) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); TileInfo* info = m_tileMap[tile]; if (info->validNode) { @@ -235,13 +221,11 @@ fromSwap(info); } - m_swapMutex->unlock(); } void KisTileManager::maySwapTile(const KisTile* tile) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); TileInfo* info = m_tileMap[tile]; m_swappableList.push_back(info); @@ -250,16 +234,13 @@ doSwapping(); - m_swapMutex->unlock(); } void KisTileManager::fromSwap(TileInfo* info) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); if (info->inMem) { - m_swapMutex->unlock(); return; } @@ -272,7 +253,6 @@ if (!kritaMmap(info->tile->m_data, 0, info->size, PROT_READ | PROT_WRITE, MAP_SHARED, info->file->handle(), info->filePos)) { kWarning() << "fromSwap failed!" << endl; - m_swapMutex->unlock(); return; } @@ -282,16 +262,13 @@ m_currentInMem++; m_bytesInMem += info->size; - m_swapMutex->unlock(); } void KisTileManager::toSwap(TileInfo* info) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); //Q_ASSERT(info->inMem); if (!info || !info->inMem) { - m_swapMutex->unlock(); return; } @@ -357,7 +334,6 @@ printInfo(); m_swapForbidden = true; - m_swapMutex->unlock(); return; } @@ -372,7 +348,6 @@ if(!file) { kWarning() << "Opening the file as QFile failed" << endl; m_swapForbidden = true; - m_swapMutex->unlock(); return; } @@ -382,19 +357,16 @@ fd, info->filePos)) { kWarning() << "Initial mmap failed" << endl; m_swapForbidden = true; - m_swapMutex->unlock(); return; } memcpy(data, info->tile->m_data, info->size); munmap(data, info->size); - m_poolMutex->lock(); if (isPoolTile(tile->m_data, tile->m_pixelSize)) reclaimTileToPool(tile->m_data, tile->m_pixelSize); else delete[] tile->m_data; - m_poolMutex->unlock(); tile->m_data = 0; } else { @@ -413,16 +385,13 @@ m_currentInMem--; m_bytesInMem -= info->size; - m_swapMutex->unlock(); } void KisTileManager::doSwapping() { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); if (m_swapForbidden || m_currentInMem <= m_maxInMem) { - m_swapMutex->unlock(); return; } @@ -438,12 +407,11 @@ #endif - m_swapMutex->unlock(); } void KisTileManager::printInfo() { - QMutexLocker lock(m_bigKritaLock); + QMutexLocker lock(&m_bigKritaLock); kDebug(DBG_AREA_TILES) << m_bytesInMem << " out of " << m_bytesTotal << " bytes in memory\n"; kDebug(DBG_AREA_TILES) << m_currentInMem << " out of " << m_tileMap.size() << " tiles in memory\n"; kDebug(DBG_AREA_TILES) << m_files.size() << " swap files in use" << endl; @@ -469,40 +437,34 @@ quint8* KisTileManager::requestTileData(qint32 pixelSize) { - QMutexLocker lock(m_bigKritaLock); - m_swapMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); quint8* data = findTileFor(pixelSize); if ( data ) { - m_swapMutex->unlock(); return data; } - m_swapMutex->unlock(); + return new quint8[m_tileSize * pixelSize]; } void KisTileManager::dontNeedTileData(quint8* data, qint32 pixelSize) { - QMutexLocker lock(m_bigKritaLock); - m_poolMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); if (isPoolTile(data, pixelSize)) { reclaimTileToPool(data, pixelSize); } else delete[] data; - m_poolMutex->unlock(); } quint8* KisTileManager::findTileFor(qint32 pixelSize) { - QMutexLocker lock(m_bigKritaLock); - m_poolMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); for (int i = 0; i < 4; i++) { if (m_poolPixelSizes[i] == pixelSize) { if (!m_poolFreeList[i].isEmpty()) { quint8* data = m_poolFreeList[i].front(); m_poolFreeList[i].pop_front(); - m_poolMutex->unlock(); return data; } } @@ -513,56 +475,48 @@ // j = 1 because we return the first element, so no need to add it to the freelist for (int j = 1; j < m_tilesPerPool; j++) m_poolFreeList[i].append(&m_pools[i][j * pixelSize * m_tileSize]); - m_poolMutex->unlock(); return m_pools[i]; } } - m_poolMutex->unlock(); return 0; } bool KisTileManager::isPoolTile(quint8* data, qint32 pixelSize) { - QMutexLocker lock(m_bigKritaLock); + QMutexLocker lock(&m_bigKritaLock); if (data == 0) return false; - m_poolMutex->lock(); for (int i = 0; i < 4; i++) { if (m_poolPixelSizes[i] == pixelSize) { bool b = data >= m_pools[i] && data < m_pools[i] + pixelSize * m_tileSize * m_tilesPerPool; if (b) { - m_poolMutex->unlock(); return true; } } } - m_poolMutex->unlock(); return false; } void KisTileManager::reclaimTileToPool(quint8* data, qint32 pixelSize) { - QMutexLocker lock(m_bigKritaLock); - m_poolMutex->lock(); + QMutexLocker lock(&m_bigKritaLock); + for (int i = 0; i < 4; i++) { if (m_poolPixelSizes[i] == pixelSize) if (data >= m_pools[i] && data < m_pools[i] + pixelSize * m_tileSize * m_tilesPerPool) { m_poolFreeList[i].append(data); } } - m_poolMutex->unlock(); } void KisTileManager::configChanged() { - QMutexLocker lock(m_bigKritaLock); + QMutexLocker lock(&m_bigKritaLock); KConfigGroup cfg = KGlobal::config()->group(""); m_maxInMem = cfg.readEntry("maxtilesinmem", 4000); m_swappiness = cfg.readEntry("swappiness", 100); - m_swapMutex->lock(); doSwapping(); - m_swapMutex->unlock(); } bool KisTileManager::kritaMmap(quint8*& result, void *start, size_t length, --- trunk/koffice/krita/image/tiles/kis_tilemanager.h #671475:671476 @@ -43,7 +43,7 @@ * * tries to preallocate and recycle some tiles to make future allocations faster * (not done yet) * - * XXX: Use QReadWriteLock, QReadLocker and QWriteLocker to make sure everyone can + * XXX: Use QReadWriteLock, QReadLocker and QWriteLocker to make sure everyone can * read any tile, as long as nobody is writing to it. (bsar) * See: http://doc.trolltech.com/qq/qq14-threading.html */ @@ -89,9 +89,19 @@ // filePos is the position inside the file; size is the actual size, fsize is the size // being used in the swap for this tile (may be larger!) // The file points to 0 if it is not swapped, and to the relevant TempFile otherwise - struct TileInfo { KisTile *tile; KTemporaryFile* file; off_t filePos; int size; int fsize; + struct TileInfo { + KisTile *tile; + KTemporaryFile* file; + off_t filePos; + int size; + int fsize; Q3ValueList::iterator node; - bool inMem; bool onFile; bool mmapped; bool validNode; }; + bool inMem; + bool onFile; + bool mmapped; + bool validNode; + }; + typedef struct { KTemporaryFile* file; off_t filePos; int size; } FreeInfo; typedef QHash TileMap; typedef Q3ValueList TileList; @@ -116,10 +126,9 @@ qint32 *m_poolPixelSizes; qint32 m_tilesPerPool; PoolFreeList *m_poolFreeList; - QMutex * m_poolMutex; - QMutex * m_swapMutex; - QMutex * m_bigKritaLock; // The 'BKL' ;) + QMutex m_bigKritaLock; // The 'BKL' ;) + // This is the constant that we will use to see if we want to add a new tempfile // We use 1<<30 (one gigabyte) because apparently 32bit systems don't really like very // large files. _______________________________________________ kimageshop mailing list kimageshop@kde.org https://mail.kde.org/mailman/listinfo/kimageshop