[vlc-devel] [PATCH] core: Add atomic refcounter helper

Steve Lhomme robux4 at ycbcr.xyz
Sat Jun 30 08:33:13 CEST 2018


Seems interesting. I like the part where he mentions avoiding writes when possible (mutex in his case). Something I usually try to do.

The only issue I have with the code is the use of assert in a header. But if there are other cases LGTM

> On 29 Jun 2018, at 21:03, Romain Vimont <rom1v at videolabs.io> wrote:
> 
> Implement an atomic refcounter with a weak but correct (1) memory order,
> and expose a simple API.
> 
> Use it in some existing code to demo its usage.
> 
> (1) See for example "Using weakly ordered C++ atomics correctly" by Hans
>    Boehm at CppCon 2016 (the refcounting part also applies to C11):
>    <https://www.youtube.com/watch?v=M15UKpNlpeM&t=45m15s>
> ---
> include/vlc_atomic.h | 25 +++++++++++++++++++++++++
> src/input/item.c     |  6 +++---
> src/input/item.h     |  4 ++--
> src/input/resource.c | 10 +++++-----
> src/misc/picture.c   |  9 +++------
> src/misc/picture.h   |  5 ++---
> 6 files changed, 40 insertions(+), 19 deletions(-)
> 
> diff --git a/include/vlc_atomic.h b/include/vlc_atomic.h
> index 868918fe8e6..2cb5e23f489 100644
> --- a/include/vlc_atomic.h
> +++ b/include/vlc_atomic.h
> @@ -30,6 +30,7 @@
>  * Atomic operations do not require locking, but they are not very powerful.
>  */
> 
> +# include <assert.h>
> # include <stdatomic.h>
> 
> typedef atomic_uint_least32_t vlc_atomic_float;
> @@ -57,4 +58,28 @@ static inline void vlc_atomic_store_float(vlc_atomic_float *atom, float f)
>     atomic_store(atom, u.i);
> }
> 
> +typedef atomic_uint vlc_atomic_rc_t;
> +
> +/** Init the RC to 1 */
> +static inline void vlc_atomic_rc_init(vlc_atomic_rc_t *var)
> +{
> +    atomic_init(var, 1);
> +}
> +
> +/** Increment the RC */
> +static inline void vlc_atomic_rc_inc(vlc_atomic_rc_t *var)
> +{
> +    unsigned prev = atomic_fetch_add_explicit(var, 1, memory_order_relaxed);
> +    assert(prev);
> +    VLC_UNUSED(prev);
> +}
> +
> +/** Decrement the RC and return true if it reaches 0 */
> +static inline bool vlc_atomic_rc_dec(vlc_atomic_rc_t *var)
> +{
> +    unsigned prev = atomic_fetch_sub_explicit(var, 1, memory_order_acq_rel);
> +    assert(prev);
> +    return prev == 1;
> +}
> +
> #endif
> diff --git a/src/input/item.c b/src/input/item.c
> index 6789f73caa5..8e846425b4b 100644
> --- a/src/input/item.c
> +++ b/src/input/item.c
> @@ -471,7 +471,7 @@ input_item_t *input_item_Hold( input_item_t *p_item )
> {
>     input_item_owner_t *owner = item_owner(p_item);
> 
> -    atomic_fetch_add( &owner->refs, 1 );
> +    vlc_atomic_rc_inc( &owner->rc );
>     return p_item;
> }
> 
> @@ -479,7 +479,7 @@ void input_item_Release( input_item_t *p_item )
> {
>     input_item_owner_t *owner = item_owner(p_item);
> 
> -    if( atomic_fetch_sub(&owner->refs, 1) != 1 )
> +    if( !vlc_atomic_rc_dec( &owner->rc ) )
>         return;
> 
>     vlc_event_manager_fini( &p_item->event_manager );
> @@ -1060,7 +1060,7 @@ input_item_NewExt( const char *psz_uri, const char *psz_name,
>     if( unlikely(owner == NULL) )
>         return NULL;
> 
> -    atomic_init( &owner->refs, 1 );
> +    vlc_atomic_rc_init( &owner->rc );
> 
>     input_item_t *p_input = &owner->item;
>     vlc_event_manager_t * p_em = &p_input->event_manager;
> diff --git a/src/input/item.h b/src/input/item.h
> index dce1dfc0584..bce49777f8d 100644
> --- a/src/input/item.h
> +++ b/src/input/item.h
> @@ -25,7 +25,7 @@
> #define LIBVLC_INPUT_ITEM_H 1
> 
> #include "input_interface.h"
> -#include <stdatomic.h>
> +#include <vlc_atomic.h>
> 
> void input_item_SetErrorWhenReading( input_item_t *p_i, bool b_error );
> void input_item_UpdateTracksInfo( input_item_t *item, const es_format_t *fmt );
> @@ -34,7 +34,7 @@ bool input_item_ShouldPreparseSubItems( input_item_t *p_i );
> typedef struct input_item_owner
> {
>     input_item_t item;
> -    atomic_uint refs;
> +    vlc_atomic_rc_t rc;
> } input_item_owner_t;
> 
> # define item_owner(item) ((struct input_item_owner *)(item))
> diff --git a/src/input/resource.c b/src/input/resource.c
> index f806cfeb2c9..0cad860b106 100644
> --- a/src/input/resource.c
> +++ b/src/input/resource.c
> @@ -28,10 +28,10 @@
> # include "config.h"
> #endif
> 
> -#include <stdatomic.h>
> #include <assert.h>
> 
> #include <vlc_common.h>
> +#include <vlc_atomic.h>
> #include <vlc_vout.h>
> #include <vlc_spu.h>
> #include <vlc_aout.h>
> @@ -45,7 +45,7 @@
> 
> struct input_resource_t
> {
> -    atomic_uint    refs;
> +    vlc_atomic_rc_t rc;
> 
>     vlc_object_t   *p_parent;
> 
> @@ -422,7 +422,7 @@ input_resource_t *input_resource_New( vlc_object_t *p_parent )
>     if( !p_resource )
>         return NULL;
> 
> -    atomic_init( &p_resource->refs, 1 );
> +    vlc_atomic_rc_init( &p_resource->rc );
>     p_resource->p_parent = p_parent;
>     vlc_mutex_init( &p_resource->lock );
>     vlc_mutex_init( &p_resource->lock_hold );
> @@ -431,7 +431,7 @@ input_resource_t *input_resource_New( vlc_object_t *p_parent )
> 
> void input_resource_Release( input_resource_t *p_resource )
> {
> -    if( atomic_fetch_sub( &p_resource->refs, 1 ) != 1 )
> +    if( !vlc_atomic_rc_dec( &p_resource->rc ) )
>         return;
> 
>     DestroySout( p_resource );
> @@ -446,7 +446,7 @@ void input_resource_Release( input_resource_t *p_resource )
> 
> input_resource_t *input_resource_Hold( input_resource_t *p_resource )
> {
> -    atomic_fetch_add( &p_resource->refs, 1 );
> +    vlc_atomic_rc_inc( &p_resource->rc );
>     return p_resource;
> }
> 
> diff --git a/src/misc/picture.c b/src/misc/picture.c
> index e633cee42bc..5cabd420aab 100644
> --- a/src/misc/picture.c
> +++ b/src/misc/picture.c
> @@ -205,7 +205,7 @@ static picture_priv_t *picture_NewPrivate(const video_format_t *restrict p_fmt)
>         return NULL;
>     }
> 
> -    atomic_init( &priv->gc.refs, 1 );
> +    vlc_atomic_rc_init( &priv->gc.rc );
>     priv->gc.opaque = NULL;
> 
>     return priv;
> @@ -305,8 +305,7 @@ picture_t *picture_Hold( picture_t *p_picture )
>     assert( p_picture != NULL );
> 
>     picture_priv_t *priv = (picture_priv_t *)p_picture;
> -    uintptr_t refs = atomic_fetch_add( &priv->gc.refs, 1 );
> -    assert( refs > 0 );
> +    vlc_atomic_rc_inc( &priv->gc.rc );
>     return p_picture;
> }
> 
> @@ -315,9 +314,7 @@ void picture_Release( picture_t *p_picture )
>     assert( p_picture != NULL );
> 
>     picture_priv_t *priv = (picture_priv_t *)p_picture;
> -    uintptr_t refs = atomic_fetch_sub( &priv->gc.refs, 1 );
> -    assert( refs != 0 );
> -    if( refs > 1 )
> +    if( !vlc_atomic_rc_dec( &priv->gc.rc ) )
>         return;
> 
>     PictureDestroyContext( p_picture );
> diff --git a/src/misc/picture.h b/src/misc/picture.h
> index 70ee64878da..4a70b6a6c53 100644
> --- a/src/misc/picture.h
> +++ b/src/misc/picture.h
> @@ -18,8 +18,7 @@
>  * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
>  *****************************************************************************/
> 
> -#include <stdatomic.h>
> -
> +#include <vlc_atomic.h>
> #include <vlc_picture.h>
> 
> typedef struct
> @@ -27,7 +26,7 @@ typedef struct
>     picture_t picture;
>     struct
>     {
> -        atomic_uintptr_t refs;
> +        vlc_atomic_rc_t rc;
>         void (*destroy)(picture_t *);
>         void *opaque;
>     } gc;
> -- 
> 2.18.0
> 
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> https://mailman.videolan.org/listinfo/vlc-devel



More information about the vlc-devel mailing list