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

[Romain Vimont]

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.

> > 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