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

[Romain Vimont]

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

> 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