cc: Shrink size of display item

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}

[enne (OOO), 2015-12-03 20:14:10]

Description was changed from

==========
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
==========

to

==========
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
==========

[enne (OOO), 2015-12-03 20:15:38]

I could have probably fixed the processing bug first and then after removed the
debug info members, but I started on the latter first and everything got too
entangled to try to do them one at a time.  

Sorry for the large patch.  :)

[enne (OOO), 2015-12-03 23:34:38]

enne@chromium.org changed reviewers:
+ danakj@chromium.org

[chrishtr, 2015-12-04 00:34:22]

This undoes optimizations made in patches like
https://chromium.googlesource.com/chromium/src/+/b929714ec046700f577c5ecc0987....

Right? Will this hurt record time?

[vmpstr, 2015-12-04 00:43:09]

https://codereview.chromium.org/1494223003/diff/20001/cc/playback/clip_displa...
File cc/playback/clip_display_item.h (right):

https://codereview.chromium.org/1494223003/diff/20001/cc/playback/clip_displa...
cc/playback/clip_display_item.h:27: void SetNew(gfx::Rect clip_rect,
Are the SetNew functions in all of these unused now (or can be private)?

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_tiles.cc:137: std::move(picture));
For all these 

skia::RefPtr<SkPicture> picture = stuff;
...(..., std::move(picture));

i wonder if it would just be cleaner to

...(..., stuff); directly? Or do you think this is cleaner?

[danakj, 2015-12-04 00:53:55]

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_tiles.cc:137: std::move(picture));
On 2015/12/04 00:43:09, vmpstr wrote:
> For all these 
> 
> skia::RefPtr<SkPicture> picture = stuff;
> ...(..., std::move(picture));
> 
> i wonder if it would just be cleaner to
> 
> ...(..., stuff); directly? Or do you think this is cleaner?

When you get a pointer from skia APIs the first thing you do with it is put it
in a skia::RefPtr.

[vmpstr, 2015-12-04 01:24:29]

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_tiles.cc:137: std::move(picture));
On 2015/12/04 00:53:54, danakj (behind on reviews) wrote:
> On 2015/12/04 00:43:09, vmpstr wrote:
> > For all these 
> > 
> > skia::RefPtr<SkPicture> picture = stuff;
> > ...(..., std::move(picture));
> > 
> > i wonder if it would just be cleaner to
> > 
> > ...(..., stuff); directly? Or do you think this is cleaner?
> 
> When you get a pointer from skia APIs the first thing you do with it is put it
> in a skia::RefPtr.

"stuff" would include skia::AdoptRef()

[danakj, 2015-12-04 01:24:54]

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):

https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_pixeltest_tiles.cc:137: std::move(picture));
On 2015/12/04 01:24:28, vmpstr wrote:
> On 2015/12/04 00:53:54, danakj (behind on reviews) wrote:
> > On 2015/12/04 00:43:09, vmpstr wrote:
> > > For all these 
> > > 
> > > skia::RefPtr<SkPicture> picture = stuff;
> > > ...(..., std::move(picture));
> > > 
> > > i wonder if it would just be cleaner to
> > > 
> > > ...(..., stuff); directly? Or do you think this is cleaner?
> > 
> > When you get a pointer from skia APIs the first thing you do with it is put
it
> > in a skia::RefPtr.
> 
> "stuff" would include skia::AdoptRef()

Ooh ok that'd be fine. :)

[enne (OOO), 2015-12-04 19:53:30]

PTAL

On 2015/12/04 at 00:34:22, chrishtr wrote:
> This undoes optimizations made in patches like
https://chromium.googlesource.com/chromium/src/+/b929714ec046700f577c5ecc0987....
> 
> Right? Will this hurt record time

I still suspect that my code would not have regressed, as a lot of the problem
with virtuals is that we were processing a lot of objects with virtuals long
after we'd looked at them, so I would expect lots of cache misses.  My code was
processing the object right after it was added, so it seems more likely to be
more performant.

At any rate, to avoid performance concerns, I left memory as a virtual, as it's
now only called when tracing.  The other two (op count and gpu rasterization)
are now all non-virtual inlined (mostly) member functions.  These should
probably will just be compiled out when generating the templated code most of
the time.

