Revert of cc: Shrink size of display item

Revert of cc: Shrink size of display item (patchset #9 id:160001 of https://codereview.chromium.org/1494223003/ )

Reason for revert:
Causes regression in smoothness benchmark.

I think issue 513016 probably needs to be fixed before
turning on display list rasterization, for this reason.

BUG=569021, 513016
Original issue's description:
> cc: Shrink size of display item
> 
> This removes all of the base class members from display item and instead
> calculates them during allocation.  This is done by processing each item
> as it is added, which requires passing in all the ctor args to
> CreateAndAppendItem.  This reduces the size of all display items by 16
> bytes so that the largest is now "only" 80 bytes and the smallest is 8.
> 
> Also, DisplayItemList had a bug where it would allocate an item,
> (maybe) process all added items, and then SetNew on that last item.  In
> that case, the processing would skip the final item because it was just
> a newly allocated item.  This patch fixes that by the above changes
> to CreateAndAppendItem.
> 
> R=vmpstr@chromium.org,chrishtr@chromium.org
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> 
> Committed: https://crrev.com/3b006fb5a67c9e9bc0adf3f68d72a10afa221198
> Cr-Commit-Position: refs/heads/master@{#364437}

TBR=chrishtr@chromium.org,danakj@chromium.org,vmpstr@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

[enne (OOO), 2015-12-14 22:03:29]

Created Revert of cc: Shrink size of display item

[commit-bot: I haz the power, 2015-12-14 22:06:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1521373005/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1521373005/1

[chrishtr, 2015-12-14 22:06:47]

Description was changed from

==========
Revert of cc: Shrink size of display item (patchset #9 id:160001 of
https://codereview.chromium.org/1494223003/ )

Reason for revert:
Causes regression in smoothness benchmark.

I think issue 513016 probably needs to be fixed before
turning on display list rasterization, for this reason.

BUG=569021

Original issue's description:
> cc: Shrink size of display item
> 
> This removes all of the base class members from display item and instead
> calculates them during allocation.  This is done by processing each item
> as it is added, which requires passing in all the ctor args to
> CreateAndAppendItem.  This reduces the size of all display items by 16
> bytes so that the largest is now "only" 80 bytes and the smallest is 8.
> 
> Also, DisplayItemList had a bug where it would allocate an item,
> (maybe) process all added items, and then SetNew on that last item.  In
> that case, the processing would skip the final item because it was just
> a newly allocated item.  This patch fixes that by the above changes
> to CreateAndAppendItem.
> 
> R=vmpstr@chromium.org,chrishtr@chromium.org
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> 
> Committed: https://crrev.com/3b006fb5a67c9e9bc0adf3f68d72a10afa221198
> Cr-Commit-Position: refs/heads/master@{#364437}

TBR=chrishtr@chromium.org,danakj@chromium.org,vmpstr@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
==========

to

==========
Revert of cc: Shrink size of display item (patchset #9 id:160001 of
https://codereview.chromium.org/1494223003/ )

Reason for revert:
Causes regression in smoothness benchmark.

I think issue 513016 probably needs to be fixed before
turning on display list rasterization, for this reason.

BUG=569021,513016

Original issue's description:
> cc: Shrink size of display item
> 
> This removes all of the base class members from display item and instead
> calculates them during allocation.  This is done by processing each item
> as it is added, which requires passing in all the ctor args to
> CreateAndAppendItem.  This reduces the size of all display items by 16
> bytes so that the largest is now "only" 80 bytes and the smallest is 8.
> 
> Also, DisplayItemList had a bug where it would allocate an item,
> (maybe) process all added items, and then SetNew on that last item.  In
> that case, the processing would skip the final item because it was just
> a newly allocated item.  This patch fixes that by the above changes
> to CreateAndAppendItem.
> 
> R=vmpstr@chromium.org,chrishtr@chromium.org
> CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
> 
> Committed: https://crrev.com/3b006fb5a67c9e9bc0adf3f68d72a10afa221198
> Cr-Commit-Position: refs/heads/master@{#364437}

TBR=chrishtr@chromium.org,danakj@chromium.org,vmpstr@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
==========

[commit-bot: I haz the power, 2015-12-14 22:08:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-14 22:09:00]

Failed to apply patch for cc/blink/web_display_item_list_impl.cc:
While running git apply --index -3 -p1;
  error: patch failed: cc/blink/web_display_item_list_impl.cc:169
  Falling back to three-way merge...
  Applied patch to 'cc/blink/web_display_item_list_impl.cc' with conflicts.
  U cc/blink/web_display_item_list_impl.cc

Patch:       cc/blink/web_display_item_list_impl.cc
Index: cc/blink/web_display_item_list_impl.cc
diff --git a/cc/blink/web_display_item_list_impl.cc
b/cc/blink/web_display_item_list_impl.cc
index
9859d1c39ab4782af2b9d188a8aaad336593f6d3..b030e937b32003ed94906e8f7e426a0901e79956
100644
--- a/cc/blink/web_display_item_list_impl.cc
+++ b/cc/blink/web_display_item_list_impl.cc
@@ -50,11 +50,13 @@
     const blink::WebRect& visual_rect,
     const SkPicture* picture) {
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::DrawingDisplayItem>(
-        visual_rect, skia::SharePtr(const_cast<SkPicture*>(picture)));
-  } else {
-    cc::DrawingDisplayItem item(
-        skia::SharePtr(const_cast<SkPicture*>(picture)));
+    auto* item =
+        display_item_list_->CreateAndAppendItem<cc::DrawingDisplayItem>(
+            visual_rect);
+    item->SetNew(skia::SharePtr(const_cast<SkPicture*>(picture)));
+  } else {
+    cc::DrawingDisplayItem item;
+    item.SetNew(skia::SharePtr(const_cast<SkPicture*>(picture)));
     display_item_list_->RasterIntoCanvas(item);
   }
 }
@@ -68,10 +70,12 @@
     rounded_rects.push_back(rounded_clip_rects[i]);
   }
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::ClipDisplayItem>(
-        visual_rect, clip_rect, rounded_rects);
-  } else {
-    cc::ClipDisplayItem item(clip_rect, rounded_rects);
+    auto* item = display_item_list_->CreateAndAppendItem<cc::ClipDisplayItem>(
+        visual_rect);
+    item->SetNew(clip_rect, rounded_rects);
+  } else {
+    cc::ClipDisplayItem item;
+    item.SetNew(clip_rect, rounded_rects);
     display_item_list_->RasterIntoCanvas(item);
   }
 }
@@ -92,10 +96,13 @@
     SkRegion::Op clip_op,
     bool antialias) {
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::ClipPathDisplayItem>(
-        visual_rect, clip_path, clip_op, antialias);
-  } else {
-    cc::ClipPathDisplayItem item(clip_path, clip_op, antialias);
+    auto* item =
+        display_item_list_->CreateAndAppendItem<cc::ClipPathDisplayItem>(
+            visual_rect);
+    item->SetNew(clip_path, clip_op, antialias);
+  } else {
+    cc::ClipPathDisplayItem item;
+    item.SetNew(clip_path, clip_op, antialias);
     display_item_list_->RasterIntoCanvas(item);
   }
 }
