[vlc-devel] [PATCH 4/6] fetcher: use vlc_executor_t

[Alexandre Janniaux]

Hi,

On Tue, Sep 08, 2020 at 02:37:27PM +0200, Romain Vimont wrote:
> On Tue, Sep 08, 2020 at 02:23:40PM +0200, Romain Vimont wrote:
> > On Tue, Sep 08, 2020 at 02:12:34PM +0200, Alexandre Janniaux wrote:
> > > Hi,
> > >
> > > On Tue, Sep 08, 2020 at 01:18:09PM +0200, Thomas Guillem wrote:
> > > >
> > > >
> > > > On Tue, Sep 8, 2020, at 12:36, Alexandre Janniaux wrote:
> > > > > Hi,
> > > > >
> > > > > I like the end result very much, though the race condition
> > > > > on Cancel is a bit inconvenient. I don't have any better way
> > > > > for now anyway.
> > > > >
> > > > > Just a few comments below:
> > > > >
> > > > > On Mon, Sep 07, 2020 at 05:40:09PM +0200, Romain Vimont wrote:
> > > > > > Replace the background workers by executors.
> > > > > > ---
> > > > > >  src/preparser/fetcher.c | 379 ++++++++++++++++++++++------------------
> > > > > >  1 file changed, 207 insertions(+), 172 deletions(-)
> > > > > >
> > > > > > diff --git a/src/preparser/fetcher.c b/src/preparser/fetcher.c
> > > > > > index 5bb4aa1cef..d237ded782 100644
> > > > > > --- a/src/preparser/fetcher.c
> > > > > > +++ b/src/preparser/fetcher.c
> > > > > > @@ -31,43 +31,118 @@
> > > > > >  #include <vlc_threads.h>
> > > > > >  #include <vlc_memstream.h>
> > > > > >  #include <vlc_meta_fetcher.h>
> > > > > > +#include <vlc_executor.h>
> > > > > >
> > > > > >  #include "art.h"
> > > > > >  #include "libvlc.h"
> > > > > >  #include "fetcher.h"
> > > > > >  #include "input/input_interface.h"
> > > > > > -#include "misc/background_worker.h"
> > > > > >  #include "misc/interrupt.h"
> > > > > >
> > > > > >  struct input_fetcher_t {
> > > > > > -    struct background_worker* local;
> > > > > > -    struct background_worker* network;
> > > > > > -    struct background_worker* downloader;
> > > > > > +    vlc_executor_t *executor_local;
> > > > > > +    vlc_executor_t *executor_network;
> > > > > > +    vlc_executor_t *executor_downloader;
> > > > >
> > > > > I'm ok with it, but we probably want to have a single executor
> > > > > for all of them in the very end, don't we?
> > > >
> > > > No, you don't want to block local parsing because all threads from the network executor are waiting for I/O.
> > >
> > > Indeed that's a bit annoying.
> > >
> > > For the future:
> > >
> > > Since the parsing/fetching is not done by the task itself, it
> > > could probably just not wait in the executor?
> >
> > (preparsing and thumbnailing is not done by the task itself, but
> > fetching is)
> > >
> > > The way I see it is that there are two competing asynchronous
> > > runtime in play: input_thread and vlc_executor.
> >
> > Yes, that's (probably) the reason why background worker exposed
> > pf_start() and pf_stop() and just waited for timeout/completion in the
> > worker thread (but it did not work well for fetching, because it
> > required the caller to spawn its own thread to do the job).
> >
> > > If we remove the wait in the vlc_executor and get notification
> > > from the input_thread, we almost don't need the executor.
> >
> > Yes (but it would require the caller to handle the max concurrent
> > preparsing tasks, etc).
> >
> > > We
> > > probably want to keep the executor since it's more generic
> > > and could be much easier to use in the end though.
> > >
> > > If we remove the asynchronicity in the input thread and replace
> > > the input thread by task submission, that's the case where we
> > > have the issue you mentionned.
> >
> > +1
>
> Btw, in theory, if we split the task creation from the task execution,
> the interruption/cancelation must be provided by the creator of the
> task (the one which submits the task for execution may not know how to
> interrupt it).
>
> In that case, an interrupt() callback in the runnable itself would make
> sense. ;)
>
> But it's very theoretical at this point.
>

I was actually not thinking about splitting both, but instead have
the runnable be created and managed by the bigger object with a more
«state machine» approach, meaning that the real «task» is the one
embodied by the bigger object (preparser, fetcher) which needs multiple
operation that will be done cooperatively.

Like you said, the only point is that it reports the backpressure
handling from the executor (ie. how many tasks are we able to process
simultaneously with N jobs) to the preparser (ie. how many files and
access are we allowed to open simultaneously), which seems better than
spawning thread for this imho.

It's also an incentive to have the executor itself outside of the
different task creators like preparser, fetcher, etc...

But it's probably out of the scope of your patchset so we can design
this on a new thread or trac ticket, probably for another major VLC
version if we really want it in the end.

> > > But in the end, if we succeed to have the async framework
> > > everywhere, we could just have a poll/epoll/IOCP (a readiness
> > > framework is easy to write with a completion one anyway) task
> > > somewhere and notify the access when he can read, be it a network
> > > or local access. It might even work across sandbox boundaries.
> > >
> > > This can probably be deferred to a deadline far in the future
> > > though so my remark is useless for this patchset.
> > >
> > > Regards,
> > > --
> > > Alexandre Janniaux
> > > Videolabs
> > > _______________________________________________
> > > vlc-devel mailing list
> > > To unsubscribe or modify your subscription options:
> > > https://mailman.videolan.org/listinfo/vlc-devel
> > _______________________________________________
> > vlc-devel mailing list
> > To unsubscribe or modify your subscription options:
> > https://mailman.videolan.org/listinfo/vlc-devel
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel