[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility
Filip Roséen
filip at atch.se
Wed Mar 29 20:50:50 CEST 2017
Hi Rémi,
On 2017-03-29 21:41, Rémi Denis-Courmont wrote:
> Le keskiviikkona 29. maaliskuuta 2017, 20.34.09 EEST Filip Roséen a écrit :
> > 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.
>
> It looks suspiciously redundant or misplaced like deadline does.
>
> >
> > > > > > + 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.
>
> You are making unwarranted assumptions about condition variables here.
Assumptions based on specifications, I would however be extremely
interested on implimentations where this "assumption" does not hold
(mostly because a issue should definitely be filed based on such
finding).
> Been there done that.
Previous paragraph.
>
> > > 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.
>
> You can never have more than one thread waiting in deletion path. Anything
> else would lead to multiple frees.
I have nver said that there should be two entities waiting for the
preparser deletion, I am talking about cancellations of preparser
requests. It also seems that I am forced to add, even though I thought
it was rather obvious from the implementation and comments, that there
is nothing stating that you cannot remove everything from the
preparser queue twice, or cancel a single request twice, or perhaps
cancel a group of requests by using the same id for all of them,
`background_worker_Destroy` implies `background_worker_Cancel` (with
the `id` argument being NULL to remove everything), but not the other
way around.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170329/024a0d54/attachment.html>
More information about the vlc-devel
mailing list