Revert "Revert of [oilpan]: optimize the way we allocate persistent handles in wrappers. (patchset …

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 132 matching lines @@
 
 private:
     PersistentNode* m_next;
     PersistentNode* m_prev;
 
     template<typename RootsAccessor, typename Owner> friend class PersistentBase;
     friend class PersistentAnchor;
     friend class ThreadState;
 };
 
+
+const int wrapperPersistentsPerRegion = 256;
+const size_t wrapperPersistentOffsetMask = ~static_cast<size_t>(3);
+const size_t wrapperPersistentLiveBitMask = 1;
+
+class WrapperPersistentNode {
+    ALLOW_ONLY_INLINE_ALLOCATION()

[Mads Ager (chromium), 2014/09/09 11:29:23]

Nit: Can we put a ';' at the end here. Makes it look a little more uniform and
some editors prefer to have it. :)

[wibling-chromium, 2014/09/09 11:32:48]

Disallow dynamic allocation. We only allow placement new.

[wibling-chromium, 2014/09/09 11:49:50]

Done.

+    WTF_MAKE_NONCOPYABLE(WrapperPersistentNode);
+public:
+    bool isAlive() { return m_regionOffset & wrapperPersistentLiveBitMask; }

[zerny-chromium, 2014/09/09 11:33:11]

Nit: I'd add the corresponding accessor for:

  size_t offset() { ... }

and use that below.

[wibling-chromium, 2014/09/09 11:49:50]

Done.

+
+    WrapperPersistentRegion* region()
+    {
+        return reinterpret_cast<WrapperPersistentRegion*>(
+            reinterpret_cast<Address>(this) - (m_regionOffset & wrapperPersistentOffsetMask));
+    }
+
+    virtual void trace(Visitor* visitor) { }
+
+    static inline void destroy(const WrapperPersistentNode*);

[wibling-chromium, 2014/09/09 11:32:47]

Use destroy(...) instead of calling delete on the WrapperPersistents.

+
+protected:
+    WrapperPersistentNode() : m_raw(0), m_regionOffset(0) { }
+    WrapperPersistentNode(void *raw, size_t regionOffset) : m_raw(raw), m_regionOffset(regionOffset) { }

[wibling-chromium, 2014/09/09 11:32:47]

New constructor taking the regionOffset to ensure we don't rely on it being
unchanged.

+
+private:
+    WrapperPersistentNode* takeSlot()
+    {
+        // The slot should not be alive at the point where it is allocated.
+        ASSERT(!isAlive());
+        WrapperPersistentNode* nextFree = reinterpret_cast<WrapperPersistentNode*>(m_raw);
+        m_raw = 0;
+        return nextFree;
+    }
+
+    WrapperPersistentNode* freeSlot(WrapperPersistentNode* nextFree)
+    {
+        m_regionOffset &= ~wrapperPersistentLiveBitMask;
+        m_raw = nextFree;
+        return this;
+    }
+
+    // Don't allow delete being called on wrapper persistent nodes. We
+    // do use placement new to initialize the slot with the right vtable. See
+    // WrapperPersistent<T> below.
+    void operator delete(void*);

[wibling-chromium, 2014/09/09 11:32:47]

Ensure no one calls delete.

+
+protected:
+    // m_raw is used both to point to the object when the WrapperPersistentNode is used/alive
+    // and to point to the next free wrapperPersistentNode in the region when the node is
+    // unused/dead.
+    void* m_raw;
+
+    // The m_regionOffset field is an offset from this node to the base of the containing
+    // WrapperPersistentRegion.

[zerny-chromium, 2014/09/09 11:33:11]

The m_regionOffset field encodes the liveness of the node and the offset ...

[wibling-chromium, 2014/09/09 11:49:50]

Done.

