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

Filip Roséen filip at atch.se
Wed Mar 29 20:34:09 CEST 2017


Hi Rémi,

On 2017-03-29 21:16, Rémi Denis-Courmont wrote:

> Le keskiviikkona 29. maaliskuuta 2017, 19.21.07 EEST Filip Roséen a écrit :
> > 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.
> 
> That´s extremely confusingly named.

What would you propose would be a better name?

> > 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).
> 
> Same as deadline.

I still do not see what you are saying.

> > 
> > > > +        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.
> 
> No, the thread just wakes its own self up, which makes no sense.

The thread is per definition not waiting for the conditiona variable
at that time, so I cannot see how it would wake itself up.

> Also it´s highly questionable that more than one thread should ever wait on 
> the worker. Normally, we probe the item, we set the flag and release the item. 
> And move onto the next item, or exit.

I am taking the safe route as this path is also invoked from users of
*libvlc*. We do not control who waits for what, and given that it is
not documented that a single waiter is max (by intention given the
implementaiton) the code is written as it is.

The implementation also takes into consideration that we _maybe_ would
like to have concurrently running tasks in the future (which this
implementation would allow with very few changes), but that's a whole
other discussion.

> So the only case that one waits on the worker is at deletion.

See previous paragraph.

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


More information about the vlc-devel mailing list