On 2015/12/04 at 00:43:09, vmpstr wrote:
>
https://codereview.chromium.org/1494223003/diff/20001/cc/playback/clip_displa...
> File cc/playback/clip_display_item.h (right):
> 
>
https://codereview.chromium.org/1494223003/diff/20001/cc/playback/clip_displa...
> cc/playback/clip_display_item.h:27: void SetNew(gfx::Rect clip_rect,
> Are the SetNew functions in all of these unused now (or can be private)?

Done.
 
>
https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_pixeltest_tiles.cc (right):
> 
>
https://codereview.chromium.org/1494223003/diff/20001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_pixeltest_tiles.cc:137: std::move(picture));
> For all these 
> 
> skia::RefPtr<SkPicture> picture = stuff;
> ...(..., std::move(picture));
> 
> i wonder if it would just be cleaner to
> 
> ...(..., stuff); directly? Or do you think this is cleaner?

Done.

[enne (OOO), 2015-12-08 18:54:08]

...ping?

[vmpstr, 2015-12-08 19:07:02]

lgtm

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/drawing_dis...
File cc/playback/drawing_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/drawing_dis...
cc/playback/drawing_display_item.cc:42: picture_ = picture;
Still move to save on refcounts?

[danakj, 2015-12-08 19:18:16]

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
File cc/playback/clip_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
cc/playback/clip_display_item.cc:21: gfx::Rect clip_rect,
This should be const Rect&

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
cc/playback/clip_display_item.cc:30: void ClipDisplayItem::SetNew(gfx::Rect
clip_rect,
hm, that should be const Rect&

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
File cc/playback/clip_display_item.h (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
cc/playback/clip_display_item.h:28: void FromProtobuf(const proto::DisplayItem&
proto) override;
should these From methods be private then?

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_path_d...
File cc/playback/clip_path_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_path_d...
cc/playback/clip_path_display_item.cc:87: return 0;
you could inline this in the header (if clang plugin has been updated yet - i
think so?)

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_path_d...
cc/playback/clip_path_display_item.cc:124: return 0;
ditto for all these "return 0"s

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/compositing...
File cc/playback/compositing_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/compositing...
cc/playback/compositing_display_item.cc:26: SetNew(alpha, xfermode, bounds, cf);
move cf

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/compositing...
cc/playback/compositing_display_item.cc:46: color_filter_ = cf;
this should move cf too while you're at it

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_item.h
File cc/playback/display_item.h (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item.h:33: virtual size_t ExternalMemoryUsage() const = 0;
Can you leave a comment about this being for tracing, or maybe consider
ForTracing in the name?

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:160: void DisplayItemList::Finalize() {
Do we actually need the Finalize method after this?

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:204: for (const DisplayItem& item : items_) {
no auto? :)

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.h:72: // TODO(enne): This should forward the args.
I'll work on allowing forward soon, now that you can write T&& here.

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (left):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:587: canvas->drawPath(path, paint);
Where'd this case go?

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/drawing_dis...
File cc/playback/drawing_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/drawing_dis...
cc/playback/drawing_display_item.cc:27: SetNew(picture);
prefer to move() the reference when it makes sense like here

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/drawing_dis...
cc/playback/drawing_display_item.cc:42: picture_ = picture;
ditto

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/filter_disp...
File cc/playback/filter_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/filter_disp...
cc/playback/filter_display_item.cc:53: // TODO(dtrainor): Support deserializing
FilterOperations
random comment wrapping?

https://codereview.chromium.org/1494223003/diff/60001/cc/test/solid_color_con...
File cc/test/solid_color_content_layer_client.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/test/solid_color_con...
cc/test/solid_color_content_layer_client.cc:46: std::move(picture));
or pass the AdoptRef directly like you did elsewhere

[enne (OOO), 2015-12-08 19:47:27]

Thanks, PTAL

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
File cc/playback/clip_display_item.h (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_displa...
cc/playback/clip_display_item.h:28: void FromProtobuf(const proto::DisplayItem&
proto) override;
On 2015/12/08 at 19:18:14, danakj (behind on reviews) wrote:
> should these From methods be private then?

ToProtoBuf should be public.
FromProtoBuf is now protected (as it's in the base class).

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_path_d...
File cc/playback/clip_path_display_item.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/clip_path_d...
cc/playback/clip_path_display_item.cc:87: return 0;
On 2015/12/08 at 19:18:14, danakj (behind on reviews) wrote:
> you could inline this in the header (if clang plugin has been updated yet - i
think so?)

Does not appear to.

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_item.h
File cc/playback/display_item.h (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item.h:33: virtual size_t ExternalMemoryUsage() const = 0;
On 2015/12/08 at 19:18:14, danakj (behind on reviews) wrote:
> Can you leave a comment about this being for tracing, or maybe consider
ForTracing in the name?

Left a comment.

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list.cc:160: void DisplayItemList::Finalize() {
On 2015/12/08 at 19:18:15, danakj (behind on reviews) wrote:
> Do we actually need the Finalize method after this?

Yes.  We can remove Finalize once we no longer use the cached picture.

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
File cc/playback/display_item_list_unittest.cc (left):

https://codereview.chromium.org/1494223003/diff/60001/cc/playback/display_ite...
cc/playback/display_item_list_unittest.cc:587: canvas->drawPath(path, paint);
On 2015/12/08 at 19:18:16, danakj (behind on reviews) wrote:
> Where'd this case go?

This patch changes the behavior of DisplayItemList to be more consistent.  For
both use cached picture and not use cached picture, it process the gpu veto in
the same way (item by item instead of the entire cached picture).

This test no longer applies.  See crbug.com/513016.

https://codereview.chromium.org/1494223003/diff/60001/cc/test/solid_color_con...
File cc/test/solid_color_content_layer_client.cc (right):

https://codereview.chromium.org/1494223003/diff/60001/cc/test/solid_color_con...
cc/test/solid_color_content_layer_client.cc:46: std::move(picture));
On 2015/12/08 at 19:18:16, danakj (behind on reviews) wrote:
> or pass the AdoptRef directly like you did elsewhere

Oops.  Missed that one.  :C

[danakj, 2015-12-08 19:52:04]

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
File cc/playback/display_item.h (right):

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
cc/playback/display_item.h:36: virtual void FromProtobuf(const
proto::DisplayItem& proto) = 0;
Why is this in the base class if it's protected? It seems like a helper method
called by the Proto constructors (should it just collapse into the
constructors?)

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
cc/playback/display_item_list.h:75: // None of the items might individually
trigger a veto even though they
TODO(crbug.com/513016): 
^ would be nice

[enne (OOO), 2015-12-08 20:00:59]

PTAL

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
File cc/playback/display_item.h (right):

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
cc/playback/display_item.h:36: virtual void FromProtobuf(const
proto::DisplayItem& proto) = 0;
On 2015/12/08 at 19:52:04, danakj (behind on reviews) wrote:
> Why is this in the base class if it's protected? It seems like a helper method
called by the Proto constructors (should it just collapse into the
constructors?)

Collapsed.  Removed.  Done.

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
File cc/playback/display_item_list.h (right):

https://codereview.chromium.org/1494223003/diff/120001/cc/playback/display_it...
cc/playback/display_item_list.h:75: // None of the items might individually
trigger a veto even though they
On 2015/12/08 at 19:52:04, danakj (behind on reviews) wrote:
> TODO(crbug.com/513016): 
> ^ would be nice

Done.

[enne (OOO), 2015-12-09 22:57:52]

chrishtr, danakj: you both responded here previously.  Did you have any other
comments?

[danakj, 2015-12-09 22:59:34]

oops sorry LGTM

[chrishtr, 2015-12-09 23:21:32]

lgtm

[enne (OOO), 2015-12-09 23:34:38]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2015-12-09 23:34:38]

The patchset sent to the CQ was uploaded after l-g-t-m from vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1494223003/#ps160001
(title: "Fix some compilation issues in ui oops")

[commit-bot: I haz the power, 2015-12-09 23:38:06]

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

[commit-bot: I haz the power, 2015-12-10 03:59:51]

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

[commit-bot: I haz the power, 2015-12-10 03:59:52]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[enne (OOO), 2015-12-10 18:29:22]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2015-12-10 18:30:29]

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

[commit-bot: I haz the power, 2015-12-10 19:50:30]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-10 19:51:24]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2015-12-10 19:51:25]

Patchset 9 (id:??) landed as
https://crrev.com/3b006fb5a67c9e9bc0adf3f68d72a10afa221198
Cr-Commit-Position: refs/heads/master@{#364437}

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

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1521373005/ by enne@chromium.org.

The reason for reverting is: 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.