[Development] Evolving Qt's multithreading API

[Olivier Goffart]

On Friday 22 February 2013 00:07:28 Sze Howe Koh wrote:
> On 21 February 2013 02:16, Corentin Jabot <corentin.jabot at gmail.com> wrote:
> > Hi. I'm the one Olivier mentioned :p
> > 
> > I didn't have time to pursue further the work I started, but I intend
> > to, someday.
> > 
> > The plan, as suggested by thiago was to have a QThread::run(functor)
> > method acting exactly like QtConcurrent::run, but using a new QThead.
> > A similar QThreadPool::run function would call a functor in a thread
> > allocated in its pool.
> 
> I think the the plan is good; QtConcurrent::run() always felt
> out-of-place with the filter-map-reduce API. We just need to be
> mindful of how we integrate it into Qt -- the solution should cover
> all 3 of functors, QRunnable, and parallel event loops.
> 
> I notice you also have QThread::run(QRunnable*). Let's take into
> consideration the existing methods QThreadPool::start() and
> QThreadPool::tryStart(). It's inconsistent to have:
> 
>     QThread::run(QRunnable*)
>     QThreadPool::start(QRunnable*)
> 
> (Look for "static polymorphism" in
> http://doc.qt.digia.com/qq/qq13-apis.html) I think we should either
> make the new functions match the old names, or rename the old
> functions to match the new names. (rename = create new names,
> deprecate old names)

Yes. We need to find good APIs.

> > Those function return QFuture and do not require event loop so this
> > silly snippet work:
> > 
> > int main() {
> > 
> >     auto future = QThread::run( []() { qDebug() << "Hello world" ; } );
> >     future.waitForFinished();
> > 
> > }
> > 
> > My goal was to make it as simple as I could.
> 
> Sounds good
> > Anyway, I don't think we should returns a QThread*. the underlying
> > implementation does not mater and shouldn't mater.
> > What would you do with a QThread* you can't do with a QFuture ?
>
> Apologies, I didn't realize that you had a version that returns
> QFuture; I only saw the one that returned void. (Line 115,
> https://codereview.qt-project.org/#patch,sidebyside,45297,5,src/corelib/thre
> ad/qthread.h) I think returning QThread* is better than void. I still can't
> decide if QThread* or QFuture is better for these new methods, however.

This has uses case, but i'm not sure it is the main use case.

Some more common use case would be (pseudo-code)

auto watcher = new QFutureWatcher;
QObject::connect(watcher, SIGNAL(finished()),  myObject, SLOT(doStuff()));
watcher->setFuture(QThrerad::run([]() { return computeStuff();  } ));
// who deletes the watcher?


I think this pattern should be made easier
Something like:

QObject::connect(
      QThread::runFunction([]() {  return doSomethingComplicated();  }),
      &QThread::finished,
      this,
      &MyObject::doStuff   //cannot use lambda here because we want a
					        //   QueuedConnection
);




> 
> Technically, returning a QThread does hide the implementation. The
> programmer only sees the QThread, and doesn't know that you've
> implemented a QRunnableThread underneath. The programmer already knows
> about QThread (since s/he called QThread::run()), so getting a
> QThread* won't be a big deal.
> 
> 
> About QThread vs. QFuture... QThread:
> - Lets the programmer postpone starting the thread
> - Lets the programmer query and manipulate the thread using
> event-driven programming (signals and slots)
> 
> On the other hand, QFuture makes it easy to retrieve the function's
> return value.
> 
> I leaned towards returning QThread* because QtConcurrent::run()
> couldn't use most of QFuture's features anyway, and QThread* can be
> returned from all 3 static functions:
> 
>     static QThread* QThread::setupSimpleThread(QRunnable *runnable);
>     static QThread* QThread::setupSimpleThread(Function func, Args arg,
> ...); static QThread* QThread::setupEventLoop(QObject* worker);

So then the caller is responsible on calling start()  and also deleting the 
thread ?
 
> ...but I'm not sure if losing the functor's return value is worth it.
> 
> > The Thread is started right-away, you can't really interrupt the
> > function until its done, etc.... but you could delete the thread while
> > it's running, so yeah, I'd rather not a return a pointer to the
> > thread.
> 
> I don't think we have to worry about people deleting the QThread while
> it's running. If we did, we'd have similar problems the
> QAudioInput::start() (returns a QIODevice*) and
> QNetworkAccessManager::get() (returns a QNetworkReply*).
> 
> > I find the signature "QThread::run(function)", quite straightforward,
> > there is no confusion about what it does.
> > I don't think the fact there is also the "static void run()" method is
> > confusing, but maybe that's just me.
> 
> I agree that, by itself, "run()" is a good name for this feature. The
> issue, however, is that QThread already has something completely
> different, also called run(). Thus, our choices become more limited.
> 
> I disagree that calling them all "run()" is non-confusing. It produces
> an inconsistent API. Imagine that a new user finds a class with these
> functions:
> 
>     public static QFuture<T> run(QFunction&);
>     public static void run(QRunnable*);
>     protected void run();
> 
> Will this new user be reasonably able to guess their differences?
> 
> > That said, there is no real reason to put these functions in QThread,
> > except the name.
> > These functions could be put elsewhere, I actually started to work
> > with a bunch of free functions before integrated them to QThread
> 
> I think we should try to keep it inside QThread if possible; creating
> another class/namespace feels excessive
> 
> > The implementation use the same sort of generator that QtConcurrent.
> > Thiago suggested making this feature only compatible C++11, which
> > would make it easier to maintain. I actually envisaged to send a mail
> > about that particular issue.
> > Can c++03 really be dropped for that particular feature ?
> > Also, I we were to make a c++11 only feature, what would be the
> > benefits over std::async ?
> 
> I would love to see an implementation using C++11. 

What do you mean?  
Do you mean an implementation of QThread that would use std::thread as an 
internal rather than pthread/windows API? (qthread_cxx11.cpp)

I think it would be good to have. But would just be another layer, and more 
code to maintain.

> I think that
> decision can't be made lightly though, and should be a separate
> discussion -- it has implications for all other Qt modules.
> 
> Compared to std::async, a Qt-based solution would also work for
> QRunnable and support signalling.
> 
> > About running QObject* methods on a separate thread, it can become
> > quite complex.
> > Which slot is called when the thread start ?
> 
> The workerObject->moveToThread() approach seems to work quite well. No
> slot should need to be called when the thread starts. The programmer
> can choose to connect any signal (including QThread::started()) to any
> slot, if the QThread* is available.
> 
> > Which signal makes the thread quit ?
> 
> The programmer can choose to connect any signal to QThread::quit(), if
> the QThread* is available.
> 
> > Should the object be deleted afterwards ? Moved back to
> > its original thread ?
> 
> In principle, the thread should only finish when the object has
> finished is job. So, I'd imagine that the end of a thread would be
> followed by the destruction of its objects. Of course, being a
> low-level solution, the programmer can decide. Can someone find a use
> case for an object to change threads multiple times?
> 
> > Considering the number of scenario, I'm not sure we would benefit from
> > a function - the only factorizable part is
> > 
> > QThread* t = new QThread();
> > obj->moveToThread(t);
> > connect(t, SIGNAL(finished()), t, SLOT(deleteLater()));
> 
> I suggested QThread::setupEventLoop() to make it easy to compare with
> QThread::setupSimpleThread(), and to make it clear that the former has
> an event loop while the latter doesn't. The goal here was to unify the
> API. I'm not 100% happy with these names because they're quite long,
> but I noticed that Qt traditionally chose clear names over short names
> 
> > Maybe we should first agree on how QThread sould work before trying to
> > add yet-another-way.
> 
> Agreed.
> 
> Currently, QThread is Qt's fundamental, low-level thread-control
> interface. It has the ability to run a parallel event loop, and
> provides access to the thread's event dispatcher. Its API lets us
> query and manipulate the state of a thread using event-driven
> programming (i.e. signals + slots).
> 
> When I wrote my proposal, my goal was only to extend and unify the API
> for QThread and QThreadPool; I didn't consider changing the classes'
> roles/behaviour. Which of these characteristics should we incorporate
> into the new API? Is there anything we should deprecate?

-- 
Olivier 

Woboq - Qt services and support - http://woboq.com - http://code.woboq.org