Devirtualize DisplayItem

This reduces size of DisplayItem by 1 pointer (size of derived types from 48 bytes to 40 bytes on 64bit systems) and allows more sophisticated caching mechanisms, e.g. we may use memcpy/memset more freely in the future for better performance. Bug: 917911 Change-Id: I1a60dd6c8023af6c9b6840a064bd076e76f4cac7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2496291 Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Reviewed-by: Philip Rogers <pdr@chromium.org> Reviewed-by: Stefan Zager <szager@chromium.org> Cr-Commit-Position: refs/heads/master@{#880247}
chromium · May 7, 2021 · 8422a74 · 8422a74
1 parent d58d302
commit 8422a74
Show file tree

Hide file tree

Showing 13 changed files with 191 additions and 140 deletions.
diff --git a/third_party/blink/renderer/platform/graphics/paint/display_item.cc b/third_party/blink/renderer/platform/graphics/paint/display_item.cc
@@ -4,19 +4,67 @@
 
 #include "third_party/blink/renderer/platform/graphics/paint/display_item.h"
 
+#include "third_party/blink/renderer/platform/graphics/paint/drawing_display_item.h"
+#include "third_party/blink/renderer/platform/graphics/paint/foreign_layer_display_item.h"
+#include "third_party/blink/renderer/platform/graphics/paint/scrollbar_display_item.h"
 #include "third_party/blink/renderer/platform/wtf/size_assertions.h"
 
 namespace blink {
 
 struct SameSizeAsDisplayItem {
-  virtual ~SameSizeAsDisplayItem() = default;  // Allocate vtable pointer.
   void* pointer;
   IntRect rect;
   uint32_t i1;
   uint32_t i2;
 };
 ASSERT_SIZE(DisplayItem, SameSizeAsDisplayItem);
 
+void DisplayItem::Destruct() {
+  if (IsTombstone())
+    return;
+  if (IsDrawing()) {
+    static_cast<DrawingDisplayItem*>(this)->~DrawingDisplayItem();
+  } else if (IsForeignLayer()) {
+    static_cast<ForeignLayerDisplayItem*>(this)->~ForeignLayerDisplayItem();
+  } else {
+    DCHECK(IsScrollbar());
+    static_cast<ScrollbarDisplayItem*>(this)->~ScrollbarDisplayItem();
+  }
+}
+
+bool DisplayItem::EqualsForUnderInvalidation(const DisplayItem& other) const {
+  DCHECK(RuntimeEnabledFeatures::PaintUnderInvalidationCheckingEnabled());
+  SECURITY_CHECK(!IsTombstone());
+  if (client_ != other.client_ || type_ != other.type_ ||
+      fragment_ != other.fragment_ ||
+      raster_effect_outset_ != other.raster_effect_outset_ ||
+      draws_content_ != other.draws_content_ ||
+      is_cacheable_ != other.is_cacheable_)
+    return false;
+
+  if (visual_rect_ != other.visual_rect_ &&
+      // Change of empty visual rect doesn't matter.
+      (visual_rect_.IsEmpty() && other.visual_rect_.IsEmpty()) &&
+      // Visual rect of a DrawingDisplayItem not drawing content doesn't matter.
+      (!IsDrawing() || draws_content_))
+    return false;
+
+  if (IsDrawing()) {
+    return static_cast<const DrawingDisplayItem*>(this)
+        ->EqualsForUnderInvalidationImpl(
+            static_cast<const DrawingDisplayItem&>(other));
+  }
+  if (IsForeignLayer()) {
+    return static_cast<const ForeignLayerDisplayItem*>(this)
+        ->EqualsForUnderInvalidationImpl(
+            static_cast<const ForeignLayerDisplayItem&>(other));
+  }
+  DCHECK(IsScrollbar());
+  return static_cast<const ScrollbarDisplayItem*>(this)
+      ->EqualsForUnderInvalidationImpl(
+          static_cast<const ScrollbarDisplayItem&>(other));
+}
+
 #if DCHECK_IS_ON()
 
 static WTF::String PaintPhaseAsDebugString(int paint_phase) {
@@ -164,16 +212,25 @@ WTF::String DisplayItem::AsDebugString() const {
 }
 
 void DisplayItem::PropertiesAsJSON(JSONObject& json) const {
-  if (IsTombstone())
-    json.SetBoolean("ISTOMBSTONE", true);
-
   json.SetString("id", GetId().ToString());
   json.SetString("visualRect", VisualRect().ToString());
   if (GetRasterEffectOutset() != RasterEffectOutset::kNone) {
     json.SetDouble(
         "outset",
         GetRasterEffectOutset() == RasterEffectOutset::kHalfPixel ? 0.5 : 1);
   }
+
+  if (IsTombstone()) {
+    json.SetBoolean("ISTOMBSTONE", true);
+  } else if (IsDrawing()) {
+    static_cast<const DrawingDisplayItem*>(this)->PropertiesAsJSONImpl(json);
+  } else if (IsForeignLayer()) {
+    static_cast<const ForeignLayerDisplayItem*>(this)->PropertiesAsJSONImpl(
+        json);
+  } else {
+    DCHECK(IsScrollbar());
+    static_cast<const ScrollbarDisplayItem*>(this)->PropertiesAsJSONImpl(json);
+  }
 }
 
 #endif  // DCHECK_IS_ON()

diff --git a/third_party/blink/renderer/platform/graphics/paint/display_item.h b/third_party/blink/renderer/platform/graphics/paint/display_item.h
@@ -52,6 +52,8 @@ class PLATFORM_EXPORT DisplayItem {
   // - DEFINE_PAINT_PHASE_CONVERSION_METHOD(<Category>[<Subset>]) to define
   //   paintPhaseTo<Category>[<Subset>]Type(PaintPhase) method.
   enum Type {
+    kUninitializedType,
+
     kDrawingFirst,
     kDrawingPaintPhaseFirst = kDrawingFirst,
     kDrawingPaintPhaseLast = kDrawingFirst + kPaintPhaseMax,
@@ -148,32 +150,13 @@ class PLATFORM_EXPORT DisplayItem {
     kScrollbarHorizontal,
     kScrollbarVertical,
 
-    kUninitializedType,
-    kTypeLast = kUninitializedType
+    kTypeLast = kScrollbarVertical,
   };
 
-  // Some fields are copied from |client|, because we need to access them in
-  // later paint cycles when |client| may have been destroyed.
-  DisplayItem(const DisplayItemClient& client,
-              Type type,
-              const IntRect& visual_rect,
-              bool draws_content = false)
-      : client_(&client),
-        visual_rect_(visual_rect),
-        fragment_(0),
-        type_(type),
-        raster_effect_outset_(
-            static_cast<unsigned>(client.VisualRectOutsetForRasterEffects())),
-        draws_content_(draws_content),
-        is_cacheable_(client.IsCacheable()),
-        is_tombstone_(false),
-        known_to_be_opaque_is_set_(false),
-        known_to_be_opaque_(false) {
-    DCHECK_EQ(client.VisualRectOutsetForRasterEffects(),
-              GetRasterEffectOutset());
-  }
-
-  virtual ~DisplayItem() = default;
+  DisplayItem(const DisplayItem&) = delete;
+  DisplayItem(DisplayItem&&) = delete;
+  DisplayItem& operator=(const DisplayItem&) = delete;
+  DisplayItem& operator=(DisplayItem&&) = delete;
 
   // Ids are for matching new DisplayItems with existing DisplayItems.
   struct Id {
@@ -247,36 +230,60 @@ class PLATFORM_EXPORT DisplayItem {
   bool IsCacheable() const { return is_cacheable_; }
   void SetUncacheable() { is_cacheable_ = false; }
 
-  virtual bool Equals(const DisplayItem& other) const {
-    // Failure of this DCHECK would cause bad casts in subclasses.
-    SECURITY_CHECK(!is_tombstone_);
-    return client_ == other.client_ && type_ == other.type_ &&
-           fragment_ == other.fragment_;
-  }
+  bool EqualsForUnderInvalidation(const DisplayItem& other) const;
 
   // True if this DisplayItem is the tombstone/"dead display item" as part of
-  // moving an item from one list to another. See the default constructor of
-  // DisplayItem.
-  bool IsTombstone() const { return is_tombstone_; }
+  // moving an item from one list to another. See CreateTombstone().
+  bool IsTombstone() const { return !is_not_tombstone_; }
 
   bool DrawsContent() const { return draws_content_; }
 
 #if DCHECK_IS_ON()
   static WTF::String TypeAsDebugString(DisplayItem::Type);
   WTF::String AsDebugString() const;
-  virtual void PropertiesAsJSON(JSONObject&) const;
+  void PropertiesAsJSON(JSONObject&) const;
 #endif
 
+ protected:
+  // Some fields are copied from |client|, because we need to access them in
+  // later paint cycles when |client| may have been destroyed.
+  DisplayItem(const DisplayItemClient& client,
+              Type type,
+              const IntRect& visual_rect,
+              bool draws_content = false)
+      : client_(&client),
+        visual_rect_(visual_rect),
+        fragment_(0),
+        type_(type),
+        raster_effect_outset_(
+            static_cast<unsigned>(client.VisualRectOutsetForRasterEffects())),
+        draws_content_(draws_content),
+        is_cacheable_(client.IsCacheable()),
+        is_not_tombstone_(true),
+        known_to_be_opaque_is_set_(false),
+        known_to_be_opaque_(false) {
+    DCHECK_EQ(client.VisualRectOutsetForRasterEffects(),
+              GetRasterEffectOutset());
+  }
+
+  ~DisplayItem() = default;
+
  private:
   friend class DisplayItemList;
 
-  // The default DisplayItem constructor is only used by DisplayItemList::
-  // AppendByMoving() and ReplaceLastByMoving() where a tombstone DisplayItem is
-  // constructed at the source location. Only set draws_content_ to false and
-  // is_tombstone_ to true, leaving other fields as-is so that we can get their
-  // original values. |visual_rect_| and |raster_effect_outset_| are special,
-  // see DisplayItemList::AppendByMoving().
-  DisplayItem() : draws_content_(false), is_tombstone_(true) {}
+  // DisplayItemList calls this method to destruct a DisplayItem in place.
+  // It knows how to destruct subclasses.
+  void Destruct();
+
+  // Used by DisplayItemList::AppendByMoving() and ReplaceLastByMoving() where
+  // a tombstone DisplayItem is constructed at the source location. Only set
+  // draws_content_ and is_not_tombstone_ to false, leaving other fields as-is
+  // so that we can get their original values for debugging and raster
+  // invalidation.
+  void CreateTombstone() {
+    draws_content_ = false;
+    is_not_tombstone_ = false;
+  }
 
   const DisplayItemClient* client_;
   IntRect visual_rect_;
@@ -287,7 +294,9 @@ class PLATFORM_EXPORT DisplayItem {
   unsigned raster_effect_outset_ : 2;
   unsigned draws_content_ : 1;
   unsigned is_cacheable_ : 1;
-  unsigned is_tombstone_ : 1;
+  // This is not |is_tombstone_| to allow memset(0) to clear a display item to
+  // be a tombstone.
+  unsigned is_not_tombstone_ : 1;
 
  protected:
   // These are for DrawingDisplayItem to save memory.

diff --git a/third_party/blink/renderer/platform/graphics/paint/display_item_list.cc b/third_party/blink/renderer/platform/graphics/paint/display_item_list.cc
@@ -10,10 +10,8 @@
 namespace blink {
 
 DisplayItemList::~DisplayItemList() {
-  for (auto& item : *this) {
-    (void)item;  // MSVC incorrectly reports this variable as unused.
-    item.~DisplayItem();
-  }
+  for (auto& item : *this)
+    item.Destruct();
 }
 
 #if DCHECK_IS_ON()

diff --git a/third_party/blink/renderer/platform/graphics/paint/display_item_list.h b/third_party/blink/renderer/platform/graphics/paint/display_item_list.h
@@ -53,7 +53,7 @@ class PLATFORM_EXPORT DisplayItemList {
   DisplayItem& ReplaceLastByMoving(DisplayItem& item) {
     SECURITY_CHECK(!item.IsTombstone());
     DisplayItem& last = back();
-    last.~DisplayItem();
+    last.Destruct();
     return MoveItem(item, &last);
   }
 
@@ -207,21 +207,13 @@ class PLATFORM_EXPORT DisplayItemList {
 
     // Created a tombstone/"dead display item" that can be safely destructed but
     // should never be used except for debugging and raster invalidation.
-    new (&item) DisplayItem;
+    item.CreateTombstone();
     DCHECK(item.IsTombstone());
-    // We need |visual_rect_| and |outset_for_raster_effects_| of the old
-    // display item for raster invalidation. Also, the fields that make up the
-    // ID (|client_|, |type_| and |fragment_|) need to match. As their values
-    // were either initialized to default values or were left uninitialized by
-    // DisplayItem's default constructor, now copy their original values back
-    // from |result|.
+    // Original values for other fields are kept for debugging and raster
+    // invalidation.
     DisplayItem& new_item = *static_cast<DisplayItem*>(new_item_space);
-    item.client_ = new_item.client_;
-    item.type_ = new_item.type_;
-    item.fragment_ = new_item.fragment_;
-    DCHECK_EQ(item.GetId(), new_item.GetId());
-    item.visual_rect_ = new_item.visual_rect_;
-    item.raster_effect_outset_ = new_item.raster_effect_outset_;
+    DCHECK_EQ(item.VisualRect(), new_item.VisualRect());
+    DCHECK_EQ(item.GetRasterEffectOutset(), new_item.GetRasterEffectOutset());
     return new_item;
   }
 

diff --git a/third_party/blink/renderer/platform/graphics/paint/display_item_test.cc b/third_party/blink/renderer/platform/graphics/paint/display_item_test.cc
@@ -20,5 +20,10 @@ TEST(DisplayItemTest, DebugStringsExist) {
 }
 #endif  // DCHECK_IS_ON()
 
+TEST(DisplayItemTest, AllZeroIsTombstone) {
+  alignas(alignof(DisplayItem)) uint8_t buffer[sizeof(DisplayItem)] = {0};
+  EXPECT_TRUE(reinterpret_cast<const DisplayItem*>(buffer)->IsTombstone());
+}
+
 }  // namespace
 }  // namespace blink
diff --git a/third_party/blink/renderer/platform/graphics/paint/drawing_display_item.cc b/third_party/blink/renderer/platform/graphics/paint/drawing_display_item.cc
@@ -7,12 +7,11 @@
 #include "cc/paint/display_item_list.h"
 #include "third_party/blink/renderer/platform/graphics/graphics_context.h"
 #include "third_party/blink/renderer/platform/graphics/paint/paint_canvas.h"
+#include "third_party/blink/renderer/platform/wtf/size_assertions.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkData.h"
 
-#include "third_party/blink/renderer/platform/graphics/logging_canvas.h"
-
 namespace blink {
 
 static SkBitmap RecordToBitmap(sk_sp<const PaintRecord> record,
@@ -51,13 +50,12 @@ static bool BitmapsEqual(sk_sp<const PaintRecord> record1,
   return !mismatch_count;
 }
 
-bool DrawingDisplayItem::Equals(const DisplayItem& other) const {
-  if (!DisplayItem::Equals(other))
-    return false;
+bool DrawingDisplayItem::EqualsForUnderInvalidationImpl(
+    const DrawingDisplayItem& other) const {
+  DCHECK(RuntimeEnabledFeatures::PaintUnderInvalidationCheckingEnabled());
 
   const auto& record = GetPaintRecord();
-  const auto& other_record =
-      static_cast<const DrawingDisplayItem&>(other).GetPaintRecord();
+  const auto& other_record = other.GetPaintRecord();
   if (!record && !other_record)
     return true;
   if (!record || !other_record)

diff --git a/third_party/blink/renderer/platform/graphics/paint/drawing_display_item.h b/third_party/blink/renderer/platform/graphics/paint/drawing_display_item.h
@@ -16,15 +16,6 @@ namespace blink {
 
 // DrawingDisplayItem contains recorded painting operations which can be
 // replayed to produce a rastered output.
-//
-// This class has two notions of the bounds around the content that was recorded
-// and will be produced by the item. The first is the |record_bounds| which
-// describes the bounds of all content in the |record| in the space of the
-// record. The second is the |visual_rect| which should describe the same thing,
-// but takes into account transforms and clips that would apply to the
-// PaintRecord, and is in the space of the DisplayItemList. This allows the
-// visual_rect to be compared between DrawingDisplayItems, and to give bounds
-// around what the user can actually see from the PaintRecord.
 class PLATFORM_EXPORT DrawingDisplayItem : public DisplayItem {
  public:
   DISABLE_CFI_PERF
@@ -35,8 +26,6 @@ class PLATFORM_EXPORT DrawingDisplayItem : public DisplayItem {
 
   const sk_sp<const PaintRecord>& GetPaintRecord() const { return record_; }
 
-  bool Equals(const DisplayItem& other) const final;
-
   bool KnownToBeOpaque() const {
     if (!RuntimeEnabledFeatures::CompositeAfterPaintEnabled())
       return false;
@@ -54,6 +43,12 @@ class PLATFORM_EXPORT DrawingDisplayItem : public DisplayItem {
   SkColor BackgroundColor(float& area) const;
 
  private:
+  friend class DisplayItem;
+  bool EqualsForUnderInvalidationImpl(const DrawingDisplayItem&) const;
+#if DCHECK_IS_ON()
+  void PropertiesAsJSONImpl(JSONObject&) const {}
+#endif
+
   bool CalculateKnownToBeOpaque(const PaintRecord*) const;
 
   sk_sp<const PaintRecord> record_;