Add a 'unique' method to SkRefCnt, document the usage, and add support.

include/core/SkRefCnt.h

 
 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #ifndef SkRefCnt_DEFINED
@@ skipping 23 matching lines @@
 
     /** Destruct, asserting that the reference count is 1.
     */
     virtual ~SkRefCnt() {
 #ifdef SK_DEBUG
         SkASSERT(fRefCnt == 1);
         fRefCnt = 0;    // illegal value, to catch us if we reuse after delete
 #endif
     }
 
-    /** Return the reference count.
-    */
-    int32_t getRefCnt() const { return fRefCnt; }
+    /** Return the reference count. Use only for debugging. */

[bsalomon, 2013/07/15 13:04:08]

It seems a little weird that these obviously call the same function but one is
only for debugging purposes. Maybe we should put this function in a SK_DEBUG
block or not inline it? Do we call it anywhere after this change?

[bungeman-skia, 2013/07/15 16:02:41]

I believe that with this change this is only used in debug blocks. It's actually
somewhat shaky to use it there anyway, but I haven't done anything about it yet
because I haven't reviewed all of them. This is a somewhat odd method to have
around, so reasoning about it is a bit iffy anyway. The return value (other than
1) doesn't really mean anything, and the idea is to have unique handle that,
since that's the only good use for it. Maybe we should make this very heavy
weight, putting full barriers on both sides.