@@ -114,10 +121,13 @@
     const blink::WebRect& visual_rect,
     const blink::WebFloatRect& clip_rect) {
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::FloatClipDisplayItem>(
-        visual_rect, clip_rect);
-  } else {
-    cc::FloatClipDisplayItem item(clip_rect);
+    auto* item =
+        display_item_list_->CreateAndAppendItem<cc::FloatClipDisplayItem>(
+            visual_rect);
+    item->SetNew(clip_rect);
+  } else {
+    cc::FloatClipDisplayItem item;
+    item.SetNew(clip_rect);
     display_item_list_->RasterIntoCanvas(item);
   }
 }
@@ -139,10 +149,13 @@
   transform.matrix() = matrix;
 
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::TransformDisplayItem>(
-        visual_rect, transform);
-  } else {
-    cc::TransformDisplayItem item(transform);
+    auto* item =
+        display_item_list_->CreateAndAppendItem<cc::TransformDisplayItem>(
+            visual_rect);
+    item->SetNew(transform);
+  } else {
+    cc::TransformDisplayItem item;
+    item.SetNew(transform);
     display_item_list_->RasterIntoCanvas(item);
   }
 }
@@ -169,13 +182,15 @@
   // value, but that breaks slimming paint reftests.
 
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::CompositingDisplayItem>(
-        visual_rect, static_cast<uint8_t>(gfx::ToFlooredInt(255 * opacity)),
-        xfermode, bounds, skia::SharePtr(color_filter));
-  } else {
-    cc::CompositingDisplayItem item(
-        static_cast<uint8_t>(gfx::ToFlooredInt(255 * opacity)), xfermode,
-        bounds, skia::SharePtr(color_filter));
+    auto* item =
+        display_item_list_->CreateAndAppendItem<cc::CompositingDisplayItem>(
+            visual_rect);
+    item->SetNew(static_cast<uint8_t>(gfx::ToFlooredInt(255 * opacity)),
+                 xfermode, bounds, skia::SharePtr(color_filter));
+  } else {
+    cc::CompositingDisplayItem item;
+    item.SetNew(static_cast<uint8_t>(gfx::ToFlooredInt(255 * opacity)),
+                xfermode, bounds, skia::SharePtr(color_filter));
     display_item_list_->RasterIntoCanvas(item);
   }
 }
