discuss-gnustep
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: libobjc2 & arc & multithreading


From: David Chisnall
Subject: Re: libobjc2 & arc & multithreading
Date: Mon, 27 Nov 2017 17:33:32 +0000

On 27 Nov 2017, at 09:03, David Chisnall <address@hidden> wrote:
> 
> The other thing that would probably make a bigger difference is to use a bit 
> in the refcount to indicate whether an object has any weak references at all. 
>  This would allow us to completely avoid having the lock.  

The attached patch should improve things a lot for you.  Please can you test it 
and report back?  I’m a bit hesitant to commit it without feedback, because 
there aren’t any multithreaded tests in the libobjc2 test suite and it’s 
possible that I’ve managed to introduce a subtle race condition.

David

diff --git a/arc.m b/arc.m
index b7a3b3b..24f8b7c 100644
--- a/arc.m
+++ b/arc.m
@@ -1,4 +1,5 @@
 #include <stdlib.h>
+#include <stdbool.h>
 #include <assert.h>
 #import "stdio.h"
 #import "objc/runtime.h"
@@ -163,6 +164,17 @@ extern BOOL FastARCAutorelease;
 
 static BOOL useARCAutoreleasePool;
 
+/**
+ * We use the top bit of the reference count to indicate whether an object has
+ * ever had a weak reference taken.  This lets us avoid acquiring the weak
+ * table lock for most objects on deallocation.
+ */
+static const long weak_mask = ((size_t)1)<<((sizeof(size_t)*8)-1);
+/**
+ * All of the bits other than the top bit are the real reference count.
+ */
+static const long refcount_mask = ~weak_mask;
+
 static inline id retain(id obj)
 {
        if (isSmallObject(obj)) { return obj; }
@@ -174,14 +186,40 @@ static inline id retain(id obj)
        }
        if (objc_test_class_flag(cls, objc_class_flag_fast_arc))
        {
-               intptr_t *refCount = ((intptr_t*)obj) - 1;
-               // Note: this should be an atomic read, so that a sufficiently 
clever
-               // compiler doesn't notice that there's no happens-before 
relationship
-               // here.
-               if (*refCount >= 0)
-               {
-                       __sync_add_and_fetch(refCount, 1);
-               }
+               uintptr_t *refCount = ((uintptr_t*)obj) - 1;
+               uintptr_t refCountVal = __sync_fetch_and_add(refCount, 0);
+               uintptr_t newVal = refCountVal;
+               do {
+                       refCountVal = newVal;
+                       long realCount = refCountVal & refcount_mask;
+                       // If this object's reference count is already less 
than 0, then
+                       // this is a spurious retain.  This can happen when one 
thread is
+                       // attempting to acquire a strong reference from a weak 
reference
+                       // and the other thread is attempting to destroy it.  
The
+                       // deallocating thread will decrement the reference 
count with no
+                       // locks held and will then acquire the weak ref table 
lock and
+                       // attempt to zero the weak references.  The caller of 
this will be
+                       // `objc_loadWeakRetained`, which will also hold the 
lock.  If the
+                       // serialisation is such that the locked retain happens 
after the
+                       // decrement, then we return nil here so that the 
weak-to-strong
+                       // transition doesn't happen and the object is actually 
destroyed.
+                       // If the serialisation happens the other way, then the 
locked
+                       // check of the reference count will happen after we've 
referenced
+                       // this and we don't zero the references or deallocate.
+                       if (realCount < 0)
+                       {
+                               return nil;
+                       }
+                       // If the reference count is saturated, don't increment 
it.
+                       if (realCount == refcount_mask)
+                       {
+                               return obj;
+                       }
+                       realCount++;
+                       realCount |= refCountVal & weak_mask;
+                       uintptr_t updated = (uintptr_t)realCount;
+                       newVal = __sync_val_compare_and_swap(refCount, 
refCountVal, updated);
+               } while (newVal != refCountVal);
                return obj;
        }
        return [obj retain];
@@ -203,12 +241,37 @@ static inline void release(id obj)
        }
        if (objc_test_class_flag(cls, objc_class_flag_fast_arc))
        {
-               intptr_t *refCount = ((intptr_t*)obj) - 1;
+               uintptr_t *refCount = ((uintptr_t*)obj) - 1;
+               uintptr_t refCountVal = __sync_fetch_and_add(refCount, 0);
+               uintptr_t newVal = refCountVal;
+               bool isWeak;
+               bool shouldFree;
+               do {
+                       refCountVal = newVal;
+                       size_t realCount = refCountVal & refcount_mask;
+                       // If the reference count is saturated, don't decrement 
it.
+                       if (realCount == refcount_mask)
+                       {
+                               return;
+                       }
+                       realCount--;
+                       isWeak = (refCountVal & weak_mask) == weak_mask;
+                       shouldFree = realCount == -1;
+                       realCount |= refCountVal & weak_mask;
+                       uintptr_t updated = (uintptr_t)realCount;
+                       newVal = __sync_val_compare_and_swap(refCount, 
refCountVal, updated);
+               } while (newVal != refCountVal);
                // We allow refcounts to run into the negative, but should only
                // deallocate once.
-               if (__sync_sub_and_fetch(refCount, 1) == -1)
+               if (shouldFree)
                {
-                       objc_delete_weak_refs(obj);
+                       if (isWeak)
+                       {
+                               if (!objc_delete_weak_refs(obj))
+                               {
+                                       return;
+                               }
+                       }
                        [obj dealloc];
                }
                return;
@@ -524,6 +587,7 @@ void* block_load_weak(void *block);
 
 id objc_storeWeak(id *addr, id obj)
 {
+       LOCK_FOR_SCOPE(&weakRefLock);
        id old = *addr;
 
        BOOL isGlobalObject = (obj == nil) || isSmallObject(obj);
@@ -538,16 +602,44 @@ id objc_storeWeak(id *addr, id obj)
                        isGlobalObject = YES;
                }
        }