+    int32_t getRefCnt() const { return sk_atomic_unprotected_read(fRefCnt); }
+
+    /** Returns true if the caller is the only owner.
+     *
+     *  This provides no memory barriers.
+     *
+     *  Using the object without additional memory barriers when this returns
+     *  true is safe so long as the object is only modified when this returns
+     *  true and the caller is still the only owner. This is true in the usual
+     *  case of optimizing copy on write semantics.
+     *
+     *  If other threads may have modified the object (even within a mutex),
+     *  then an acquire barrier (L/SL) is required before using the object
+     *  (may already be provided by the load in unique() on some platforms).
+     *  Without this barrier, modifications to the object made by the other
+     *  thread may not yet be visible or complete on the current thread.
+     */
+    bool unique() const {

[Alexander Potapenko, 2013/07/15 15:05:34]

I suggest to add an sk_atomic_acquire_load() function and use it here instead.
That reflects the semantics of unique() (see
https://code.google.com/p/chromium/codesearch#chromium/src/base/atomic_ref_co...
for how this is done in base/) and will be properly understood by TSan if
annotated as such.

In regular x86 sk_atomic_acquire_load() will be a plain load and a compiler
barrier, whereas under #if defined(THREAD_SANITIZER) it'll be a call to
__tsan_atomic32_load(ptr, __tsan_memory_order_acquire) (see
https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/atomicops_i...)

Missing acquire/release semantics for the accesses to the refcounters may result
in more false positives.

[bungeman-skia, 2013/07/15 16:02:41]

Yes, this is addressed in the comment above. Adding such a barrier should be
essentially cheap or free, except that Android is using full barriers for all
memory barrier requests. As a result, using a memory barrier here is rather
heavy on Android (this is called on every path modification, and ARM
traditionally has slow full memory barriers). The reason for this is that, as
stated in the comment, there is no need for any barrier in the case of
implementing copy on write only (as there are no modifications on other threads
to be out of order with). Copy on write is the only thing we ever use this for.
Yes, that is the way Chromium does it, though annoyingly the C++ specification
doesn't seem to require any memory barriers from shared_ptr::unique. Unless I've
missed something, someone really needs to clarify this in the C++ spec.
And, as mentioned above, on Android will be a full memory barrier. Which will
happen on every path modification at least. And we don't need it. Also, an
acquire barrier here is enough for exclusivity (when returning true), but isn't
enough to prevent races. This read would still be a race with previous ref()s,
and in fact still in a race with previous unref()s. In practice this will happen
much less often, but it's still possible. In fact, a previous version of this
was written something like

    bool isUnique = (1 == fRefCnt);
    if (unique) {
        sk_membar__aquire_after_load(); //rmb()
    }
    return unique;

because the memory barrier is totally wasted if we're not returning true,
because returning false provides no information about the state of the reference
count. However, this optimization doesn't help much, since the condition will,
in practice, be true 99.9% of the time. Then I realized that in pure copy on
write situations the barrier isn't needed at all, hence the verbose comment.
False positives of what? A false positive from unique? I don't believe that is
possible. A false positive from tsan? Of course, that's why I'm trying to mark
it.

[Alexander Potapenko, 2013/07/16 14:03:30]

Yes, I was talking about false positives from TSan.
Let thread 1 read a memory location and thread 2 write to it without any
additional synchronization.

Consider these possible cases:
1. Both threads are using regular, nonatomic writes. In this case TSan will
report a data race on the memory location despite of the fact these accesses are
atomic on x86 - it is just too aggressive for the tool to treat every word-sized
memory access as an atomic one.
2. Both threads are using relaxed atomics to access memory. In this case TSan
won't report a race on that particular address, but it may report other races on
nearby memory locations if they're being accessed in a non-atomic fashion.
Annotating every memory access as an atomic one will effectively hide all the
races, both benign and harmful ones.
3. Threads are using Acquire_Load() and Release_Store() operations to express
the synchronization. In this case TSan understands there's a happens-before arc
between the store and the load and takes that into account when looking for
races.

[bungeman-skia, 2013/07/16 15:50:57]

All writes here are atomic. The entire point of this exercise is to get the load
in unique into NoBarrier_Load to address this issue (and ensure that the load
is, itself, atomic).
Yes, that's what the comment above is about. If you're only doing copy on write
(only writing to the owned object when unique returns/would return true) then
tsan won't be reporting races on nearby locations, because there aren't any
races there to begin with. If you are potentially writing to the owned object at
times when unique might not return true, then you must provide an acquire
barrier in the case where unique returns true before using the owned object in
order to ensure that previous writes to the owned object are observed to have
happened. We actually want this behavior from tsan, to detect cases where we are
doing something other than copy on write and not using the appropriate barriers.

Personally I believe that shared_ptrs in general should stick to the pointer
guarantee and shy away from trying to protect the owned object.
 

+        // sk_atomic_unprotected_read forces an atomic read of fRefCnt and
+        // marks the read has a benign race with ref() and unref().
+        return (1 == sk_atomic_unprotected_read(fRefCnt));

[Dmitry Vyukov, 2013/07/16 13:17:51]

sk_atomic_unprotected_read() needs to use Acquire_Load() to support this use
case -- it needs to to synchronize with concurrent dec() operations.

[bungeman-skia, 2013/07/16 15:50:57]

I've tried to clarify via email, but nothing inter-thread synchronizes with
unref except itself. Adding an acquire here also will not do anything about the
current race in question, which is the race with ref. This load racing with both
ref and unref is benign, as we will not provide any information to the caller
unless the reference count is 1. The acquire would only be useful to ensure
modifications to the owned object on other threads happen before looking at the
owned object without whatever locks would normally be used in the case where
unique returns true.

Note that the acquire in unref is actually unneeded (for the same reasons) if
dispose is empty or the owned object is never modified when unique could return
false. This is somewhat unpractical as an optimization, however, since dispose
probably must modify something (even if implicitly) and the general case cannot
be assumed to be copy on write. With unique, however, the primary use is copy on
write, and the use of unique is external and easy to see (as opposed to
dispose).

+    }
 
     /** Increment the reference count. Must be balanced by a call to unref().
     */
     void ref() const {
         SkASSERT(fRefCnt > 0);
         sk_atomic_inc(&fRefCnt);  // No barrier required.
     }
 
     /** Decrement the reference count. If the reference count is 1 before the
         decrement, then delete the object. Note that if this is the case, then
         the object needs to have been allocated via new, and not on the stack.
     */
     void unref() const {
         SkASSERT(fRefCnt > 0);
         // Release barrier (SL/S), if not provided below.
         if (sk_atomic_dec(&fRefCnt) == 1) {
             // Aquire barrier (L/SL), if not provided above.
             // Prevents code in dispose from happening before the decrement.
             sk_membar_aquire__after_atomic_dec();
             internal_dispose();
         }
     }
 
     void validate() const {
         SkASSERT(fRefCnt > 0);
     }
 
     /**
-     *  Alias for ref(), for compatibility with scoped_refptr.
-     */
-    void AddRef() { this->ref(); }
-
-    /**
-     *  Alias for unref(), for compatibility with scoped_refptr.
-     */
-    void Release() { this->unref(); }
-
-    /**
      * Alias for unref(), for compatibility with WTF::RefPtr.
      */
     void deref() { this->unref(); }
 
 protected:
     /**
      *  Allow subclasses to call this if they've overridden internal_dispose
      *  so they can reset fRefCnt before the destructor is called. Should only
      *  be called right before calling through to inherited internal_dispose()
      *  or before calling the destructor.
      */
     void internal_dispose_restore_refcnt_to_1() const {
 #ifdef SK_DEBUG
         SkASSERT(0 == fRefCnt);
         fRefCnt = 1;
 #endif
     }
 
 private:
     /**
      *  Called when the ref count goes to 0.
      */
     virtual void internal_dispose() const {
         this->internal_dispose_restore_refcnt_to_1();
         SkDELETE(this);
     }
 
+    // The following friends are those which override internal_dispose()
+    // and conditionally call SkRefCnt::internal_dispose().
+    friend class GrTexture;
     friend class SkWeakRefCnt;
-    friend class GrTexture;     // to allow GrTexture's internal_dispose to
-                                // call SkRefCnt's & directly set fRefCnt (to 1)
 
     mutable int32_t fRefCnt;
 
     typedef SkNoncopyable INHERITED;
 };
 
 ///////////////////////////////////////////////////////////////////////////////
 
 /** Helper macro to safely assign one SkRefCnt[TS]* to another, checking for
     null in on each side of the assignment, and ensuring that ref() is called
@@ skipping 137 matching lines @@
     typedef T* SkRefPtr::*unspecified_bool_type;
     operator unspecified_bool_type() const {
         return fObj ? &SkRefPtr::fObj : NULL;
     }
 
 private:
     T* fObj;
 };
 
 #endif