@@ -198,10 +213,12 @@
       static_cast<const WebFilterOperationsImpl&>(filters);
 
   if (display_item_list_->RetainsIndividualDisplayItems()) {
-    display_item_list_->CreateAndAppendItem<cc::FilterDisplayItem>(
-        visual_rect, filters_impl.AsFilterOperations(), bounds);
-  } else {
-    cc::FilterDisplayItem item(filters_impl.AsFilterOperations(), bounds);
+    auto* item =
display_item_list_->CreateAndAppendItem<cc::FilterDisplayItem>(
+        visual_rect);
+    item->SetNew(filters_impl.AsFilterOperations(), bounds);
+  } else {
+    cc::FilterDisplayItem item;
+    item.SetNew(filters_impl.AsFilterOperations(), bounds);
     display_item_list_->RasterIntoCanvas(item);
   }
 }

[chrishtr, 2015-12-14 22:11:29]

Interesting. How did this patch regress is_suitable_for_gpu_rasterization? Not
seeing the
reason yet from reading the code.

[commit-bot: I haz the power, 2015-12-14 22:11:57]

The CQ bit was unchecked by commit-bot@chromium.org

[enne (OOO), 2015-12-14 22:24:44]

On 2015/12/14 at 22:11:29, chrishtr wrote:
> Interesting. How did this patch regress is_suitable_for_gpu_rasterization? Not
seeing the
> reason yet from reading the code.

https://codereview.chromium.org/1494223003/#msg14

See the comment about issue 513016.

[chrishtr, 2015-12-14 22:33:13]

On 2015/12/14 at 22:24:44, enne wrote:
> On 2015/12/14 at 22:11:29, chrishtr wrote:
> > Interesting. How did this patch regress is_suitable_for_gpu_rasterization?
Not seeing the
> > reason yet from reading the code.
> 
> https://codereview.chromium.org/1494223003/#msg14
> 
> See the comment about issue 513016.

I saw that, but am still not sure about why this patch specifically changes the
situation..why now and not before?

[enne (OOO), 2015-12-14 22:46:04]

On 2015/12/14 at 22:33:13, chrishtr wrote:
> On 2015/12/14 at 22:24:44, enne wrote:
> > On 2015/12/14 at 22:11:29, chrishtr wrote:
> > > Interesting. How did this patch regress is_suitable_for_gpu_rasterization?
Not seeing the
> > > reason yet from reading the code.
> > 
> > https://codereview.chromium.org/1494223003/#msg14
> > 
> > See the comment about issue 513016.
> 
> I saw that, but am still not sure about why this patch specifically changes
the situation..why now and not before?

This patch changes the gpu rasterization veto to check its conditions per
display item instead of per flattened picture when using flattened pictures. 
(The behavior when rasterizing from display lists is unchanged and continues to
be per display item, due to bug 513016.)  Per display item is a much more lax
check, and so that telemetry test no longer hits the veto.  Therefore, the test
is rastering too many paths using ganesh, which is slow enough to drive down
smoothness metrics.

[chrishtr, 2015-12-14 22:51:12]

On 2015/12/14 at 22:46:04, enne wrote:
> On 2015/12/14 at 22:33:13, chrishtr wrote:
> > On 2015/12/14 at 22:24:44, enne wrote:
> > > On 2015/12/14 at 22:11:29, chrishtr wrote:
> > > > Interesting. How did this patch regress
is_suitable_for_gpu_rasterization? Not seeing the
> > > > reason yet from reading the code.
> > > 
> > > https://codereview.chromium.org/1494223003/#msg14
> > > 
> > > See the comment about issue 513016.
> > 
> > I saw that, but am still not sure about why this patch specifically changes
the situation..why now and not before?
> 
> This patch changes the gpu rasterization veto to check its conditions per
display item instead of per flattened picture when using flattened pictures. 
(The behavior when rasterizing from display lists is unchanged and continues to
be per display item, due to bug 513016.)  Per display item is a much more lax
check, and so that telemetry test no longer hits the veto.  Therefore, the test
is rastering too many paths using ganesh, which is slow enough to drive down
smoothness metrics.

Ah, got it thanks. I couldn't find the code somehow which looked at the
flattened picture's bit.

[enne (OOO), 2016-01-04 19:06:51]

Closing this as senorblanco fixed it via a more targetted
https://codereview.chromium.org/1538913002/