-       if (cls && objc_test_class_flag(cls, objc_class_flag_fast_arc))
+       if (obj && cls && objc_test_class_flag(cls, objc_class_flag_fast_arc))
        {
-               intptr_t *refCount = ((intptr_t*)obj) - 1;
-               if (obj && *refCount < 0)
+               uintptr_t *refCount = ((uintptr_t*)obj) - 1;
+               if (obj)
                {
-                       obj = nil;
-                       cls = Nil;
+                       uintptr_t refCountVal = __sync_fetch_and_add(refCount, 
0);
+                       uintptr_t newVal = refCountVal;
+                       do {
+                               refCountVal = newVal;
+                               long realCount = refCountVal & refcount_mask;
+                               // If this object has already been deallocated 
(or is in the
+                               // process of being deallocated) then don't 
bother storing it.
+                               if (realCount < 0)
+                               {
+                                       obj = nil;
+                                       cls = Nil;
+                                       break;
+                               }
+                               // The weak ref flag is monotonic (it is set, 
never cleared) so
+                               // don't bother trying to re-set it.
+                               if ((refCountVal & weak_mask) == weak_mask)
+                               {
+                                       break;
+                               }
+                               // Set the flag in the reference count to 
indicate that a weak
+                               // reference has been taken.
+                               //
+                               // We currently hold the weak ref lock, so 
another thread
+                               // racing to deallocate this object will have 
to wait to do so
+                               // if we manage to do the reference count 
update first.  This
+                               // shouldn't be possible, because `obj` should 
be a strong
+                               // reference and so it shouldn't be possible to 
deallocate it
+                               // while we're assigning it.
+                               uintptr_t updated = ((uintptr_t)realCount | 
weak_mask);
+                               newVal = __sync_val_compare_and_swap(refCount, 
refCountVal, updated);
+                       } while (newVal != refCountVal);
                }
        }
-       LOCK_FOR_SCOPE(&weakRefLock);
        if (nil != old)
        {
                WeakRef *oldRef = weak_ref_table_get(weakRefs, old);
@@ -583,7 +675,8 @@ id objc_storeWeak(id *addr, id obj)
        }
        else if (objc_test_class_flag(cls, objc_class_flag_fast_arc))
        {
-               if ((*(((intptr_t*)obj) - 1)) < 0)
+               uintptr_t *refCount = ((uintptr_t*)obj) - 1;
+               if ((long)((__sync_fetch_and_add(refCount, 0) & refcount_mask)) 
< 0)
                {
                        return nil;
                }
@@ -648,20 +741,38 @@ static void zeroRefs(WeakRef *ref, BOOL shouldFree)
        }
 }
 
-void objc_delete_weak_refs(id obj)
+BOOL objc_delete_weak_refs(id obj)
 {
        LOCK_FOR_SCOPE(&weakRefLock);
+       if (objc_test_class_flag(classForObject(obj), objc_class_flag_fast_arc))
+       {
+               // If another thread has done a load of a weak reference, then 
it will
+               // have incremented the reference count with the lock held.  It 
may
+               // have done so in between this thread's decrementing the 
reference
+               // count and its acquiring the lock.  In this case, report 
failure.
+               uintptr_t *refCount = ((uintptr_t*)obj) - 1;
+               if ((long)((__sync_fetch_and_add(refCount, 0) & refcount_mask)) 
< 0)
+               {
+                       return NO;
+               }
+       }
        WeakRef *oldRef = weak_ref_table_get(weakRefs, obj);
        if (0 != oldRef)
        {
                zeroRefs(oldRef, NO);
                weak_ref_remove(weakRefs, obj);
        }
+       return YES;
 }
 
 id objc_loadWeakRetained(id* addr)
 {
        LOCK_FOR_SCOPE(&weakRefLock);
+       // *addr can only be zeroed by another thread if it holds the 
weakRefLock.
+       // It is possible for another thread to zero the reference count here, 
but
+       // it will then acquire the weak ref lock prior to zeroing the weak
+       // references and deallocating the object.  If this thread has 
incremented
+       // the reference count, then it will skip deallocating.
        id obj = *addr;
        if (nil == obj) { return nil; }
        // Small objects don't need reference count modification
@@ -674,14 +785,7 @@ id objc_loadWeakRetained(id* addr)
        {
                obj = block_load_weak(obj);
        }
-       else if (objc_test_class_flag(cls, objc_class_flag_fast_arc))
-       {
-               if ((*(((intptr_t*)obj) - 1)) < 0)
-               {
-                       return nil;
-               }
-       }
-       else
+       else if (!objc_test_class_flag(cls, objc_class_flag_fast_arc))
        {
                obj = _objc_weak_load(obj);
        }
diff --git a/objc/objc-arc.h b/objc/objc-arc.h
index 0da4030..203cfef 100644
--- a/objc/objc-arc.h
+++ b/objc/objc-arc.h
@@ -94,9 +94,11 @@ void objc_release(id obj);
  * weak pointers will return 0.  This function should be called in -release,
  * before calling [self dealloc].
  *
+ * This will return `YES` if the weak references were deleted, `NO` otherwise.
+ *
  * Nonstandard extension.
  */
-void objc_delete_weak_refs(id obj);
+BOOL objc_delete_weak_refs(id obj);
 /**
  * Returns the total number of objects in the ARC-managed autorelease pool.
  */




reply via email to

[Prev in Thread] Current Thread [Next in Thread]