[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility
Rémi Denis-Courmont
remi at remlab.net
Wed Mar 29 20:16:51 CEST 2017
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.
> 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.
>
> > > + 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.
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.
So the only case that one waits on the worker is at deletion.
--
雷米‧德尼-库尔蒙
https://www.remlab.net/
More information about the vlc-devel
mailing list