[vlc-devel] [RFC v2 1/2] executor: introduce new executor API

[Alexandre Janniaux]

Hi,

On Fri, Sep 04, 2020 at 09:26:10AM +0200, Pierre Ynard via vlc-devel wrote:
> Date: Fri, 4 Sep 2020 09:26:10 +0200
> From: Pierre Ynard <linkfanel at yahoo.fr>
> To: vlc-devel at videolan.org
> Subject: Re: [vlc-devel] [RFC v2 1/2] executor: introduce new executor API
> User-Agent: Mutt/1.5.23 (2014-03-12)
>
> > First, I'm not sure I see the point of supporting this use
> > case. Here the fact there are two tasks is completely useless
> > since they are the only ones running, one after the other and
> > no parallel work can happen.
> >
> > Then, since you'll need to wait for the tasks to complete
> > anyway, you can just execute the task directly in foo, removing
> > the need for the vlc_executor and the need to support this use
> > case at all.
>
> Splitting into two tasks can be done for convenience or for reasons of
> code organization - not supporting it means favoring monolithic tasks
> and code.

Sure, I agree, but I think functions are a better match than
task with regards to code organizatoin. I still agree that
supporting chained task is great, I'm far from being against
it and would like it supported.

What I wrote is that I don't find this example relevant
because it doesn't really express any need for async, like
I elaborate in the next paragraph.

> Beyond that, it's not functionally equivalent either: you may
> want to yield the worker thread to other tasks waiting for a chance to
> run.

Again, I agree, but it's functionnally equivalent in the
current code, where the owner of the executor launch the
task and basically kill the executor without launching
additional task.

My whole point is that in a real-world design, you either
don't write this code in async or the case in which this
happens doesn't exist (or, well, is not well-defined
regarding memory safety and race conditions).

If you have additional tasks that you want the initial task
to cooperate with, you don't kill the executor while it's
running them. My point is as simple as this and it completely
describe why I consider this example ill-formed.

It wouldn't come to your mind to consider that writing an
example including race condition with threads means that the
thread abstraction is a bad design. I expect quite similar
expectation for an async design, which is basically threads
with the thread and synchronization part hidden.

> Then you say it yourself, there is no parallelism; but there could
> be. It's common that parallelizable work is preceded or followed by
> unparallelizable sections. There could be a first unparallelizable task,
> which would then attempt to queue several parallel tasks. There's not
> even any obligation that it would output results to wait for.

There are daunting issues if you want to run multiple issues
with the issue creator being destroyed. In particular, in the
case we destroy the executor and consider that it will cancel
every task, thus remove the tasks that are not running.

Since you chained tasks and killed the owner, you probably
had a dynamically allocated state, which means you probably
expected to release it at the end of the task run.

But if you cancel the task, and kill the owner, there's no
one to destroy the task state, except if either

 - you add a destroy() callback to the task, but then it's
   much more difficult to achieve task chaining with a shared
   state.

 - you pass the ownership to another component, which means
   the allocation and destruction happen at different
   location, which is IMHO much worse to read and review than
   expect the vlc_executor to be destroyed when there are no
   more tasks.


> > I don't think it's reasonable to write the async framework
> > while having sync use cases and situation. Thus, concrete
> > examples might be a better fit for the design than minimal
> > codes which can't really enjoy an async design.
>
> Well, is it an async framework? It runs tasks in a thread pool. The API
> doesn't say much about the sync or async nature of the tasks.

I'm not sure I understand this statement. An async executor
can technically run every task synchronously.

The point of async, as I mentioned in previous thread, is
that the tasks cannot wait for other tasks to be executed.
Instead, it must queue itself as soon as the other tasks
got executed and signaled their state. How it queues is the
paradigm that must be decided when designing the API
afterwards, and it can take multiple forms depending on the
integration of coroutines/generators or not.

It's very simple to notice this with the sequential executor
since, if you wait, you cannot execute the other tasks that
will unblock the waiting ones, but in general it's mostly
obvious that if you wait in async code, you're just wasting
a thread which could execute some work instead.

If the sequential executor is considered too "radical" and
not real-world, you can also take a simple executor with
a pool of two threads. If one task is waiting for one another
which is also waiting on a third, you get deadlocks which
defeats almost every benefit from writing with an async
paradigm.

I hope it explains what I consider "async" well enough.

To get back to your point.

> Well, is it an async framework? It runs tasks in a thread pool. The API
> doesn't say much about the sync or async nature of the tasks.

If you are chaining tasks, mentionning future, it is an
async framework. But let's take your point in consideration.

If you were running one or multiple jobs in a thread pool,
or to simplify if you have one thread running, would you
destroy the thread without joining it first?

If the answer is no, why would it be different with the
VLC executor?

Regards,
--
Alexandre Janniaux
Videolabs