<html><head></head><body>The only times I recall that somebody screwed up reference counting where nontrivial cases not handled by this patch.<br>
<br>
And the Boolean result is not obvious at all. As far as error proneness comes, this is actually *worse*.<br><br><div class="gmail_quote">Le 30 juin 2018 22:19:12 GMT+02:00, Romain Vimont <rom1v@videolabs.io> a écrit :<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<pre class="k9mail">On Sat, Jun 30, 2018 at 09:12:27PM +0200, Rémi Denis-Courmont wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> Hei,<br> <br> It is not that simple. Leaving aside that asserts in public headers are troublesome, there is a lot of variance in reference counters:<br> - type sizes (uint, uintptr...),<br></blockquote><br>(which refcount would need uintptr_t?)<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> - order on decrement - acq_rel is not always needed,<br></blockquote><br>That's not a problem to guarantee a memory order stronger than required.<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> - corner cases where the decrement is known to not reach zero,<br></blockquote><br>Like?<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> - corner cases where the decrement cannot be permitted to reach zero(e.g. vlc_object_t, IIRC).<br> <br> We used to have a wrapper and it was removed because it turned more into an inconvenience.<br></blockquote><br>Even if it does not fit all (corner) cases, I think it is worth having a<br>simple refcounter for the most general case (a refcounted structure).<br><br>In the typical case:<br><br> if (vlc_atomic_rc_dec(rc))<br> destroy_object();<br><br>is simpler and less error-prone than:<br><br> if (atomic_fetch_sub_explicit(refs, 1, memory_order_acq_rel) == 1)<br> destroy_object();<br><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"> Le 29 juin 2018 21:03:49 GMT+02:00, Romain Vimont <rom1v@videolabs.io> a écrit :<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #ad7fa8; padding-left: 1ex;">Implement an atomic refcounter with a weak but correct (1) memory<br>order,<br>and expose a simple API.<br><br>Use it in some existing code to demo its usage.<br><br>(1) See for example "Using weakly ordered C++ atomics correctly" by<br>Hans<br> Boehm at CppCon 2016 (the refcounting part also applies to C11):<br> <https://www.youtube.com/watch?v=M15UKpNlpeM&t=45m15s><br>---<br> include/vlc_atomic.h | 25 +++++++++++++++++++++++++<br> src/input/item.c | 6 +++---<br> src/input/item.h | 4 ++--<br> src/input/resource.c | 10 +++++-----<br> src/misc/picture.c | 9 +++------<br> src/misc/picture.h | 5 ++---<br> 6 files changed, 40 insertions(+), 19 deletions(-)<br><br>diff --git a/include/vlc_atomic.h b/include/vlc_atomic.h<br>index 868918fe8e6..2cb5e23f489 100644<br>--- a/include/vlc_atomic.h<br>+++ b/include/vlc_atomic.h<br>@@ -30,6 +30,7 @@<br>* Atomic operations do not require locking, but they are not very<br>powerful.<br> */<br> <br>+# include <assert.h><br> # include <stdatomic.h><br> <br> typedef atomic_uint_least32_t vlc_atomic_float;<br>@@ -57,4 +58,28 @@ static inline void<br>vlc_atomic_store_float(vlc_atomic_float *atom, float f)<br> atomic_store(atom, u.i);<br> }<br> <br>+typedef atomic_uint vlc_atomic_rc_t;<br>+<br>+/** Init the RC to 1 */<br>+static inline void vlc_atomic_rc_init(vlc_atomic_rc_t *var)<br>+{<br>+ atomic_init(var, 1);<br>+}<br>+<br>+/** Increment the RC */<br>+static inline void vlc_atomic_rc_inc(vlc_atomic_rc_t *var)<br>+{<br>+ unsigned prev = atomic_fetch_add_explicit(var, 1,<br>memory_order_relaxed);<br>+ assert(prev);<br>+ VLC_UNUSED(prev);<br>+}<br>+<br>+/** Decrement the RC and return true if it reaches 0 */<br>+static inline bool vlc_atomic_rc_dec(vlc_atomic_rc_t *var)<br>+{<br>+ unsigned prev = atomic_fetch_sub_explicit(var, 1,<br>memory_order_acq_rel);<br>+ assert(prev);<br>+ return prev == 1;<br>+}<br>+<br> #endif<br>diff --git a/src/input/item.c b/src/input/item.c<br>index 6789f73caa5..8e846425b4b 100644<br>--- a/src/input/item.c<br>+++ b/src/input/item.c<br>@@ -471,7 +471,7 @@ input_item_t *input_item_Hold( input_item_t *p_item<br>)<br> {<br> input_item_owner_t *owner = item_owner(p_item);<br> <br>- atomic_fetch_add( &owner->refs, 1 );<br>+ vlc_atomic_rc_inc( &owner->rc );<br> return p_item;<br> }<br> <br>@@ -479,7 +479,7 @@ void input_item_Release( input_item_t *p_item )<br> {<br> input_item_owner_t *owner = item_owner(p_item);<br> <br>- if( atomic_fetch_sub(&owner->refs, 1) != 1 )<br>+ if( !vlc_atomic_rc_dec( &owner->rc ) )<br> return;<br> <br> vlc_event_manager_fini( &p_item->event_manager );<br>@@ -1060,7 +1060,7 @@ input_item_NewExt( const char *psz_uri, const<br>char *psz_name,<br> if( unlikely(owner == NULL) )<br> return NULL;<br> <br>- atomic_init( &owner->refs, 1 );<br>+ vlc_atomic_rc_init( &owner->rc );<br> <br> input_item_t *p_input = &owner->item;<br> vlc_event_manager_t * p_em = &p_input->event_manager;<br>diff --git a/src/input/item.h b/src/input/item.h<br>index dce1dfc0584..bce49777f8d 100644<br>--- a/src/input/item.h<br>+++ b/src/input/item.h<br>@@ -25,7 +25,7 @@<br> #define LIBVLC_INPUT_ITEM_H 1<br> <br> #include "input_interface.h"<br>-#include <stdatomic.h><br>+#include <vlc_atomic.h><br> <br>void input_item_SetErrorWhenReading( input_item_t *p_i, bool b_error );<br>void input_item_UpdateTracksInfo( input_item_t *item, const es_format_t<br>*fmt );<br>@@ -34,7 +34,7 @@ bool input_item_ShouldPreparseSubItems( input_item_t<br>*p_i );<br> typedef struct input_item_owner<br> {<br> input_item_t item;<br>- atomic_uint refs;<br>+ vlc_atomic_rc_t rc;<br> } input_item_owner_t;<br> <br> # define item_owner(item) ((struct input_item_owner *)(item))<br>diff --git a/src/input/resource.c b/src/input/resource.c<br>index f806cfeb2c9..0cad860b106 100644<br>--- a/src/input/resource.c<br>+++ b/src/input/resource.c<br>@@ -28,10 +28,10 @@<br> # include "config.h"<br> #endif<br> <br>-#include <stdatomic.h><br> #include <assert.h><br> <br> #include <vlc_common.h><br>+#include <vlc_atomic.h><br> #include <vlc_vout.h><br> #include <vlc_spu.h><br> #include <vlc_aout.h><br>@@ -45,7 +45,7 @@<br> <br> struct input_resource_t<br> {<br>- atomic_uint refs;<br>+ vlc_atomic_rc_t rc;<br> <br> vlc_object_t *p_parent;<br> <br>@@ -422,7 +422,7 @@ input_resource_t *input_resource_New( vlc_object_t<br>*p_parent )<br> if( !p_resource )<br> return NULL;<br> <br>- atomic_init( &p_resource->refs, 1 );<br>+ vlc_atomic_rc_init( &p_resource->rc );<br> p_resource->p_parent = p_parent;<br> vlc_mutex_init( &p_resource->lock );<br> vlc_mutex_init( &p_resource->lock_hold );<br>@@ -431,7 +431,7 @@ input_resource_t *input_resource_New( vlc_object_t<br>*p_parent )<br> <br> void input_resource_Release( input_resource_t *p_resource )<br> {<br>- if( atomic_fetch_sub( &p_resource->refs, 1 ) != 1 )<br>+ if( !vlc_atomic_rc_dec( &p_resource->rc ) )<br> return;<br> <br> DestroySout( p_resource );<br>@@ -446,7 +446,7 @@ void input_resource_Release( input_resource_t<br>*p_resource )<br> <br> input_resource_t *input_resource_Hold( input_resource_t *p_resource )<br> {<br>- atomic_fetch_add( &p_resource->refs, 1 );<br>+ vlc_atomic_rc_inc( &p_resource->rc );<br> return p_resource;<br> }<br> <br>diff --git a/src/misc/picture.c b/src/misc/picture.c<br>index e633cee42bc..5cabd420aab 100644<br>--- a/src/misc/picture.c<br>+++ b/src/misc/picture.c<br>@@ -205,7 +205,7 @@ static picture_priv_t *picture_NewPrivate(const<br>video_format_t *restrict p_fmt)<br> return NULL;<br> }<br> <br>- atomic_init( &priv->gc.refs, 1 );<br>+ vlc_atomic_rc_init( &priv->gc.rc );<br> priv->gc.opaque = NULL;<br> <br> return priv;<br>@@ -305,8 +305,7 @@ picture_t *picture_Hold( picture_t *p_picture )<br> assert( p_picture != NULL );<br> <br> picture_priv_t *priv = (picture_priv_t *)p_picture;<br>- uintptr_t refs = atomic_fetch_add( &priv->gc.refs, 1 );<br>- assert( refs > 0 );<br>+ vlc_atomic_rc_inc( &priv->gc.rc );<br> return p_picture;<br> }<br> <br>@@ -315,9 +314,7 @@ void picture_Release( picture_t *p_picture )<br> assert( p_picture != NULL );<br> <br> picture_priv_t *priv = (picture_priv_t *)p_picture;<br>- uintptr_t refs = atomic_fetch_sub( &priv->gc.refs, 1 );<br>- assert( refs != 0 );<br>- if( refs > 1 )<br>+ if( !vlc_atomic_rc_dec( &priv->gc.rc ) )<br> return;<br> <br> PictureDestroyContext( p_picture );<br>diff --git a/src/misc/picture.h b/src/misc/picture.h<br>index 70ee64878da..4a70b6a6c53 100644<br>--- a/src/misc/picture.h<br>+++ b/src/misc/picture.h<br>@@ -18,8 +18,7 @@<br> * Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.<br>*****************************************************************************/<br> <br>-#include <stdatomic.h><br>-<br>+#include <vlc_atomic.h><br> #include <vlc_picture.h><br> <br> typedef struct<br>@@ -27,7 +26,7 @@ typedef struct<br> picture_t picture;<br> struct<br> {<br>- atomic_uintptr_t refs;<br>+ vlc_atomic_rc_t rc;<br> void (*destroy)(picture_t *);<br> void *opaque;<br> } gc;<br>-- <br>2.18.0<br><br><hr><br>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></blockquote> <br> -- <br> Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.<br></blockquote><br><blockquote class="gmail_quote" style="margin: 0pt 0pt 1ex 0.8ex; border-left: 1px solid #729fcf; padding-left: 1ex;"><hr><br> vlc-devel mailing list<br> To unsubscribe or modify your subscription options:<br> <a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a><br></blockquote><br><hr><br>vlc-devel mailing list<br>To unsubscribe or modify your subscription options:<br><a href="https://mailman.videolan.org/listinfo/vlc-devel">https://mailman.videolan.org/listinfo/vlc-devel</a></pre></blockquote></div><br>
-- <br>
Envoyé de mon appareil Android avec Courriel K-9 Mail. Veuillez excuser ma brièveté.</body></html>