Make base::WeakPtr::Get() fast

base/memory/weak_ptr.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Weak pointers are pointers to an object that do not affect its lifetime,
 // and which may be invalidated (i.e. reset to nullptr) by the object, or its
 // owner, at any time, most commonly when the object is about to be deleted.
 
 // Weak pointers are useful when an object needs to be accessed safely by one
 // or more objects other than its owner, and those callers can cope with the
@@ skipping 77 matching lines @@
 // These classes are part of the WeakPtr implementation.
 // DO NOT USE THESE CLASSES DIRECTLY YOURSELF.
 
 class BASE_EXPORT WeakReference {
  public:
   // 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();
+    static Flag* nullFlag();

[Nico, 2017/06/28 14:54:10]

google (and chromium) style spells functions NullFlag() (or, optionally,
null_flag() if it's inline and cheap)

Give this function a comment. (Is it valid to call methods on the flag; what's
the NullFlag() good for, can I addref() it, does its refcount ever become small
enough for this presumably singleton object to be deleted, etc)

[hans, 2017/06/28 20:37:35]

Done.

 
-    void Invalidate();
-    bool IsValid() const;
+    void Invalidate() {
+#if DCHECK_IS_ON()
+      if (this == nullFlag())
+        return;

[Nico, 2017/06/28 14:54:09]

This looks kind of confusing – do you not want to set is_valid_ to 0 if this ==
nullFlag(), but only in dcheck builds? Probably fine either way since it's 0
already there so, setting it might be cheaper than the check. I think this is
correct, but it's not obvious with a casual glance. Maybe

#if DCHECK_IS_ON()
  if (this != NullFlag()) {
    // The flag being...
    DCHECK...
  }
#endif
  is_valid = 0

?

(Also, moving the functions from cc to h could be its own CL too...)

[hans, 2017/06/28 20:37:35]

I've tried to make this clearer with a comment. The point of the code is to skip
the sequence check for the Null Flag. I've tried to make this clearer.

+      // 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())
+          << "WeakPtrs must be invalidated on the same sequenced thread.";
+#endif
+      is_valid_ = 0;
+    }
+    uintptr_t IsValid() const {

[Nico, 2017/06/28 14:54:10]

Add comment that this returns either all 0s or all 1s and can be used as a
bitmask.

[hans, 2017/06/28 20:37:35]

Done.

+#if DCHECK_IS_ON()
+      if (this == nullFlag()) {
+        DCHECK(!is_valid_);
+        return 0;
+      }
+      DCHECK(sequence_checker_.CalledOnValidSequence())
+          << "WeakPtrs must be checked on the same sequenced thread.";
+#endif
+      return is_valid_;
+    }
 
    private:
     friend class base::RefCountedThreadSafe<Flag>;
 
+    enum NullFlagTag { kNullFlagTag };
+    Flag(NullFlagTag);
+
     ~Flag();
 
+    uintptr_t is_valid_;
+#if DCHECK_IS_ON()
+    // Even if SequenceChecker is an empty class in non-dcheck builds, it still
+    // takes up space in the class.

[Nico, 2017/06/28 14:36:56]

This bit seems unrelated to what's in the CL description, and is a space
optimization, not a time optimization. Can this be done in a separated CL, with
an estimate how much space it saves?

(Slightly related thread "[chromium-dev] PSA : Sequencification and PostTask
paradigm update", and fairly related thread "ThreadChecker vs. NonThreadSafe"
(also on chromium-dev))

[hans, 2017/06/28 20:37:35]

It's related in that it keeps the size from growing with my change.

On trunk, Flag has an 8-byte ref count + 1-byte bool + 1-byte empty Sequence
Checker. With 8-byte alignment that means its sizeof is 16.

With my change, we get 8-byte ref count + 8-byte uintptr_t + 1-byte empty
Sequence Checker. With 8-byte alignment, the sizeof would become 24.

So I'm removing the Sequence Checker in release builds to maintain Flag's size
at 16 bytes.
I saw them, but is there any action I need to take in this CL?

[Nico, 2017/06/28 20:57:03]

Ah ok, sounds good then

     SequenceChecker sequence_checker_;
-    bool is_valid_;
+#endif
   };
 
   WeakReference();
   explicit WeakReference(const Flag* flag);
   ~WeakReference();
 
   WeakReference(WeakReference&& other);
   WeakReference(const WeakReference& other);
   WeakReference& operator=(WeakReference&& other) = default;
   WeakReference& operator=(const WeakReference& other) = default;
 
-  bool is_valid() const;
+  uintptr_t is_valid() const { return flag_->IsValid(); }
 
  private:
+  // 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 {
  public:
   WeakReferenceOwner();
   ~WeakReferenceOwner();
 
   WeakReference GetRef() const;
 
-  bool HasRefs() const {
-    return flag_.get() && !flag_->HasOneRef();
-  }
+  bool HasRefs() const { return flag_->IsValid() && !flag_->HasOneRef(); }
 
   void Invalidate();
 
  private:
   mutable scoped_refptr<WeakReference::Flag> flag_;
 };
 
 // This class simplifies the implementation of WeakPtr's type conversion
 // constructor by avoiding the need for a public accessor for ref_.  A
 // WeakPtr<T> cannot access the private members of WeakPtr<U>, so this
 // base class gives us a way to access ref_ in a protected fashion.
 class BASE_EXPORT WeakPtrBase {
  public:
   WeakPtrBase();
   ~WeakPtrBase();
 
   WeakPtrBase(const WeakPtrBase& other) = default;
   WeakPtrBase(WeakPtrBase&& other) = default;
   WeakPtrBase& operator=(const WeakPtrBase& other) = default;
   WeakPtrBase& operator=(WeakPtrBase&& other) = default;
 
  protected:
   explicit WeakPtrBase(const WeakReference& ref);
 
   WeakReference ref_;
+
+  // This pointer is only valid when ref_.is_valid() is true.  Otherwise, its
+  // value is undefined (as opposed to nullptr).
+  uintptr_t ptr_;

[Nico, 2017/06/28 14:54:10]

Moving this to the base class also feels kind of unrelated to the main thrust
(but it's mentioned in the CL description)

[hans, 2017/06/28 20:37:35]

Done. Splitting this out.

 };
 
 // This class provides a common implementation of common functions that would
 // otherwise get instantiated separately for each distinct instantiation of
 // SupportsWeakPtr<>.
 class SupportsWeakPtrBase {
  public:
   // A safe static downcast of a WeakPtr<Base> to WeakPtr<Derived>. This
   // conversion will only compile if there is exists a Base which inherits
   // from SupportsWeakPtr<Base>. See base::AsWeakPtr() below for a helper
   // function that makes calling this easier.
   template<typename Derived>
   static WeakPtr<Derived> StaticAsWeakPtr(Derived* t) {
     static_assert(
         std::is_base_of<internal::SupportsWeakPtrBase, Derived>::value,
         "AsWeakPtr argument must inherit from SupportsWeakPtr");
     return AsWeakPtrImpl<Derived>(t, *t);
   }
 
  private:
   // This template function uses type inference to find a Base of Derived
   // which is an instance of SupportsWeakPtr<Base>. We can then safely
   // static_cast the Base* to a Derived*.
   template <typename Derived, typename Base>
   static WeakPtr<Derived> AsWeakPtrImpl(
       Derived* t, const SupportsWeakPtr<Base>&) {
     WeakPtr<Base> ptr = t->Base::AsWeakPtr();
-    return WeakPtr<Derived>(ptr.ref_, static_cast<Derived*>(ptr.ptr_));
+    return WeakPtr<Derived>(
+        ptr.ref_, static_cast<Derived*>(reinterpret_cast<Base*>(ptr.ptr_)));
   }
 };
 
 }  // namespace internal
 
 template <typename T> class WeakPtrFactory;
 
 // The WeakPtr class holds a weak reference to |T*|.
 //
 // This class is designed to be used like a normal pointer.  You should always
 // null-test an object of this class before using it or invoking a method that
 // may result in the underlying object being destroyed.
 //
 // EXAMPLE:
 //
 //   class Foo { ... };
 //   WeakPtr<Foo> foo;
 //   if (foo)
 //     foo->method();
 //
 template <typename T>
 class WeakPtr : public internal::WeakPtrBase {
  public:
-  WeakPtr() : ptr_(nullptr) {}
+  WeakPtr() {}
 
-  WeakPtr(std::nullptr_t) : ptr_(nullptr) {}
+  WeakPtr(std::nullptr_t) {}
 
   // Allow conversion from U to T provided U "is a" T. Note that this
   // is separate from the (implicit) copy and move constructors.
   template <typename U>
-  WeakPtr(const WeakPtr<U>& other) : WeakPtrBase(other), ptr_(other.ptr_) {
+  WeakPtr(const WeakPtr<U>& other) : WeakPtrBase(other) {
+    static_assert(std::is_base_of<T, U>::value,
+                  "other must be a subclass of T");
   }
   template <typename U>
-  WeakPtr(WeakPtr<U>&& other)
-      : WeakPtrBase(std::move(other)), ptr_(other.ptr_) {}
+  WeakPtr(WeakPtr<U>&& other) : WeakPtrBase(std::move(other)) {
+    static_assert(std::is_base_of<T, U>::value,
+                  "other must be a subclass of T");
+  }
 
-  T* get() const { return ref_.is_valid() ? ptr_ : nullptr; }
+  T* get() const { return reinterpret_cast<T*>(ref_.is_valid() & ptr_); }

[Nico, 2017/06/28 14:54:09]

Since this is so unusual, I'd probably add `// Intentionally bitwise and, see
comment on Ref::is_valid()

[hans, 2017/06/28 20:37:35]

Done.

 
   T& operator*() const {
     DCHECK(get() != nullptr);
     return *get();
   }
   T* operator->() const {
     DCHECK(get() != nullptr);
     return get();
   }
 
   void reset() {
     ref_ = internal::WeakReference();
-    ptr_ = nullptr;
+    ptr_ = 0;
   }
 
   // Allow conditionals to test validity, e.g. if (weak_ptr) {...};
   explicit operator bool() const { return get() != nullptr; }
 
  private:
   friend class internal::SupportsWeakPtrBase;
   template <typename U> friend class WeakPtr;
   friend class SupportsWeakPtr<T>;
   friend class WeakPtrFactory<T>;
 
-  WeakPtr(const internal::WeakReference& ref, T* ptr)
-      : WeakPtrBase(ref),
-        ptr_(ptr) {
+  WeakPtr(const internal::WeakReference& ref, T* ptr) : WeakPtrBase(ref) {
+    ptr_ = reinterpret_cast<uintptr_t>(ptr);
   }
-
-  // This pointer is only valid when ref_.is_valid() is true.  Otherwise, its
-  // value is undefined (as opposed to nullptr).
-  T* ptr_;
 };
 
 // Allow callers to compare WeakPtrs against nullptr to test validity.
 template <class T>
 bool operator!=(const WeakPtr<T>& weak_ptr, std::nullptr_t) {
   return !(weak_ptr == nullptr);
 }
 template <class T>
 bool operator!=(std::nullptr_t, const WeakPtr<T>& weak_ptr) {
   return weak_ptr != nullptr;
 }
 template <class T>
 bool operator==(const WeakPtr<T>& weak_ptr, std::nullptr_t) {
   return weak_ptr.get() == nullptr;
 }
 template <class T>
 bool operator==(std::nullptr_t, const WeakPtr<T>& weak_ptr) {
   return weak_ptr == nullptr;
 }
 
+namespace internal {
+class BASE_EXPORT WeakPtrFactoryBase {
+ protected:
+  WeakPtrFactoryBase(uintptr_t ptr);
+  ~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 {
+class WeakPtrFactory : public internal::WeakPtrFactoryBase {
  public:
-  explicit WeakPtrFactory(T* ptr) : ptr_(ptr) {
-  }
+  explicit WeakPtrFactory(T* ptr)
+      : WeakPtrFactoryBase(reinterpret_cast<uintptr_t>(ptr)) {}
 
-  ~WeakPtrFactory() { ptr_ = nullptr; }
+  ~WeakPtrFactory() {}
 
   WeakPtr<T> GetWeakPtr() {
     DCHECK(ptr_);
-    return WeakPtr<T>(weak_reference_owner_.GetRef(), ptr_);
+    return WeakPtr<T>(weak_reference_owner_.GetRef(),
+                      reinterpret_cast<T*>(ptr_));
   }
 
   // Call this method to invalidate all existing weak pointers.
   void InvalidateWeakPtrs() {
     DCHECK(ptr_);
     weak_reference_owner_.Invalidate();
   }
 
   // Call this method to determine if any weak pointers exist.
   bool HasWeakPtrs() const {
     DCHECK(ptr_);
     return weak_reference_owner_.HasRefs();
   }
 
  private:
-  internal::WeakReferenceOwner weak_reference_owner_;
-  T* ptr_;
   DISALLOW_IMPLICIT_CONSTRUCTORS(WeakPtrFactory);
 };
 
 // A class may extend from SupportsWeakPtr to let others take weak pointers to
 // it. This avoids the class itself implementing boilerplate to dispense weak
 // pointers.  However, since SupportsWeakPtr's destructor won't invalidate
 // weak pointers to the class until after the derived class' members have been
 // destroyed, its use can lead to subtle use-after-destroy issues.
 template <class T>
 class SupportsWeakPtr : public internal::SupportsWeakPtrBase {
@@ skipping 31 matching lines @@
 //   base::WeakPtr<Derived> ptr = derived.AsWeakPtr();  // Fails.
 
 template <typename Derived>
 WeakPtr<Derived> AsWeakPtr(Derived* t) {
   return internal::SupportsWeakPtrBase::StaticAsWeakPtr<Derived>(t);
 }
 
 }  // namespace base
 
 #endif  // BASE_MEMORY_WEAK_PTR_H_