Blink-side contiguous allocation of display items.

Source/platform/graphics/paint/DisplayItemList.h

Index: Source/platform/graphics/paint/DisplayItemList.h
diff --git a/Source/platform/graphics/paint/DisplayItemList.h b/Source/platform/graphics/paint/DisplayItemList.h
index 6140393706b7623c5d446e44b6535b85afe53190..4ac87e51d36bcdc6b97f22e428f9963b78f1986b 100644
--- a/Source/platform/graphics/paint/DisplayItemList.h
+++ b/Source/platform/graphics/paint/DisplayItemList.h
@@ -6,16 +6,23 @@
 #define DisplayItemList_h
 
 #include "platform/PlatformExport.h"
+#include "platform/graphics/ListContainer.h"
 #include "platform/graphics/paint/DisplayItem.h"
-#include "platform/graphics/paint/DisplayItems.h"
+#include "platform/graphics/paint/Transform3DDisplayItem.h"
 #include "wtf/HashMap.h"
 #include "wtf/PassOwnPtr.h"
+#include "wtf/Utility.h"
 #include "wtf/Vector.h"
 
 namespace blink {
 
 class GraphicsContext;
 
+using DisplayItems = ListContainer<DisplayItem>;
+
+static const size_t kInitialDisplayItemsCapacity = 64;
+static const size_t kMaximumDisplayItemSize = sizeof(BeginTransform3DDisplayItem);

[jbroman, 2015/06/29 18:11:56]

I think you should be able to move this to a source file, and then make it more
durable, with something like:

static const size_t kMaximumDisplayItemSize = MaxSize<ThisDisplayItem,
ThatDisplayItem, SomeOtherDisplayItem>::value;

with

template <typename... Types> struct MaxSize;
template <> struct MaxSize<> { static const size_t value = 0; };
template <typename T, typename... Rest> struct MaxSize<T, Rest...> {
  static const size_t value = sizeof(T) >= MaxSize<Rest...>::value ? sizeof(T) :
MaxSize<Rest...>::value;
};

On a related note: it hasn't bitten us yet in cc, but ListContainer doesn't take
alignment into account, so I'm concerned we could end up with unaligned data in
the future. In particular, TransformationMatrix wants its data to be aligned on
16-byte boundaries (and if this doesn't happen, we could get runtime
exceptions). Our tests would probably catch this, so I won't block this CL on
figuring this out, but wanted to make sure it's on your radar.

[danakj, 2015/06/29 18:15:29]

FWIW there is a static assert that makes it impossible for this to be wrong for
any DisplayItem type actually used. This seems more complicated? We could static
assert alignment by checking size % 16?

[pdr., 2015/06/29 22:20:03]

This is a lot of complexity without much benefit. If you make a mistake and set
kMaximumDisplayItemSize to sizeof(DisplayItem), you'll get a static assert that
describes how to fix the issue. I think this is more robust than keeping a list
of all subclasses up to date.

[jbroman, 2015/06/30 14:40:50]

You'd still have the other static_assert, but in any case I'm okay with leaving
this as-is (I'm just not thrilled that including DisplayItemList.h transitively
includes Transform3DDisplayItem.h, but I'll get over it).

+
 class PLATFORM_EXPORT DisplayItemList {
     WTF_MAKE_NONCOPYABLE(DisplayItemList);
     WTF_MAKE_FAST_ALLOCATED(DisplayItemList);
@@ -30,7 +37,19 @@ public:
     void invalidateAll();
 
     // These methods are called during painting.
-    void add(WTF::PassOwnPtr<DisplayItem>);
+    template <typename DisplayItemClass, typename... Args>
+    DisplayItemClass& createAndAppend(Args&&... args)
+    {
+        static_assert(WTF::IsSubclass<DisplayItemClass, DisplayItem>::value,
+            "Can only createAndAppend subclasses of DisplayItem.");
+        static_assert(sizeof(DisplayItemClass) <= kMaximumDisplayItemSize,
+            "DisplayItem subclass is larger than kMaximumDisplayItemSize.");
+
+        DisplayItemClass* displayItem = m_newDisplayItems.allocateWithoutConstruction<DisplayItemClass>();

[danakj, 2015/06/29 17:43:51]

why isnt it allocateAndContruct<DisplayItemClass>(WTF::forward<Args>(args)...)?

[jbroman, 2015/06/29 18:11:56]

Agreed; we can do this in one step.

[pdr., 2015/06/29 22:20:03]

Agreed x2. Done.

This also removes the ListContainer::allocateWithoutConstruction cray-cray.

+        new (displayItem) DisplayItemClass(WTF::forward<Args>(args)...);
+        processNewItem(displayItem);
+        return *displayItem;
+    }
     void beginScope(DisplayItemClient);
     void endScope(DisplayItemClient);
 
@@ -72,7 +91,9 @@ public:
 
 protected:
     DisplayItemList()
-        : m_validlyCachedClientsDirty(false)
+        : m_currentDisplayItems(kMaximumDisplayItemSize, 0)
+        , m_newDisplayItems(kMaximumDisplayItemSize, kInitialDisplayItemsCapacity)
+        , m_validlyCachedClientsDirty(false)
         , m_constructionDisabled(false)
         , m_skippingCacheCount(0)
         , m_numCachedItems(0) { }
@@ -82,6 +103,10 @@ private:
     friend class DisplayItemListPaintTest;
     friend class LayoutObjectDrawingRecorderTest;
 
+    // Set new item state (scopes, cache skipping, etc) for a new item.
+    // TODO(pdr): This only passes a pointer to make the patch easier to review. Change to a reference.
+    void processNewItem(DisplayItem*);
+
     void updateValidlyCachedClientsIfNeeded() const;
 
 #ifndef NDEBUG
@@ -100,11 +125,11 @@ private:
 #if ENABLE(ASSERT)
     // The following two methods are for checking under-invalidations
     // (when RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled).
-    void checkCachedDisplayItemIsUnchanged(const DisplayItems::ItemHandle&, DisplayItemIndicesByClientMap&);
+    void checkCachedDisplayItemIsUnchanged(const DisplayItem&, DisplayItemIndicesByClientMap&);
     void checkNoRemainingCachedDisplayItems();
 #endif
 
-    void replay(GraphicsContext&) const;
+    void replay(GraphicsContext&);
 
     DisplayItems m_currentDisplayItems;
     DisplayItems m_newDisplayItems;