+    size_t m_regionOffset;
+
+    friend class WrapperPersistentRegion;
+};
+
+template<typename T>
+class WrapperPersistent FINAL : public WrapperPersistentNode {
+    ALLOW_ONLY_INLINE_ALLOCATION()

[wibling-chromium, 2014/09/09 11:32:47]

Only allow placement new.

+public:
+    static WrapperPersistent<T>* create(T* raw);

[wibling-chromium, 2014/09/09 11:32:48]

Allocate WrapperPersistent<T> via the create method not via new.

+
+    virtual void trace(Visitor* visitor)

[zerny-chromium, 2014/09/09 11:33:11]

Nit: OVERRIDE

[wibling-chromium, 2014/09/09 11:49:50]

Done.

+    {
+        ASSERT(isAlive());
+        visitor->mark(static_cast<T*>(m_raw));
+    }
+
+private:
+    WrapperPersistent() { }
+
+    // We need to use a constructor to initialize the allocated slot since it
+    // has a vtable which must be set to the WrapperPersistent<T> type.
+    WrapperPersistent(T* raw, size_t regionOffset) : WrapperPersistentNode(raw, regionOffset) { }

[wibling-chromium, 2014/09/09 11:32:48]

New constructor taking both the raw pointer and the region offset to ensure we
don't rely on the state being unmodified by the allocator.

+
+    // Don't allow delete being called on wrapper persistents.
+    void operator delete(void*);

[wibling-chromium, 2014/09/09 11:32:48]

Disallow delete.

+};
+
+class PLATFORM_EXPORT WrapperPersistentRegion {
+    WTF_MAKE_NONCOPYABLE(WrapperPersistentRegion);
+public:
+    WrapperPersistentRegion()
+    {
+        WrapperPersistentNode* nextFree = 0;
+        for (int i = wrapperPersistentsPerRegion - 1; i >= 0; --i) {
+            size_t regionOffset = reinterpret_cast<Address>(&m_entries[i]) - reinterpret_cast<Address>(this);
+            // Setup the free slot with an offset to the containing region's base and a pointer to the next
+            // free slot in the region.
+            ASSERT(!(regionOffset & ~wrapperPersistentOffsetMask));
+            new (&m_entries[i]) WrapperPersistentNode(nextFree, regionOffset);
+            nextFree = &m_entries[i];
+        }
+        m_prev = 0;
+        m_next = 0;
+        m_freeHead = nextFree;
+        m_count = 0;
+    }
+
+    Address allocate()
+    {
+        if (!m_freeHead) {
+            ASSERT(m_count == wrapperPersistentsPerRegion);
+            return 0;
+        }
+        // We have a free persistent slot in this region.
+        WrapperPersistentNode* freeSlot = m_freeHead;
+        // Take the slot and advance m_freeHead to the next free slot.
+        m_freeHead = freeSlot->takeSlot();
+        ASSERT(m_count < wrapperPersistentsPerRegion);
+        m_count++;
+        return reinterpret_cast<Address>(freeSlot);
+    }
+
+    void free(WrapperPersistentNode* object)
+    {
+        ASSERT(object);
+        m_freeHead = object->freeSlot(m_freeHead);
+        ASSERT(m_count > 0);
+        m_count--;
+        if (!m_count)
+            ThreadState::current()->freeWrapperPersistentRegion(this);
+    }
+
+    bool removeIfNotLast(WrapperPersistentRegion** headPtr);
+    static void insertHead(WrapperPersistentRegion** headPtr, WrapperPersistentRegion* newHead);
+    static WrapperPersistentRegion* removeHead(WrapperPersistentRegion** headPtr);
+    static Address outOfLineAllocate(ThreadState*, WrapperPersistentRegion**);
+    static void trace(WrapperPersistentRegion* head, Visitor* visitor)
+    {
+        for (WrapperPersistentRegion* current = head; current; current = current->m_next)
+            current->traceRegion(visitor);
+    }
+
+private:
+    void traceRegion(Visitor* visitor)
+    {
+        size_t live = 0;
+
+#ifdef NDEBUG
+        for (int i = 0; i < wrapperPersistentsPerRegion && live < m_count; ++i) {
+#else
+        // In DEBUG mode we scan all entries to validate we only have m_count
+        // live entries.
+        for (int i = 0; i < wrapperPersistentsPerRegion; ++i) {
+#endif
+            if (m_entries[i].isAlive()) {
+                m_entries[i].trace(visitor);
+                live++;
+            }
+        }
+        ASSERT(live == m_count);
+    }
+
+    WrapperPersistentRegion* m_prev;
+    WrapperPersistentRegion* m_next;
+    WrapperPersistentNode* m_freeHead;
+    size_t m_count;
+    WrapperPersistentNode m_entries[wrapperPersistentsPerRegion];
+};
+
+template<typename T>
+WrapperPersistent<T>* WrapperPersistent<T>::create(T* raw)

[wibling-chromium, 2014/09/09 11:32:48]

New create and destroy...

+{
+    ThreadState* state = ThreadState::current();
+    WrapperPersistentRegion* region = state->wrapperRoots();
+    ASSERT(region);
+    Address persistentSlot = region->allocate();
+    if (!persistentSlot)
+        persistentSlot = WrapperPersistentRegion::outOfLineAllocate(state, &region);
+    ASSERT(persistentSlot);
+    ASSERT(!reinterpret_cast<WrapperPersistentNode*>(persistentSlot)->isAlive());
+
+    size_t regionOffset = persistentSlot - reinterpret_cast<Address>(region);
+    regionOffset |= wrapperPersistentLiveBitMask;

[zerny-chromium, 2014/09/09 11:33:11]

Nit: Would it not be more natural to have this OR in the initialization list of
the constructor of WrapperPersistentNode?

[wibling-chromium, 2014/09/09 11:49:50]

I can't do that as the code is now, since the constructor is also used to
initialize the entries when first allocated where the bit shouldn't be set.

+
+    // We use placement new to call the constructor to ensure that we setup vtable correctly.

[Mads Ager (chromium), 2014/09/09 11:29:23]

vtable -> the vtable

[wibling-chromium, 2014/09/09 11:49:49]

Done.

+    return new (persistentSlot) WrapperPersistent<T>(raw, regionOffset);
+}
+
+void WrapperPersistentNode::destroy(const WrapperPersistentNode* node)
+{
+    WrapperPersistentNode* persistent = const_cast<WrapperPersistentNode*>(node);
+    persistent->region()->free(persistent);
+}
+
 // RootsAccessor for Persistent that provides access to thread-local list
 // of persistent handles. Can only be used to create handles that
 // are constructed and destructed on the same thread.
 template<ThreadAffinity Affinity>
 class ThreadLocalPersistents {
 public:
     static PersistentNode* roots() { return state()->roots(); }
 
     // No locking required. Just check that we are at the right thread.
     class Lock {
@@ skipping 1038 matching lines @@
 struct ParamStorageTraits<T*> : public PointerParamStorageTraits<T*, blink::IsGarbageCollectedType<T>::value> {
 };
 
 template<typename T>
 struct ParamStorageTraits<RawPtr<T> > : public PointerParamStorageTraits<T*, blink::IsGarbageCollectedType<T>::value> {
 };
 
 } // namespace WTF
 
 #endif