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

[Filip Roséen]

Hi Rémi,

On 2017-03-22 19:56, Rémi Denis-Courmont wrote:

> Le keskiviikkona 22. maaliskuuta 2017, 18.43.44 EET Filip Roséen a écrit :
> > > I think you should not keep reproducing the same.
> > 
> > Where would you recommend putting them? I was struggling to find a
> > good place, but I guess `src/misc` is good enough?
> 
> I don´t particularly like src/misc either, but that still seems saner than 
> src/playlist/

No, me neither - but as it is hard to come up with another suitable
location.. `src/misc` it is (at least for now). Thanks.

> (...)

> > > I don´t see the point in doing that there. You have all the disadvantage
> > > of
> > > insulated private data, and none of the advantages, since the private data
> > > is still exposed to the owner.
> > 
> > The reason there is a *data-member* of type `struct
> > background_worker_private` was because, inspired by some recent
> > changes by you (which I really like), I wanted
> > `background_worker_Init` to be always successful (in order to prevent
> > the number of error-paths).
> 
> I would just inline the private structure into the public one.

So you mean having a `struct` without an anonymous type directly
within `struct background_worker`, that refers to the private
data-members?

Yes, that is certainly another way of doing it but I somewhat like
having a named structure (in terms of type) as one can declare a
dereferencable pointer to it (shorting paths where a lot of access to
the private members are).

One could of course *"de-wrap"* the struct and put the nested
data-members directly inside the "top" layered struct, but I somewhat
wanted a way to denote that the data-members inside the private
section are not to be touched from the outside (even though the only
real protection is in its name).

> Or rather, I would design the thing so that there are no public
> members at all. You can still use init/deinit instead of new/delete
> if you want. Only expose the structure layout is exposed for the
> purpose of allocation/storage.

Noted. I will take all written into consideration as I prepare
patch-batch version `#2`.
 
> > It can of course be moved to be *translation-unit* local to
> > `src/playlist/misc/background_worker.c`.
> 
> > > > +
> > > > +    vlc_mutex_lock( &priv->lock );
> > > > +    for( ;; )
> > > > +    {
> > > > +        vlc_cond_broadcast( &priv->wait );
> > > > +
> > > > +        if( priv->current.handle == NULL )
> > > > +        {
> > > > +            if( vlc_array_count( &priv->queue ) == 0 )
> > > > +                break;
> > > > +
> > > > +            struct background_worker_item* item =
> > > > +                vlc_array_item_at_index( &priv->queue, 0 );
> > > > +
> > > > +            void* handle;
> > > > +            if( !worker->pf_start( priv->owner, item->entity, &handle )
> > > > )
> > > > +            {
> > > > +                priv->current.item = item;
> > > > +                priv->current.handle = handle;
> > > > +                priv->current.deadline = item->timeout > VLC_TS_INVALID
> > > > +                    ? mdate() + item->timeout : INT64_MAX;
> > > > +            }
> > > > +
> > > > +            vlc_array_remove( &priv->queue, 0 );
> > > > +            continue;
> > > > +        }
> > > > +
> > > > +        for( ;; )
> > > > +        {
> > > > +            bool const b_timeout = priv->current.deadline < mdate();
> > > > +
> > > > +            vlc_mutex_unlock( &priv->lock );
> > > > +
> > > > +            if( b_timeout ||
> > > > +                worker->pf_probe( priv->owner, priv->current.handle ) )
> > > > +            {
> > > > +                worker->pf_stop( priv->owner, priv->current.handle );
> > > > +                worker->pf_release( priv->current.item->entity );
> > > > +
> > > > +                vlc_mutex_lock( &priv->lock );
> > > > +                break;
> > > > +            }
> > > > +
> > > > +            vlc_mutex_lock( &priv->lock );
> > > > +            vlc_cond_timedwait( &priv->wait, &priv->lock,
> > > > +                                 priv->current.deadline );
> > > 
> > > This sequence of two instructions can´t be correct, AFAIK.
> > 
> > `(1)`
> > 
> > Oh that was an unfortunate side-effect of some heavy rebasing right
> > before pushing to *vlc-devel*, the signal can of course be lost if the
> > background-task finishes during the time the lock is released.
> > 
> > I will fix this in an upcoming patch, thank you!
> > 
> > > > +        }
> > > > +
> > > > +        priv->current.item = NULL;
> > > > +        priv->current.handle = NULL;
> > > > +    }
> > > > +
> > > > +    priv->active = false;
> > > > +    vlc_mutex_unlock( &priv->lock );
> > > > +    return NULL;
> > > > +}
> > > > +
> > > > +static void BackgroundWorkerCancel( struct background_worker* worker,
> > > > void* id) +{
> > > > +    struct background_worker_private* priv = &worker->p;
> > > > +
> > > > +    vlc_mutex_lock( &priv->lock );
> > > > +    for( size_t i = 0; i < vlc_array_count( &priv->queue ); )
> > > > +    {
> > > > +        struct background_worker_item* item =
> > > > +            vlc_array_item_at_index( &priv->queue, i );
> > > > +
> > > > +        if( id == NULL || item->id == id )
> > > > +        {
> > > > +            vlc_array_remove( &priv->queue, i );
> > > 
> > > Looks like reinventing vlc_array_index_of_item().
> > 
> > I was tempted to introduce an alternative to `vlc_array_index_of_item`
> > that would take a callback so that one could query something other
> > than the item-address itself (but have the same semantics as
> > `vlc_array_index_of_item`).
> 
> > It would simplify the code, but I did not find too many occurances
> > where such a new function would benefit the codebase as a whole, and
> > as it wouldn't be used too many times in `background_worker.c`, I was
> > hesitant to add a *translation-unit* local helper.
> > 
> > > > +            worker->pf_release( item->entity );
> > > 
> > > This seems to assume that the callbacks are reentrant. It might be true.
> > > But it seems like a dangerous design assumption.
> > 
> > A `pf_release` callback that would release the worker in which the
> > item is part of would be the only case where we would end up in a
> > reentrent state, and as I thought that did not make sense I didn't go
> > out to make that a valid usecase.
> 
> Sorry, I am not sure what you are trying to express here.
> 
> My point is that pf_release is also used in function calls, and without the 
> lock. So it has to be reentrant as things stand. And I´m not sure that´s a 
> good idea. At the very least, this would need to be clearly and explicitly 
> documented.

I will try to state things in a clear manner in the upcoming
documentation, and as I plan to change some synchronization details
(given your earlier replies) - it might make more sense discussing
this after those.

Thank you for your swift replies,\
Filip Roséen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20170322/39a6f83c/attachment.html>