Blink-side contiguous allocation of display items.

Source/platform/graphics/paint/DisplayItemList.h

 // Copyright 2014 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.
 
 #ifndef DisplayItemList_h
 #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);
 public:
     static PassOwnPtr<DisplayItemList> create()
     {
         return adoptPtr(new DisplayItemList());
     }
 
     // These methods are called during paint invalidation.
     void invalidate(DisplayItemClient);
     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);
 
     // True if the last display item is a begin that doesn't draw content.
     bool lastDisplayItemIsNoopBegin() const;
     void removeLastDisplayItem();
 
     void beginSkippingCache() { ++m_skippingCacheCount; }
     void endSkippingCache() { ASSERT(m_skippingCacheCount > 0); --m_skippingCacheCount; }
     bool skippingCache() const { return m_skippingCacheCount; }
@@ skipping 21 matching lines @@
 #if ENABLE(ASSERT)
     size_t newDisplayItemsSize() const { return m_newDisplayItems.size(); }
 #endif
 
 #ifndef NDEBUG
     void showDebugData() const;
 #endif
 
 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) { }
 
 private:
     friend class DisplayItemListTest;
     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
     WTF::String displayItemsAsDebugString(const DisplayItems&) const;
 #endif
 
     // Indices into PaintList of all DrawingDisplayItems and BeginSubtreeDisplayItems of each client.
     // Temporarily used during merge to find out-of-order display items.
     using DisplayItemIndicesByClientMap = HashMap<DisplayItemClient, Vector<size_t>>;
 
     static size_t findMatchingItemFromIndex(const DisplayItem::Id&, DisplayItem::Type matchingType, const DisplayItemIndicesByClientMap&, const DisplayItems&);
     static void addItemToIndex(DisplayItemClient, DisplayItem::Type, size_t index, DisplayItemIndicesByClientMap&);
     DisplayItems::Iterator findOutOfOrderCachedItem(DisplayItems::Iterator currentIt, const DisplayItem::Id&, DisplayItem::Type, DisplayItemIndicesByClientMap&);
     DisplayItems::Iterator findOutOfOrderCachedItemForward(DisplayItems::Iterator currentIt, const DisplayItem::Id&, DisplayItem::Type, DisplayItemIndicesByClientMap&);
 
 #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;
 
     // Contains all clients having valid cached paintings if updated.
     // It's lazily updated in updateValidlyCachedClientsIfNeeded().
     // FIXME: In the future we can replace this with client-side repaint flags
     // to avoid the cost of building and querying the hash table.
     mutable HashSet<DisplayItemClient> m_validlyCachedClients;
     mutable bool m_validlyCachedClientsDirty;
@@ skipping 22 matching lines @@
     // This is used to check duplicated ids during add(). We could also check during
     // updatePaintList(), but checking during add() helps developer easily find where
     // the duplicated ids are from.
     DisplayItemIndicesByClientMap m_newDisplayItemIndicesByClient;
 #endif
 };
 
 } // namespace blink
 
 #endif // DisplayItemList_h