[vlc-devel] [PATCH] core: Add atomic refcounter helper
Rémi Denis-Courmont
remi at remlab.net
Sat Jun 30 22:28:14 CEST 2018
The only times I recall that somebody screwed up reference counting where nontrivial cases not handled by this patch.
And the Boolean result is not obvious at all. As far as error proneness comes, this is actually *worse*.
Le 30 juin 2018 22:19:12 GMT+02:00, Romain Vimont <rom1v at videolabs.io> a écrit :
>On Sat, Jun 30, 2018 at 09:12:27PM +0200, Rémi Denis-Courmont wrote:
>> Hei,
>>
>> It is not that simple. Leaving aside that asserts in public headers
>are troublesome, there is a lot of variance in reference counters:
>> - type sizes (uint, uintptr...),
>
>(which refcount would need uintptr_t?)
>
>> - order on decrement - acq_rel is not always needed,
>
>That's not a problem to guarantee a memory order stronger than
>required.
>
>> - corner cases where the decrement is known to not reach zero,
>
>Like?
>
>> - corner cases where the decrement cannot be permitted to reach
>zero(e.g. vlc_object_t, IIRC).
>>
>> We used to have a wrapper and it was removed because it turned more
>into an inconvenience.
>
>Even if it does not fit all (corner) cases, I think it is worth having
>a
>simple refcounter for the most general case (a refcounted structure).
>
>In the typical case:
>
> if (vlc_atomic_rc_dec(rc))
> destroy_object();
>
>is simpler and less error-prone than:
>
> if (atomic_fetch_sub_explicit(refs, 1, memory_order_acq_rel) == 1)
> destroy_object();
>
>> Le 29 juin 2018 21:03:49 GMT+02:00, Romain Vimont
><rom1v at videolabs.io> a écrit :
>> >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
>>
>> --
>> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez
>excuser ma brièveté.
>
>> _______________________________________________
>> vlc-devel mailing list
>> To unsubscribe or modify your subscription options:
>> https://mailman.videolan.org/listinfo/vlc-devel
>
>_______________________________________________
>vlc-devel mailing list
>To unsubscribe or modify your subscription options:
>https://mailman.videolan.org/listinfo/vlc-devel
--
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20180630/69a07411/attachment.html>
More information about the vlc-devel
mailing list