[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