[vlc-devel] [RFC 0/2] New executor API

[Romain Vimont]

On Fri, Aug 28, 2020 at 12:05:42PM +0200, Alexandre Janniaux wrote:
> Hi,
> 
> On Fri, Aug 28, 2020 at 11:16:21AM +0200, Romain Vimont wrote:
> > On Fri, Aug 28, 2020 at 10:25:16AM +0200, Alexandre Janniaux wrote:
> > > Hi,
> > >
> > > On Thu, Aug 27, 2020 at 10:28:58PM +0300, Rémi Denis-Courmont wrote:
> > > > Le torstaina 27. elokuuta 2020, 22.13.37 EEST Romain Vimont a écrit :
> > > > > > So the runnable will be using poll(), vlc_cond_t, vlc_sem_t or some other
> > > > > > wait method that supports timing-out. It might as well use that time-out
> > > > > > capability instead of relying on a thread for the sole purpose of maybe
> > > > > > (ab)using the interrupt callback on time-out.
> > > > >
> > > > > Yes, if you only want timeout, but not "unexpected" cancelation, that
> > > > > works (and is better).
> > > > >
> > > > > > Besides, timing out is an error that deserves logging. Interrupt is not.
> > > > > > So using the same callback for both purpose looks dubious to me.
> > > > >
> > > > > The goal is simplicity.
> > > >
> > > > By making things more complicated. Yeah right...
> > >
> > > I agree with Remi, if you want simplicity, the best thing
> > > would be to limit the asynchronous framework to the
> > > asynchronous execution and signalling like almost every other
> > > asynchronous framework.
> >
> > For example, the Java concurrent framework provides a method cancel() on
> > futures:
> > https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/concurrent/Future.html#cancel(boolean)
> >
> > Of course, this is a higher level language, and thread interruption is
> > something managed at the Thread level, so there is no custom interrupt
> > callback to provide.
> >
> > > Especially since youǘe talked about some `future` object, and
> > > thus a pro-actor model which doesn't have ways to do
> > > signalling from the outside of the asynchronous framework to
> > > the task by itself in almost all implementation, and instead
> > > have each layer of promise being able to return either a result
> > > or an error that the future will catch, and an internal
> > > composition rule for each layer's result.
> > >
> > > In the case the vlc_executor is killed, the preparser client
> > > needs to be killed first since it is using the executor, and
> > > thus can cancel its tasks internally directly, which might be
> > > at the middle of the preprocessing, at the beginning or right
> > > before the end. It's much less convoluted than having the
> > > preparser client use the vlc_executor to cancel the taks, which
> > > will just really call the preparser specific cancellation code
> > > anyway.
> >
> > (The executor would still need a Cancel() function to remove the tasks
> > not started yet.)
> >
> > This would greatly simplify the executor itself.
> >
> > But then, to get the same features, each user (like the preparser) will
> > have to:
> >  - still implement the interrupt mechanism for cancelation (it will just
> >    not pass the callback to the executor)
> 
> We can remove this point since it's need for both.
> 
> >  - implement the timeout manually in addition (which came "for free" if
> >    interruption was implemented)
> 
> It can use the already implemented vlc_timer for this case
> and in the best case we're starting one thread less for timer
> queries within VLC.

(The executor starts the "interrupt thread" only when a task with a
timeout is provided, otherwise it is not started.)

> >  - manage the list of its tasks submitted to the executor
> 
> IMHO, it's a lie to put it only here, you still need to
> manage the list of tasks if you want to get a result somehow,
> and manage the lifecycle of the resources shared between the
> task and the task creator.

You get the instance back in the on_finished() callback. You don't need
to store the list on your side.

I actually implemented it, cf the diff:
https://mailman.videolan.org/pipermail/vlc-devel/2020-August/136784.html
https://code.videolan.org/rom1v/vlc/-/commit/790fb59841a1cc40d0028702a6452ab5aea489b8
> 
> >  - interrupt all remaining tasks before deleting the executor
> 
> You need this in both case anyway,

In one case, the executor does it for you, on the other case you have to
write it manually.

> and like I wrote, before
> the vlc_executor is destroyed anyway since the task creator
> has access to it.
> 
> > Instead of just providing:
> >  - the "interrupt" callback (that must be written for cancelation
> >    anyway);
> >  - the timeout parameter to vlc_executor_Submit().
> >
> > In fact, there are 3 levels of features for the executor:
> >  1. submit tasks (and cancel tasks not started yet)
> >  2. cancel tasks with interruption (to be called on explicit cancel or
> >     executor deletion)
> >  3. timeout (calling the interrupt() callback)
> >
> > Possibilities are basically either 1, 1+2 or 1+2+3.
> >
> > IIUC, what you (Rémi & Alex) suggest is 1.
> >
> > I don't really like the principle of an additional thread to trigger
> > "interrupt" calls either, but I see that as a trade-off between
> > simplicity for the executor impl VS simplicity for the users of the
> > executor.
> >
> > I initially implemented 1+2+3. The current version is:
> > https://code.videolan.org/rom1v/vlc/commits/executor.16
> >
> > I just added two commits to only get 1+2 (I removed the timeout):
> > https://code.videolan.org/rom1v/vlc/commits/executor.17
> >
> > As you can see, it greatly simplifies the executor:
> > https://code.videolan.org/rom1v/vlc/-/commit/e25283cf4dbb03a0c145a4f4aea6da1ae9382126
> >
> > But the users have to handle timeout manually (in addition to the
> > interrupt mechanism):
> > https://code.videolan.org/rom1v/vlc/-/commit/d838204802842e44fff23d7a6b46b69c338c617e
> >
> > Regards
> >
> > >
> > > Regards,
> > > --
> > > Alexandre Janniaux
> > > Videolabs

Regards