Oilpan: Replace checkPointer() with ASSERT(checkPointer())

Source/platform/heap/Handle.h

 /*
  * Copyright (C) 2014 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 141 matching lines @@
     }
 
     Persistent(std::nullptr_t) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(nullptr)
     {
         initialize();
     }
 
     Persistent(T* raw) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(raw)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     Persistent(T& raw) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(&raw)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     Persistent(const Persistent& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     template<typename U>
     Persistent(const Persistent<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     template<typename U>
     Persistent(const Member<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     template<typename U>
     Persistent(const RawPtr<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other.get())
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     void clear() { m_raw = nullptr; }
 
     ~Persistent()
     {
         uninitialize();
         m_raw = nullptr;
         m_trace = nullptr;
@@ skipping 20 matching lines @@
 
     operator T*() const { return m_raw; }
     operator RawPtr<T>() const { return m_raw; }
 
     T* operator->() const { return *this; }
 
     template<typename U>
     Persistent& operator=(U* other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     Persistent& operator=(std::nullptr_t)
     {
         m_raw = nullptr;
         return *this;
     }
 
     Persistent& operator=(const Persistent& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     template<typename U>
     Persistent& operator=(const Persistent<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     template<typename U>
     Persistent& operator=(const Member<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     template<typename U>
     Persistent& operator=(const RawPtr<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     T* get() const { return m_raw; }
 
 private:
     void initialize()
     {
         ThreadState* state = ThreadStateFor<ThreadingTrait<T>::Affinity>::state();
         state->checkThread();
         m_prev = state->roots();
         m_next = m_prev->m_next;
         m_prev->m_next = this;
         m_next->m_prev = this;
     }
 
     NO_LAZY_SWEEP_SANITIZE_ADDRESS
     void uninitialize()
     {
         ASSERT(isHeapObjectAlive());
         ASSERT(m_next->isHeapObjectAlive());
         ASSERT(m_prev->isHeapObjectAlive());
         m_next->m_prev = m_prev;
         m_prev->m_next = m_next;
     }
 
-    void checkPointer()
+#if ENABLE(ASSERT)
+    bool checkPointer()
     {
-#if ENABLE(ASSERT)
         if (!m_raw)
-            return;
+            return true;
 
         // Heap::isHeapObjectAlive(m_raw) checks that m_raw is a traceable
         // object. In other words, it checks that the pointer is either of:
         //
         //   (a) a pointer to the head of an on-heap object.
         //   (b) a pointer to the head of an on-heap mixin object.
         //
         // Otherwise, Heap::isHeapObjectAlive will crash when it calls
         // header->checkHeader().
         Heap::isHeapObjectAlive(m_raw);
+        // We're not interested in the result of Heap::isHeapObjectAlive.
+        return true;
+    }

[Yuta Kitamura, 2015/06/30 06:15:21]

IMO this use of ASSERT() is too confusing. When the checkPointer() fails,
the assertion failure occurs within two layers of ASSERT()s (one for
ASSERT(checkPointer()), another for the one within
Heap::isHeapObjectAlive()), which isn't a situation a normal developer
would expect.

I really would like to see some evidence (like disassembled code)
that the original code incurs some overhead after optimization.
I believe all modern compilers can optimize out functions like this.

If the overhead is inevitable, then why don't you use inline or
ALWAYS_INLINE, i.e.

    #if ENABLE(ASSERT)
        bool checkPointer() { ... }
    #else
        inline /* or ALWAYS_INLINE */ bool checkPointer() { }
    #endif

 #endif
-    }
 
 #if ENABLE(GC_PROFILING)
     void recordBacktrace()
     {
         if (m_raw)
             m_tracingName = Heap::createBacktraceString();
     }
 
     String m_tracingName;
 #else
