[vlc-devel] [PATCH 1/7] playlist/background_worker: introduce background-worker utility
Filip Roséen
filip at atch.se
Wed Mar 29 19:33:30 CEST 2017
Hi Rémi,
I realized that I somehow missed the above quotation in your reply,
but I hope my previous reply still explains the rationale behind it.
> > 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.
If the thread is doing nothing, it is indeed doing nothing (and will
simply set `active = false` and return from its function passed to
`vlc_clone_detach`.
/F
On 2017-03-29 19:21, Filip Roséen wrote:
> 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/4aca1c31/attachment.html>
More information about the vlc-devel
mailing list