Just an idea: perhaps it's not a big problem, but this is a cpu hungry solution. You could wait for the threads one by one, always for a single thread to finish. Then you can do it with something like pthread_join() or QThread::wait(), I don't know how the threads are implemented.

Seems this patch causes deadlock (sometimes) when I try to open new tab. I'll fix it.

I'm still working on this. Another implementation with QSemaphore created more problems than it solved.

This is still in progress, got a weird crash in KisImageTest.

ASSERT: "isDetached()" in file /usr/include/qt5/QtCore/qvector.h, line 551
#3  0x00007ffff31b7c7e in qt_assert(char const*, char const*, int) () from /usr/lib64/libQt5Core.so.5
#4  0x00007ffff78c90c5 in QVector<KisUpdateJobItem*>::reallocData (this=0x1e2a798, asize=8, aalloc=8, options=...) at /usr/include/qt5/QtCore/qvector.h:551
#5  0x00007ffff78c89b5 in QVector<KisUpdateJobItem*>::detach (this=0x1e2a798) at /usr/include/qt5/QtCore/qvector.h:355
#6  0x00007ffff78c8449 in QVector<KisUpdateJobItem*>::data (this=0x1e2a798) at /usr/include/qt5/QtCore/qvector.h:122
#7  0x00007ffff78c7ba0 in QVector<KisUpdateJobItem*>::operator[] (this=0x1e2a798, i=0) at /usr/include/qt5/QtCore/qvector.h:402
#8  0x00007ffff78c586c in KisUpdaterContext::waitForDone (this=0x1e2a778) at /home/asavonic/dev/krita/src/krita/libs/image/kis_updater_context.cpp:195
#9  0x00007ffff78e08da in KisUpdateScheduler::barrierLock (this=0x1eb1fd0) at /home/asavonic/dev/krita/src/krita/libs/image/kis_update_scheduler.cpp:337
#10 0x00007ffff79006e0 in KisImage::waitForDone (this=0x7e0080) at /home/asavonic/dev/krita/src/krita/libs/image/kis_image.cc:1161

The crash above is fixed, ready for review.

The cpunters for the threads is a good idea. For me this solution is good if the number of the threads and their order cannot change during the wait cycle.

Hi, @asavonic!

Your patch is really nice! I like the idea of making a snapshot of the jobs and checking them on waking up. There is a problem though. The patch changes the semantics of the waitForDone() call. Originally, waitForDone() in the QThreadPool waited for all the threads to finish. And since each thread before finishing started a new job, waitForDone() could not return before all the jobs of all the strokes and/or updates are finished. And this patch tries to solve a bit different problem: "wait until all the currently running jobs are finished, forget about the new ones". Your problem is actually a bit more complex, and yes, it needs snapshots and all the complications with getting snapshots.

Changing this behavior can lead to some unexpected problems in our code, because, e.g. KisToolTransform::slotRestartTransform() and all the unittests reliy on this behavior. Not sure if it is good idea, but it is what we have.

Will it work if you simplify the solution a bit?

1. In checkWaitCondition() just check if all the jobs are not running, without using the snapshots.
2. Use m_lock for waiting on a wait condition. That'll fix the problem of checking the validity of m_jobs.

"Starving" problem for a waiting threads is not a problem for us, because in most of the cases that is exactly what they want :)

Hi, @dkazakov

The patch changes the semantics of the waitForDone() call.

Yeah, I noticed that, quite a lot of code depends on this behaviour.

Will it work if you simplify the solution a bit?
In checkWaitCondition() just check if all the jobs are not running, without using the snapshots.
"Starving" problem for a waiting threads is not a problem for us, because in most of the cases that is exactly what they want :)

This is actually the solution I tried to avoid, because we can possibly wait forever in waitForDone() if we always have some jobs running (even if they are not related).
But, I guess you are right, without explicit dependencies between the jobs it is better to wait them all to finish.

I've pushed the fix (see the comment above).

And using Q_FOREACH without locking will cause a crash in a case when there is more than three thread

Didn't know this, thank you.

As far as I can understand, the original solution closes and reopens the threads. This works, just takes too much time and resources. So a solution to wait for a monent when there is no thread running, is good. It's only a hazard for the future, currently nothing creates new jobs infinitely during the wait.

