Commit 8a1f781f authored by Dmitry Kazakov's avatar Dmitry Kazakov

[NOT FOR 4.3.0] Fix a threading issue in KisTiledExtentManager

Previous implemention could create ghost areas in the extent of
the paint device if tile addition and removal happened concurrently.

There is no evidence of this problem is real-world usecase of Krita,
the only way how the problem was seen is in this test:
KisTiledDataManagerTest::stressTestExtentsColumn().
parent c503cdfe
......@@ -55,25 +55,32 @@ bool KisTiledExtentManager::Data::add(qint32 index)
KIS_ASSERT_RECOVER_NOOP(m_buffer[currentIndex].loadAcquire() >= 0);
bool needsUpdateExtent = false;
QReadLocker rl(&m_extentLock);
if (!m_buffer[currentIndex].loadAcquire()) {
rl.unlock();
QWriteLocker wl(&m_extentLock);
while (true) {
QReadLocker rl(&m_extentLock);
if (!m_buffer[currentIndex].load()) {
m_buffer[currentIndex].store(1);
int oldValue = m_buffer[currentIndex].loadAcquire();
if (oldValue == 0) {
rl.unlock();
QWriteLocker wl(&m_extentLock);
if (m_min > index) m_min = index;
if (m_max < index) m_max = index;
if ((oldValue = m_buffer[currentIndex].loadAcquire()) == 0) {
++m_count;
needsUpdateExtent = true;
} else {
m_buffer[currentIndex].ref();
if (m_min > index) m_min = index;
if (m_max < index) m_max = index;
++m_count;
needsUpdateExtent = true;
m_buffer[currentIndex].storeRelease(1);
} else {
m_buffer[currentIndex].storeRelease(oldValue + 1);
}
break;
} else if (m_buffer[currentIndex].testAndSetOrdered(oldValue, oldValue + 1)) {
break;
}
} else {
m_buffer[currentIndex].ref();
}
return needsUpdateExtent;
......@@ -84,6 +91,11 @@ bool KisTiledExtentManager::Data::remove(qint32 index)
QReadLocker lock(&m_migrationLock);
qint32 currentIndex = m_offset + index;
bool needsUpdateExtent = false;
QReadLocker rl(&m_extentLock);
const int oldValue = m_buffer[currentIndex].fetchAndAddAcquire(-1);
/**
* That is not the droid you're looking for. If you see this assert
* in the backtrace, most probably, the bug is not here. The crash
......@@ -91,39 +103,20 @@ bool KisTiledExtentManager::Data::remove(qint32 index)
* concurrently for the overlapping rects. That is, they are trying
* to remove the same tile. Look higher!
*/
KIS_ASSERT_RECOVER_NOOP(m_buffer[currentIndex].loadAcquire() > 0);
bool needsUpdateExtent = false;
QReadLocker rl(&m_extentLock);
KIS_SAFE_ASSERT_RECOVER(oldValue > 0) {
m_buffer[currentIndex].store(0);
return false;
}
if (m_buffer[currentIndex].loadAcquire() == 1) {
if (oldValue == 1) {
rl.unlock();
QWriteLocker wl(&m_extentLock);
if (m_buffer[currentIndex].load() == 1) {
m_buffer[currentIndex].store(0);
if (m_min == index) updateMin();
if (m_max == index) updateMax();
if (m_min == index) updateMin();
if (m_max == index) updateMax();
--m_count;
needsUpdateExtent = true;
} else {
m_buffer[currentIndex].deref();
}
} else {
const bool nonZero = m_buffer[currentIndex].deref();
/**
* BUG: since we are not holding the write lock, it might happen
* that two threads failed the original 'if' and entered this
* branch. It means that the device will report unexistent
* extent... See KisTiledDataManagerTest::stressTestExtentsColumn()
* for reproduction.
*
* The same bug is present in KisTiledExtentManager::Data::add()
*/
KIS_SAFE_ASSERT_RECOVER_NOOP(nonZero);
--m_count;
needsUpdateExtent = true;
}
return needsUpdateExtent;
......@@ -228,6 +221,8 @@ void KisTiledExtentManager::Data::migrate(qint32 index)
void KisTiledExtentManager::Data::updateMin()
{
KIS_SAFE_ASSERT_RECOVER_NOOP(m_min != qint32_MAX);
qint32 start = m_min + m_offset;
for (qint32 i = start; i < m_capacity; ++i) {
......@@ -235,13 +230,17 @@ void KisTiledExtentManager::Data::updateMin()
if (current > 0) {
m_min = i - m_offset;
break;
return;
}
}
m_min = qint32_MAX;
}
void KisTiledExtentManager::Data::updateMax()
{
KIS_SAFE_ASSERT_RECOVER_NOOP(m_min != qint32_MIN);
qint32 start = m_max + m_offset;
for (qint32 i = start; i >= 0; --i) {
......@@ -249,9 +248,11 @@ void KisTiledExtentManager::Data::updateMax()
if (current > 0) {
m_max = i - m_offset;
break;
return;
}
}
m_max = qint32_MIN;
}
KisTiledExtentManager::KisTiledExtentManager()
......
......@@ -955,6 +955,8 @@ void KisTiledDataManagerTest::stressTestExtentsColumn()
for(qint32 i = 0; i < m_numCycles; i++) {
if (!m_isCreated) {
m_column.add(m_index);
KIS_SAFE_ASSERT_RECOVER_NOOP(m_column.max() >= m_index);
KIS_SAFE_ASSERT_RECOVER_NOOP(m_column.min() <= m_index);
} else {
m_column.remove(m_index);
}
......@@ -972,7 +974,7 @@ void KisTiledDataManagerTest::stressTestExtentsColumn()
#ifdef LIMIT_LONG_TESTS
const int numThreads = 8;
const int numWorkers = 16;
const int numWorkers = 32;
const int numCycles = 10000;
#else
const int numThreads = 16;
......@@ -988,6 +990,9 @@ void KisTiledDataManagerTest::stressTestExtentsColumn()
pool.start(new Job(column, index, numCycles));
}
pool.waitForDone();
QVERIFY(column.isEmpty());
QVERIFY(column.max() < column.min()); // really empty :)
}
void KisTiledDataManagerTest::benchmaskQRegion()
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment