[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