Hi, @asavonic!

Speaking truly, I like your previous implementation more :)

I tried to calculate the locks order and it seems like your algorithm is actually safe. The only thing that worries me is that now the semaphore duplicates functionality of m_jobs and that KisSimpleUpdateQueue::optimize() call is neither covered by the semaphore nor m_jobs structure. It should not cause any problems right now, but it might be a problem in the future, because a semaphore is basically a lock, so it should be considered carefully.

The main blocker right now is that optimize() is called after the actually processed the queue. That is not good because we may end up doing to many unnecessary updates.

So I can see two solutions:

1. Fix your current patch to ensure optimize() is called before processQueue().
2. Get back to your QWaitCondition solution, just simplify it with having no snapshots. QWaitCondition doesn't add any lock (it can be based on m_lock), so it doesn't add complexity to the system. At least I guess so :)

And if you have any questions, don't hesitate to ping me on IRC. I might be wrong as well :)

Hi, @asavonic!

I'm sorry, I didn't notice you updated the patch! :)

The patch looks cool now, there are only three minor issues:

1. waitForDone() is usually considered to work like lock()/unlock() so people usually doesn't expect it to need any locking. Please move the locking inside waitForDone() itself :)
2. Please rename QT_NO_DEBUG into something local, like SANITY_CHECK_CONTEXT_LOCKING. We usually don't control the Qt's debugging definitions, and this sanity check should obviously be disabled in most of the cases :)
3. Btw, it would be a good idea to change Q_ASSERT into KIS_ASSERT, because the former ones are always disabled on Ubuntu.

Please fix these small issues and I will push your patch! :)

In D1166#45611, @dkazakov wrote:

1. waitForDone() is usually considered to work like lock()/unlock() so people usually doesn't expect it to need any locking. Please move the locking inside waitForDone() itself :)

Okay, but since the context lock is not recursive it implies the opposite requirement: we must not call waitForDone with lock held.
But now we have an assert for this in lock() :)

2. Please rename QT_NO_DEBUG into something local, like SANITY_CHECK_CONTEXT_LOCKING. We usually don't control the Qt's debugging definitions, and this sanity check should obviously be disabled in most of the cases :)

Maybe this is debatable, but I think these checks make sense only if they enabled by default (in debug builds only, of course).
Their only purpose is to warn about an improper use of KisUpdaterContext functions (calling them without locks).
Buy if you need a special macro to do this check, and you know about its existence, then you are probably already aware about the requirements.

3. Btw, it would be a good idea to change Q_ASSERT into KIS_ASSERT, because the former ones are always disabled on Ubuntu.

Good to know. Actually, Q_ASSERT works as expected on my Ubuntu machine (Qt 5.5.1), so it maybe depends on a Qt version.

Okay, but since the context lock is not recursive it implies the opposite requirement: we must not call waitForDone with lock held.
But now we have an assert for this in lock() :)

Awesome!

2. Please rename QT_NO_DEBUG into something local, like SANITY_CHECK_CONTEXT_LOCKING. We usually don't control the Qt's debugging definitions, and this sanity check should obviously be disabled in most of the cases :)

Maybe this is debatable, but I think these checks make sense only if they enabled by default (in debug builds only, of course).
Their only purpose is to warn about an improper use of KisUpdaterContext functions (calling them without locks).
Buy if you need a special macro to do this check, and you know about its existence, then you are probably already aware about the requirements.

Well, the point is that most of the developers use RelWithDebInfo builds. Or its derivative, like KritaDevs. So in 99% of the cases the check will be disabled and you will not be able to activate it without rebuilding entire Krita. We use DebugFull builds very rarely because they tend to be extremely slow.

3. Btw, it would be a good idea to change Q_ASSERT into KIS_ASSERT, because the former ones are always disabled on Ubuntu.

Good to know. Actually, Q_ASSERT works as expected on my Ubuntu machine (Qt 5.5.1), so it maybe depends on a Qt version.

I think it is related to the RelWithDebInfo thing I wrote above

In D1166#45964, @dkazakov wrote:

How do you like it? If you are ok with the solution

Yes, I think it is the best option.

or just write me and I will modify the patch right before pushing it.

That would be great, thank you!