@@ skipping 13 matching lines @@
     }
 
     CrossThreadPersistent(std::nullptr_t) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(nullptr)
     {
         initialize();
     }
 
     CrossThreadPersistent(T* raw) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(raw)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     CrossThreadPersistent(T& raw) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(&raw)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     CrossThreadPersistent(const CrossThreadPersistent& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     template<typename U>
     CrossThreadPersistent(const CrossThreadPersistent<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     template<typename U>
     CrossThreadPersistent(const Member<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     template<typename U>
     CrossThreadPersistent(const RawPtr<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other.get())
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
     void clear() { m_raw = nullptr; }
 
     ~CrossThreadPersistent()
     {
         uninitialize();
         m_raw = nullptr;
         m_trace = nullptr;
@@ skipping 20 matching lines @@
 
     operator T*() const { return m_raw; }
     operator RawPtr<T>() const { return m_raw; }
 
     T* operator->() const { return *this; }
 
     template<typename U>
     CrossThreadPersistent& operator=(U* other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     CrossThreadPersistent& operator=(std::nullptr_t)
     {
         m_raw = nullptr;
         return *this;
     }
 
     CrossThreadPersistent& operator=(const CrossThreadPersistent& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     template<typename U>
     CrossThreadPersistent& operator=(const CrossThreadPersistent<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     template<typename U>
     CrossThreadPersistent& operator=(const Member<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     template<typename U>
     CrossThreadPersistent& operator=(const RawPtr<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
 
     T* get() const { return m_raw; }
 
 private:
     void initialize()
     {
         MutexLocker m_locker(ThreadState::globalRootsMutex());
         m_prev = &ThreadState::globalRoots();
         m_next = m_prev->m_next;
         m_prev->m_next = this;
         m_next->m_prev = this;
     }
 
     NO_LAZY_SWEEP_SANITIZE_ADDRESS
     void uninitialize()
     {
         MutexLocker m_locker(ThreadState::globalRootsMutex());
         ASSERT(isHeapObjectAlive());
         ASSERT(m_next->isHeapObjectAlive());
         ASSERT(m_prev->isHeapObjectAlive());
         m_next->m_prev = m_prev;
         m_prev->m_next = m_next;
     }
 
-    void checkPointer()
+#if ENABLE(ASSERT)
+    bool checkPointer()
     {
-#if ENABLE(ASSERT)
         if (!m_raw)
-            return;
+            return true;
         // Heap::isHeapObjectAlive(m_raw) checks that m_raw is a traceable
         // object. In other words, it checks that the pointer is either of:
         //
         //   (a) a pointer to the head of an on-heap object.
         //   (b) a pointer to the head of an on-heap mixin object.
         //
         // Otherwise, Heap::isHeapObjectAlive will crash when it calls
         // header->checkHeader().
         Heap::isHeapObjectAlive(m_raw);
+        // We're not interested in the result of isHeapObjectAlive.
+        return true;
+    }
 #endif
-    }
 
 #if ENABLE(GC_PROFILING)
     void recordBacktrace()
     {
         if (m_raw)
             m_tracingName = Heap::createBacktraceString();
     }
 
     String m_tracingName;
 #else
@@ skipping 126 matching lines @@
     Member() : m_raw(nullptr)
     {
     }
 
     Member(std::nullptr_t) : m_raw(nullptr)
     {
     }
 
     Member(T* raw) : m_raw(raw)
     {
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     explicit Member(T& raw) : m_raw(&raw)
     {
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     template<typename U>
     Member(const RawPtr<U>& other) : m_raw(other.get())
     {
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     Member(WTF::HashTableDeletedValueType) : m_raw(reinterpret_cast<T*>(-1))
     {
     }
 
     bool isHashTableDeletedValue() const { return m_raw == reinterpret_cast<T*>(-1); }
 
     template<typename U>
     Member(const Persistent<U>& other) : m_raw(other)
     {
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     Member(const Member& other) : m_raw(other)
     {
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     template<typename U>
     Member(const Member<U>& other) : m_raw(other)
     {
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     T* release()
     {
         T* result = m_raw;
         m_raw = nullptr;
         return result;
     }
 
     bool operator!() const { return !m_raw; }
 
     operator T*() const { return m_raw; }
 
     T* operator->() const { return m_raw; }
     T& operator*() const { return *m_raw; }
     template<typename U>
     operator RawPtr<U>() const { return m_raw; }
 
     template<typename U>
     Member& operator=(const Persistent<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
     template<typename U>
     Member& operator=(const Member<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
     template<typename U>
     Member& operator=(U* other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
     template<typename U>
     Member& operator=(RawPtr<U> other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
     Member& operator=(std::nullptr_t)
     {
         m_raw = nullptr;
         return *this;
     }
 
     void swap(Member<T>& other)
     {
         std::swap(m_raw, other.m_raw);
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     T* get() const { return m_raw; }
 
     void clear() { m_raw = nullptr; }
 
 
 protected:
-    void checkPointer()
+#if ENABLE(ASSERT)
+    bool checkPointer()
     {
-#if ENABLE(ASSERT)
         if (!m_raw)
-            return;
+            return true;
         // HashTable can store a special value (which is not aligned to the
         // allocation granularity) to Member<> to represent a deleted entry.
         // Thus we treat a pointer that is not aligned to the granularity
         // as a valid pointer.
         if (reinterpret_cast<intptr_t>(m_raw) % allocationGranularity)
-            return;
+            return true;
 
         // TODO(haraken): What we really want to check here is that the pointer
         // is a traceable object. In other words, the pointer is either of:
         //
         //   (a) a pointer to the head of an on-heap object.
         //   (b) a pointer to the head of an on-heap mixin object.
         //
         // We can check it by calling Heap::isHeapObjectAlive(m_raw),
         // but we cannot call it here because it requres to include T.h.
         // So we currently implement only the check for (a).
         if (!IsGarbageCollectedMixin<T>::value)
             HeapObjectHeader::fromPayload(m_raw)->checkHeader();
+        return true;
+    }
 #endif
-    }
 
     T* m_raw;
 
     template<bool x, WTF::WeakHandlingFlag y, WTF::ShouldWeakPointersBeMarkedStrongly z, typename U, typename V> friend struct CollectionBackingTraceTrait;
     friend class Visitor;
 
 };
 
 // WeakMember is similar to Member in that it is used to point to other oilpan
 // heap allocated objects.
@@ skipping 15 matching lines @@
     template<typename U>
     WeakMember(const Persistent<U>& other) : Member<T>(other) { }
 
     template<typename U>
     WeakMember(const Member<U>& other) : Member<T>(other) { }
 
     template<typename U>
     WeakMember& operator=(const Persistent<U>& other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
     template<typename U>
     WeakMember& operator=(const Member<U>& other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
     template<typename U>
     WeakMember& operator=(U* other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
     template<typename U>
     WeakMember& operator=(const RawPtr<U>& other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
     WeakMember& operator=(std::nullptr_t)
     {
         this->m_raw = nullptr;
         return *this;
     }
 
 private:
@@ skipping 419 matching lines @@
 struct ParamStorageTraits<RawPtr<T>> : public PointerParamStorageTraits<T*, blink::IsGarbageCollectedType<T>::value> {
     static_assert(sizeof(T), "T must be fully defined");
 };
 
 template<typename T>
 PassRefPtr<T> adoptRef(blink::RefCountedGarbageCollected<T>*) = delete;
 
 } // namespace WTF
 
 #endif