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

Source/platform/heap/Handle.h

Index: Source/platform/heap/Handle.h
diff --git a/Source/platform/heap/Handle.h b/Source/platform/heap/Handle.h
index 4fdc0a33c15aa274b4620aa9f0d21908943f0e6c..8ebeb07fe5b90ec6bd759eb2a77261f4d9b590e7 100644
--- a/Source/platform/heap/Handle.h
+++ b/Source/platform/heap/Handle.h
@@ -159,21 +159,21 @@ public:
     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();
     }
 
@@ -181,7 +181,7 @@ public:
     Persistent(const Persistent<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
@@ -189,7 +189,7 @@ public:
     Persistent(const Member<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
@@ -197,7 +197,7 @@ public:
     Persistent(const RawPtr<U>& other) : PersistentNode(TraceMethodDelegate<Persistent<T>, &Persistent<T>::trace>::trampoline), m_raw(other.get())
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
@@ -238,7 +238,7 @@ public:
     Persistent& operator=(U* other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -252,7 +252,7 @@ public:
     Persistent& operator=(const Persistent& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -261,7 +261,7 @@ public:
     Persistent& operator=(const Persistent<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -270,7 +270,7 @@ public:
     Persistent& operator=(const Member<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -279,7 +279,7 @@ public:
     Persistent& operator=(const RawPtr<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -307,11 +307,11 @@ private:
         m_prev->m_next = m_next;
     }
 
-    void checkPointer()
-    {
 #if ENABLE(ASSERT)
+    bool checkPointer()
+    {
         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:
@@ -322,8 +322,10 @@ private:
         // Otherwise, Heap::isHeapObjectAlive will crash when it calls
         // header->checkHeader().
         Heap::isHeapObjectAlive(m_raw);
-#endif
+        // 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()
@@ -357,21 +359,21 @@ public:
     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();
     }
 
@@ -379,7 +381,7 @@ public:
     CrossThreadPersistent(const CrossThreadPersistent<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
@@ -387,7 +389,7 @@ public:
     CrossThreadPersistent(const Member<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other)
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
@@ -395,7 +397,7 @@ public:
     CrossThreadPersistent(const RawPtr<U>& other) : PersistentNode(TraceMethodDelegate<CrossThreadPersistent<T>, &CrossThreadPersistent<T>::trace>::trampoline), m_raw(other.get())
     {
         initialize();
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
     }
 
@@ -436,7 +438,7 @@ public:
     CrossThreadPersistent& operator=(U* other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -450,7 +452,7 @@ public:
     CrossThreadPersistent& operator=(const CrossThreadPersistent& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -459,7 +461,7 @@ public:
     CrossThreadPersistent& operator=(const CrossThreadPersistent<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -468,7 +470,7 @@ public:
     CrossThreadPersistent& operator=(const Member<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -477,7 +479,7 @@ public:
     CrossThreadPersistent& operator=(const RawPtr<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         recordBacktrace();
         return *this;
     }
@@ -505,11 +507,11 @@ private:
         m_prev->m_next = m_next;
     }
 
-    void checkPointer()
-    {
 #if ENABLE(ASSERT)
+    bool checkPointer()
+    {
         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:
         //
@@ -519,8 +521,10 @@ private:
         // Otherwise, Heap::isHeapObjectAlive will crash when it calls
         // header->checkHeader().
         Heap::isHeapObjectAlive(m_raw);
-#endif
+        // We're not interested in the result of isHeapObjectAlive.
+        return true;
     }
+#endif
 
 #if ENABLE(GC_PROFILING)
     void recordBacktrace()
@@ -667,18 +671,18 @@ public:
 
     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))
@@ -690,18 +694,18 @@ public:
     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()
@@ -724,7 +728,7 @@ public:
     Member& operator=(const Persistent<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
@@ -732,7 +736,7 @@ public:
     Member& operator=(const Member<U>& other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
@@ -740,7 +744,7 @@ public:
     Member& operator=(U* other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
@@ -748,7 +752,7 @@ public:
     Member& operator=(RawPtr<U> other)
     {
         m_raw = other;
-        checkPointer();
+        ASSERT(checkPointer());
         return *this;
     }
 
@@ -761,7 +765,7 @@ public:
     void swap(Member<T>& other)
     {
         std::swap(m_raw, other.m_raw);
-        checkPointer();
+        ASSERT(checkPointer());
     }
 
     T* get() const { return m_raw; }
@@ -770,17 +774,17 @@ public:
 
 
 protected:
-    void checkPointer()
-    {
 #if ENABLE(ASSERT)
+    bool checkPointer()
+    {
         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:
@@ -793,8 +797,9 @@ protected:
         // So we currently implement only the check for (a).
         if (!IsGarbageCollectedMixin<T>::value)
             HeapObjectHeader::fromPayload(m_raw)->checkHeader();
-#endif
+        return true;
     }
+#endif
 
     T* m_raw;
 
@@ -830,7 +835,7 @@ public:
     WeakMember& operator=(const Persistent<U>& other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
@@ -838,7 +843,7 @@ public:
     WeakMember& operator=(const Member<U>& other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
@@ -846,7 +851,7 @@ public:
     WeakMember& operator=(U* other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }
 
@@ -854,7 +859,7 @@ public:
     WeakMember& operator=(const RawPtr<U>& other)
     {
         this->m_raw = other;
-        this->checkPointer();
+        ASSERT(this->checkPointer());
         return *this;
     }