Make base::WeakPtr::Get() fast

base/memory/weak_ptr.h

Index: base/memory/weak_ptr.h
diff --git a/base/memory/weak_ptr.h b/base/memory/weak_ptr.h
index 06e65669954007c0c940410c0a6bd0adf5b44008..fbeeb20f60b42973536442bf1e45b70c459fd183 100644
--- a/base/memory/weak_ptr.h
+++ b/base/memory/weak_ptr.h
@@ -90,27 +90,17 @@ namespace internal {
 
 class BASE_EXPORT WeakReference {
  public:
-
-  class BASE_EXPORT NewFlag {
+  // Although Flag is bound to a specific SequencedTaskRunner, it may be
+  // deleted from another via base::WeakPtr::~WeakPtr().
+  class BASE_EXPORT Flag : public RefCountedThreadSafe<Flag> {
    public:
-    constexpr static uintptr_t kTrueMask = ~static_cast<uintptr_t>(0);
-
-    static NewFlag *NullFlag() { return &g_null_flag; }
-    NewFlag() : is_valid_(kTrueMask), ref_count_(0) {
-#if DCHECK_IS_ON()
-      // Flags only become bound when checked for validity, or invalidated,
-      // so that we can check that later validity/invalidation operations on
-      // the same Flag take place on the same sequenced thread.
-      sequence_checker_.DetachFromSequence();
-#endif
-    }
+    Flag();
+    static Flag* nullFlag();
 
     void Invalidate() {
 #if DCHECK_IS_ON()
-      if (this == NullFlag()) {
-        DCHECK(!is_valid_);
+      if (this == nullFlag())
         return;
-      }
       // The flag being invalidated with a single ref implies that there are no
       // weak pointers in existence. Allow deletion on other thread in this case.
       DCHECK(sequence_checker_.CalledOnValidSequence() || HasOneRef())
@@ -118,12 +108,11 @@ class BASE_EXPORT WeakReference {
 #endif
       is_valid_ = 0;
     }
-
     uintptr_t IsValid() const {
 #if DCHECK_IS_ON()
-      if (this == NullFlag()) {
+      if (this == nullFlag()) {
         DCHECK(!is_valid_);
-        return false;
+        return 0;
       }
       DCHECK(sequence_checker_.CalledOnValidSequence())
           << "WeakPtrs must be checked on the same sequenced thread.";
@@ -131,52 +120,24 @@ class BASE_EXPORT WeakReference {
       return is_valid_;
     }
 
-    uintptr_t NumRefs() const { return ref_count_; }
-
-    void AddRef() const { AtomicRefCountInc(&ref_count_); }
-    void Release() const {
-      if (!AtomicRefCountDec(&ref_count_))
-        delete this;
-    }
-
    private:
-    ~NewFlag() = default;
+    friend class base::RefCountedThreadSafe<Flag>;
+
     enum NullFlagTag { kNullFlagTag };
-#if DCHECK_IS_ON()
-    NewFlag(NullFlagTag);
-#else
-    constexpr NewFlag(NullFlagTag);
-#endif
+    Flag(NullFlagTag);
 
-    static NewFlag g_null_flag;
+    ~Flag();
 
     uintptr_t is_valid_;
-    mutable AtomicRefCount ref_count_;
 #if DCHECK_IS_ON()
+    // Even if SequenceChecker is an empty class in non-dcheck builds, it still
+    // takes up space in the class.
     SequenceChecker sequence_checker_;
 #endif
   };
 
-  // Although Flag is bound to a specific SequencedTaskRunner, it may be
-  // deleted from another via base::WeakPtr::~WeakPtr().
-  class BASE_EXPORT Flag : public RefCountedThreadSafe<Flag> {
-   public:
-    Flag();
-
-    void Invalidate();
-    bool IsValid() const;
-
-   private:
-    friend class base::RefCountedThreadSafe<Flag>;
-
-    ~Flag();
-
-    SequenceChecker sequence_checker_;
-    bool is_valid_;
-  };
-
   WeakReference();
-  explicit WeakReference(/*const Flag* flag,*/ const NewFlag* new_flag);
+  explicit WeakReference(const Flag* flag);
   ~WeakReference();
 
   WeakReference(WeakReference&& other);
@@ -184,11 +145,12 @@ class BASE_EXPORT WeakReference {
   WeakReference& operator=(WeakReference&& other) = default;
   WeakReference& operator=(const WeakReference& other) = default;
 
-  uintptr_t is_valid() const { return new_flag_->IsValid(); }
+  uintptr_t is_valid() const { return flag_->IsValid(); }
 
  private:
-  //scoped_refptr<const Flag> flag_;
-  scoped_refptr<const NewFlag> new_flag_;
+  // Note: To avoid null-checks, flag_ always points to either Flag::nullFlag()
+  // or some other object.
+  scoped_refptr<const Flag> flag_;
 };
 
 class BASE_EXPORT WeakReferenceOwner {
@@ -198,21 +160,12 @@ class BASE_EXPORT WeakReferenceOwner {
 
   WeakReference GetRef() const;
 
-  bool HasRefs() const {
-    return (new_flag_->IsValid() & new_flag_->NumRefs()) > 1;
-
-#if 0

[Nico, 2017/06/27 22:05:41]

This looks like your diffbase might be off?

[hans, 2017/06/27 22:09:50]

Yeah, something is not right here :-(

-    return new_flag_ != WeakReference::NewFlag::NullFlag() &&
-           !new_flag_->HasOneRef();
-#endif
-    //return flag_.get() && !flag_->HasOneRef();
-  }
+  bool HasRefs() const { return flag_->IsValid() && !flag_->HasOneRef(); }
 
   void Invalidate();
 
  private:
-  //mutable scoped_refptr<WeakReference::Flag> flag_;
-  mutable scoped_refptr<WeakReference::NewFlag> new_flag_;
+  mutable scoped_refptr<WeakReference::Flag> flag_;
 };
 
 // This class simplifies the implementation of WeakPtr's type conversion
@@ -352,11 +305,7 @@ bool operator==(std::nullptr_t, const WeakPtr<T>& weak_ptr) {
   return weak_ptr == nullptr;
 }
 
-// A class may be composed of a WeakPtrFactory and thereby
-// control how it exposes weak pointers to itself.  This is helpful if you only
-// need weak pointers within the implementation of a class.  This class is also
-// useful when working with primitive types.  For example, you could have a
-// WeakPtrFactory<bool> that is used to pass around a weak reference to a bool.
+namespace internal {
 class BASE_EXPORT WeakPtrFactoryBase {
  protected:
   WeakPtrFactoryBase(uintptr_t ptr);
@@ -364,17 +313,25 @@ class BASE_EXPORT WeakPtrFactoryBase {
   internal::WeakReferenceOwner weak_reference_owner_;
   uintptr_t ptr_;
 };
+}  // namespace internal
 
+// A class may be composed of a WeakPtrFactory and thereby
+// control how it exposes weak pointers to itself.  This is helpful if you only
+// need weak pointers within the implementation of a class.  This class is also
+// useful when working with primitive types.  For example, you could have a
+// WeakPtrFactory<bool> that is used to pass around a weak reference to a bool.
 template <class T>
-class WeakPtrFactory : public WeakPtrFactoryBase {
+class WeakPtrFactory : public internal::WeakPtrFactoryBase {
  public:
-  explicit WeakPtrFactory(T* ptr) : WeakPtrFactoryBase(reinterpret_cast<uintptr_t>(ptr)) { }
+  explicit WeakPtrFactory(T* ptr)
+      : WeakPtrFactoryBase(reinterpret_cast<uintptr_t>(ptr)) {}
 
   ~WeakPtrFactory() {}
 
   WeakPtr<T> GetWeakPtr() {
     DCHECK(ptr_);
-    return WeakPtr<T>(weak_reference_owner_.GetRef(), reinterpret_cast<T*>(ptr_));
+    return WeakPtr<T>(weak_reference_owner_.GetRef(),
+                      reinterpret_cast<T*>(ptr_));
   }
 
   // Call this method to invalidate all existing weak pointers.