[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility

[Filip Roséen]

Hi Rémi,

On 2017-03-29 20:08, Rémi Denis-Courmont wrote:

> Le perjantaina 24. maaliskuuta 2017, 3.28.28 EEST Filip Roséen a écrit :
> > This added utility will make it easier to handle a queue of tasks that
> > is to be finished in the order received. It allows for users of the
> > utility to focus on the end-goal instead of having to deal with
> > synchronization issues in terms of task-queue handling.
> > 
> > --
> > 
> > Changes since previous submission:
> > 
> >  - added documentation
> >  - moved from src/playlist/ to src/misc
> >  - background-worker are now constructed through a single call
> >  - background-worker internals are now fully private
> >  - rewrote thread-synchronization logic to be more robust (and easier
> >    to prove correct)
> > ---
> >  src/Makefile.am              |   2 +
> >  src/misc/background_worker.c | 235
> > +++++++++++++++++++++++++++++++++++++++++++ src/misc/background_worker.h |
> > 182 +++++++++++++++++++++++++++++++++ 3 files changed, 419 insertions(+)
> >  create mode 100644 src/misc/background_worker.c
> >  create mode 100644 src/misc/background_worker.h
> > 
> > diff --git a/src/Makefile.am b/src/Makefile.am
> > index 1e0e70fa86..208128d076 100644
> > --- a/src/Makefile.am
> > +++ b/src/Makefile.am
> > @@ -310,6 +310,8 @@ libvlccore_la_SOURCES = \
> >  	text/filesystem.c \
> >  	text/iso_lang.c \
> >  	text/iso-639_def.h \
> > +	misc/background_worker.c \
> > +	misc/background_worker.h \
> >  	misc/md5.c \
> >  	misc/probe.c \
> >  	misc/rand.c \
> > diff --git a/src/misc/background_worker.c b/src/misc/background_worker.c
> > new file mode 100644
> > index 0000000000..527509fb98
> > --- /dev/null
> > +++ b/src/misc/background_worker.c
> > @@ -0,0 +1,235 @@
> > +/**************************************************************************
> > *** + * Copyright (C) 2017 VLC authors and VideoLAN
> > + *
> > + * This program is free software; you can redistribute it and/or modify it
> > + * under the terms of the GNU Lesser General Public License as published by
> > + * the Free Software Foundation; either version 2.1 of the License, or + *
> > (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU Lesser General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU Lesser General Public License
> > + * along with this program; if not, write to the Free Software Foundation,
> > + * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA. +
> > ***************************************************************************
> > **/ +
> > +#ifdef HAVE_CONFIG_H
> > +#  include "config.h"
> > +#endif
> > +
> > +#include <vlc_common.h>
> > +#include <vlc_threads.h>
> > +#include <vlc_arrays.h>
> > +
> > +#include "libvlc.h"
> > +#include "background_worker.h"
> > +
> > +struct bg_queued_item {
> > +    void* id; /**< id associated with entity */
> > +    void* entity; /**< the entity to process */
> > +    int timeout; /**< timeout duration in microseconds */
> > +};
> > +
> > +struct background_worker {
> > +    void* owner;
> > +    struct background_worker_config conf;
> > +
> > +    struct {
> > +        bool probe_request; /**< true if a probe is requested */
> 
> Not clear what this has to do with a supposedly generic framework. In fact, 
> not clear what this does at all.

It is used to signal that there is an incoming request to probe the
current running task so that the signal sent from within
`background_worker_RequestProbe` is not lost due to
`&worker->head.lock` being unlocked when the status and/or stopping of
the running task is done.

I was hoping that simply looking how the data-member is used would be
enough to see why it is there, and given that it is a private entity I
assumed that extensive documentation for the specific *data-member* was
not necessary.

> 
> > +        vlc_mutex_t lock; /**< acquire to inspect members that follow */
> > +        vlc_cond_t wait; /**< wait for update in terms of head */
> > +        mtime_t deadline; /**< deadline of the current task */
> 
> Err, likewise. If this is per-item, it wouldn´t be here. And if this is 
> internal state of the thread, it should be on the stack.
> 
> And if the thread is doing nothing, the whole point of the existing system is 
> that the thread should not exist at all.
> 
> > +        void* id; /**< id of the current task */
> 
> Suspicious.

Could you elaborate on why that is suspicious? I was originally using
the head of the queue to denote the currently running task, but
splitting it up lead to easier to understand code (that was at least
the intent).

> 
> > +        bool active; /**< true if there is an active thread */
> > +    } head;
> > +
> > +    struct {
> > +        vlc_mutex_t lock; /**< acquire to inspect members that follow */
> > +        vlc_array_t data; /**< queue of pending entities to process */
> > +    } tail;
> > +};
> > +
> > +static void* Thread( void* data )
> > +{
> > +    struct background_worker* worker = data;
> > +
> > +    for( ;; )
> > +    {
> > +        struct bg_queued_item* item = NULL;
> > +        void* handle;
> > +
> > +        vlc_mutex_lock( &worker->tail.lock );
> > +        {
> > +            if( vlc_array_count( &worker->tail.data ) )
> > +            {
> > +                item = vlc_array_item_at_index( &worker->tail.data, 0 );
> > +                handle = NULL;
> > +
> > +                vlc_array_remove( &worker->tail.data, 0 );
> > +            }
> > +
> > +            vlc_mutex_lock( &worker->head.lock );
> > +            {
> > +                worker->head.deadline = INT64_MAX;
> > +                worker->head.active = item != NULL;
> > +                worker->head.id = item ? item->id : NULL;
> > +
> > +                if( item && item->timeout > 0 )
> > +                    worker->head.deadline = mdate() + item->timeout;
> > +            }
> > +            vlc_cond_broadcast( &worker->head.wait );
> 
> The usage of the CV still does not make sense.

The above broadcast is to signal that the current head has changed,
letting potential waiting threads (that waits for a single task to be
cancelled) know that a change has been made.

In what way does it *"not make sense"*? Constructive feedback and
criticism is always welcome, but you are currently not given me much
to work with.

> So nack.

/F
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170329/ff0d2ec7/attachment.html>