Back PaintRecord with PaintOpBuffer instead of SkPicture

Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#468548}
Committed: https://chromium.googlesource.com/chromium/src/+/59df29deed59592d42ff7e600f33faf035e00b46

[enne (OOO), 2017-03-22 21:53:46]

Description was changed from

==========
[wip] paintopbuffer + paintrecord
==========

to

==========
[wip] paintopbuffer + paintrecord
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-22 21:53:46]

Description was changed from

==========
[wip] paintopbuffer + paintrecord
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
[wip] paintopbuffer + paintrecord
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-03-23 18:07:38]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 18:08:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 18:14:33]

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

[commit-bot: I haz the power, 2017-03-23 18:14:34]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2017-03-23 18:26:41]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 18:27:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-23 18:43:36]

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

[commit-bot: I haz the power, 2017-03-23 18:43:37]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[enne (OOO), 2017-03-24 01:37:05]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 01:37:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 01:51:23]

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

[commit-bot: I haz the power, 2017-03-24 01:51:24]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[enne (OOO), 2017-03-24 18:39:19]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 18:39:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 18:55:41]

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

[commit-bot: I haz the power, 2017-03-24 18:55:42]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[enne (OOO), 2017-03-28 16:47:19]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 16:47:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 17:05:12]

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

[commit-bot: I haz the power, 2017-03-28 17:05:13]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[vmpstr, 2017-03-28 18:27:12]

vmpstr@chromium.org changed reviewers:
+ vmpstr@chromium.org

[vmpstr, 2017-03-28 18:27:15]

Just a quick run through, this is a lot of code to digest, but looks cool!

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:103: // TODO(enne): Maybe the PaintRecord itself
could know whether this
I think it makes sense for the paint record to figure this out; it might be as
simple as a bool that says "we need a wrapping save/restore on playback" and set
it to true on any transform/clip type of things.

The code as-is is good as well, but we might be doing extra save/restores when
we don't need to (not sure how expensive that is, but work is work)

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:183: static bool MergeAndDrawIfPossible(const
CompositingDisplayItem& save_item,
nit: Can you add a comment mentioning what this does (and what the return value
means)?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:194: if (record->approximateOpCount() != 1)
I'm a bit <_< at "approximate" here... Is this guaranteed to not underestimate?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:201: op->RasterWithAlpha(canvas, save_item.alpha);
Hmm is there a way to guard/dcheck against future changes? Like if we have some
other thing that isn't just alpha/color filter that pops up, is that going to
break things?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:222: i += 2;
I think this has to be ++i because of the ++i in the for loop? Or is my math bad

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:223: // TODO(enne): does this ever happen? Is this
worth catching?
I think this is possible in this situation:

- Compositing item
- Rect draw at 0x0 100,100
- Rect draw at 100,100 100x100
- End compositing item

Then the compositing item's visual rect would have to be 0x0 200x200 and if the
tile we're drawing is at 0,101 50x50 (which doesn't intersect any of the rects),
we'd still get the compositing/end compositing indices.

FWIW, the case we're dropping here is when i == indices.size() - 2; and we just
have the compositing/end compositing. I think that's easy enough to fix tho,
with like

if (type == COMPOSITING) {
 if (i < indices.size() - 1 && type[i+1] == END_COMPOSITING) {
   i++;
   continue;
 }
 ...
}
...

or... something <_<

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:232: i += 3;
+= 2?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_canvas.h
File cc/paint/paint_canvas.h (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_canvas....
cc/paint/paint_canvas.h:108: // TODO(enne): This is a synonym for drawColor with
kSrc.  Remove it.
Remember when it wasn't a synonym? Pepperidge farm remembers.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_flags.cc
File cc/paint/paint_flags.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_flags.c...
cc/paint/paint_flags.cc:9: bool PaintFlags::IsSimpleOpacity() const {
It might be worth adding a comment somewhere in PaintFlags that if we're adding
new things, this might need to be updated.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:94: static SkRect kUnsetRect;
Is this uninitialized or does static make it all 0?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:112: return op + 1;
Could use a comment here.. I'm not really sure why +1 :(

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:208: SkRect oval;
:D

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:606: PaintOp* operator*() const { return
operator->(); }
minor thing: This should probably return PaintOp&?

I'm just worried for consistency of

it->a vs (*it).a; As it stands, it's it->a and (*it)->a;

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorde...
File cc/paint/paint_recorder.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorde...
cc/paint/paint_recorder.cc:25: // TODO(enne): consider making a mini picture
here.
Do we still need this with the optimization in the paintbuffer?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:36: sk_sp<PaintRecord> finishRecordingAsPicture();
nit: probably WARN_UNUSED_RESULT, but maybe that's obvious

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.cc:113: int diff = canvas_.getSaveCount() -
save_count;
Probably DCHECK_GE(diff, 0)

[enne (OOO), 2017-03-28 19:28:06]

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:103: // TODO(enne): Maybe the PaintRecord itself
could know whether this
On 2017/03/28 at 18:27:14, vmpstr wrote:
> I think it makes sense for the paint record to figure this out; it might be as
simple as a bool that says "we need a wrapping save/restore on playback" and set
it to true on any transform/clip type of things.
> 
> The code as-is is good as well, but we might be doing extra save/restores when
we don't need to (not sure how expensive that is, but work is work)

I think this just needs some investigation.  I think the ideal situation would
be that PaintRecord doesn't do this at all and callers are responsible in all
cases for handling it.  I think we could track if we needed a save or restore
separately too (and then remove when all callers are doing it).

Other than just wanting to land this, the other reason I'm hesitant here is that
it seems really unfortunate to have to do add these in in a world where
cc::DisplayItemList collapses into PaintRecord.  Like, every single draw op
needs one of these around it?  I think it just kind of no longer makes sense
unless the caller handles it.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:183: static bool MergeAndDrawIfPossible(const
CompositingDisplayItem& save_item,
On 2017/03/28 at 18:27:14, vmpstr wrote:
> nit: Can you add a comment mentioning what this does (and what the return
value means)?

Done.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:194: if (record->approximateOpCount() != 1)
On 2017/03/28 at 18:27:14, vmpstr wrote:
> I'm a bit <_< at "approximate" here... Is this guaranteed to not
underestimate?

This is the SkPicture API.  This patch changes PaintRecord to not be an
SkPicture, but still has to implement playback, cullRect, approximateOpCount,
and approximateBytesUsed.

I'll leave a TODO.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:201: op->RasterWithAlpha(canvas, save_item.alpha);
On 2017/03/28 at 18:27:14, vmpstr wrote:
> Hmm is there a way to guard/dcheck against future changes? Like if we have
some other thing that isn't just alpha/color filter that pops up, is that going
to break things?

I think there's really no way to guard.  There's no way to guard right now for
the SkRecordOpts, and I think anything that changes SkPaint just needs to be
aware of those optimizations.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:222: i += 2;
On 2017/03/28 at 18:27:14, vmpstr wrote:
> I think this has to be ++i because of the ++i in the for loop? Or is my math
bad

OOPS

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:223: // TODO(enne): does this ever happen? Is this
worth catching?
On 2017/03/28 at 18:27:14, vmpstr wrote:
> I think this is possible in this situation:
> 
> - Compositing item
> - Rect draw at 0x0 100,100
> - Rect draw at 100,100 100x100
> - End compositing item
> 
> Then the compositing item's visual rect would have to be 0x0 200x200 and if
the tile we're drawing is at 0,101 50x50 (which doesn't intersect any of the
rects), we'd still get the compositing/end compositing indices.
> 
> FWIW, the case we're dropping here is when i == indices.size() - 2; and we
just have the compositing/end compositing. I think that's easy enough to fix
tho, with like
> 
> if (type == COMPOSITING) {
>  if (i < indices.size() - 1 && type[i+1] == END_COMPOSITING) {
>    i++;
>    continue;
>  }
>  ...
> }
> ...
> 
> or... something <_<

Yeah, I'm not saying it's not impossible.  It just certainly didn't come up in a
bunch of the layout tests that I ran, while the other case definitely did. 
Mostly just asking if this is worth bothering to check for or if it's so
uncommon that we should just let it happen.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/display_item_...
cc/paint/display_item_list.cc:232: i += 3;
On 2017/03/28 at 18:27:14, vmpstr wrote:
> += 2?

***<_<***

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_flags.cc
File cc/paint/paint_flags.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_flags.c...
cc/paint/paint_flags.cc:9: bool PaintFlags::IsSimpleOpacity() const {
On 2017/03/28 at 18:27:14, vmpstr wrote:
> It might be worth adding a comment somewhere in PaintFlags that if we're
adding new things, this might need to be updated.

This function is *on* PaintFlags.  It seems like if you're modifying the class,
you'll need to keep all of its member functions working properly, just by
default?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:94: static SkRect kUnsetRect;
On 2017/03/28 at 18:27:14, vmpstr wrote:
> Is this uninitialized or does static make it all 0?

It's initialized in the definition not in the declaration.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:112: return op + 1;
On 2017/03/28 at 18:27:14, vmpstr wrote:
> Could use a comment here.. I'm not really sure why +1 :(

Done.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:208: SkRect oval;
On 2017/03/28 at 18:27:15, vmpstr wrote:
> :D

That's how it is!

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:606: PaintOp* operator*() const { return
operator->(); }
On 2017/03/28 at 18:27:15, vmpstr wrote:
> minor thing: This should probably return PaintOp&?
> 
> I'm just worried for consistency of
> 
> it->a vs (*it).a; As it stands, it's it->a and (*it)->a;

You might want to fix up some of the tiling iterators then if you feel that way.

This is a little bit weird because of pointer math.  I think it only makes sense
to really operator on PaintOp*, because thing are packed in memory after that. 
We could certainly just take the address of a ref and use that, but that seems a
bit weirder.  At any rate, this means that operator* needs to return a PaintOp*
so that I can then do for (auto* op : Iterator(this)).  Then do I just remove
the -> operator entirely?  It's certainly convenient.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorde...
File cc/paint/paint_recorder.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorde...
cc/paint/paint_recorder.cc:25: // TODO(enne): consider making a mini picture
here.
On 2017/03/28 at 18:27:15, vmpstr wrote:
> Do we still need this with the optimization in the paintbuffer?

Yeah, it's possible.  I think I would consider making a SmallDrawingDisplayItem
and just copy the memory in there when needed.  This is sort of up in the air
with wanting to merge DisplayItemList and PaintOpBuffer.  If we do that, then no
need for mini pictures anywhere.

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorder.h
File cc/paint/paint_recorder.h (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/paint_recorde...
cc/paint/paint_recorder.h:36: sk_sp<PaintRecord> finishRecordingAsPicture();
On 2017/03/28 at 18:27:15, vmpstr wrote:
> nit: probably WARN_UNUSED_RESULT, but maybe that's obvious

Mmm, Canvas2DLayerBridge calls this and drops the PaintRecord on the ground
intentionally.  I think because this is returning a smart pointer, it's fine?

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.cc (right):

https://codereview.chromium.org/2768143002/diff/220001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.cc:113: int diff = canvas_.getSaveCount() -
save_count;
On 2017/03/28 at 18:27:15, vmpstr wrote:
> Probably DCHECK_GE(diff, 0)

Good thought!

[enne (OOO), 2017-03-29 17:17:58]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 17:18:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 17:36:38]

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

[commit-bot: I haz the power, 2017-03-29 17:36:39]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[enne (OOO), 2017-03-29 18:13:15]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 18:13:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 18:39:02]

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

[commit-bot: I haz the power, 2017-03-29 18:39:03]

Dry run: Try jobs failed on following builders:
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[enne (OOO), 2017-03-30 22:01:30]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 22:02:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 22:22:58]

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

[commit-bot: I haz the power, 2017-03-30 22:22:59]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2017-03-31 16:25:10]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 16:25:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 16:57:06]

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

[commit-bot: I haz the power, 2017-03-31 16:57:07]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[enne (OOO), 2017-04-03 21:59:18]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 22:00:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 23:04:24]

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

[commit-bot: I haz the power, 2017-04-03 23:04:25]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[enne (OOO), 2017-04-04 00:42:37]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 00:43:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-04 00:55:35]

Description was changed from

==========
[wip] paintopbuffer + paintrecord
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in cc/paint
instead of using SkPicture directly.  This new class cribs heavily from
SkLiteDL.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-04-04 00:55:35]

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

[commit-bot: I haz the power, 2017-04-04 01:13:22]

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

[commit-bot: I haz the power, 2017-04-04 01:13:24]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[enne (OOO), 2017-04-04 19:01:34]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-04 19:03:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-04 19:42:57]

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

[commit-bot: I haz the power, 2017-04-04 19:42:59]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mtklein_C, 2017-04-05 15:26:41]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2017-04-05 15:26:43]

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/display_item_...
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/display_item_...
cc/paint/display_item_list.cc:104: // was needed?  It's not clear whether these
save/restore semantics
If you don't save and restore, you may want to at least getSaveCount() and
restoreToCount() when done, or getSaveCount() and assert that you don't need to
restoreToCount() when done.  Things get gross if you don't balance your save()s,
especially saveLayer()s where the restore() makes drawing happen.

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:171: !base::is_trivially_destructible<T>::value     
     \
In retrospect, I'd bet this doesn't matter.  It's probably harmless to just call
all destructors if you want to cut a little complexity.

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:76: uint32_t type : 8;
Looks familiar. :)

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:522: // TODO(enne): Find some cleaner way to do this?
There's always, don't call setMatrix().  :)

Kidding aside, this is pretty unavoidable.  I hate it too.

[danakj, 2017-04-05 15:31:22]

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:522: // TODO(enne): Find some cleaner way to do this?
On 2017/04/05 15:26:43, mtklein_C wrote:
> There's always, don't call setMatrix().  :)
> 
> Kidding aside, this is pretty unavoidable.  I hate it too.

What about PaintCanvas::Transform and it uses the current matrix and concats the
new one, instead of "set" which overrides?

[mtklein_C, 2017-04-05 15:34:24]

On 2017/04/05 at 15:31:22, danakj wrote:
>
https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
> File cc/paint/paint_op_buffer.h (right):
> 
>
https://codereview.chromium.org/2768143002/diff/440001/cc/paint/paint_op_buff...
> cc/paint/paint_op_buffer.h:522: // TODO(enne): Find some cleaner way to do
this?
> On 2017/04/05 15:26:43, mtklein_C wrote:
> > There's always, don't call setMatrix().  :)
> > 
> > Kidding aside, this is pretty unavoidable.  I hate it too.
> 
> What about PaintCanvas::Transform and it uses the current matrix and concats
the new one, instead of "set" which overrides?

Yep, if you convert setMatrix() calls into concat(), your job's done.

[enne (OOO), 2017-04-06 00:25:22]

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/display_item_...
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2768143002/diff/440001/cc/paint/display_item_...
cc/paint/display_item_list.cc:104: // was needed?  It's not clear whether these
save/restore semantics
On 2017/04/05 at 15:26:43, mtklein_C wrote:
> If you don't save and restore, you may want to at least getSaveCount() and
restoreToCount() when done, or getSaveCount() and assert that you don't need to
restoreToCount() when done.  Things get gross if you don't balance your save()s,
especially saveLayer()s where the restore() makes drawing happen.

That's fair.  The recorder does restoreToCount for sure, and a lot yelled when
it didn't need to.

[enne (OOO), 2017-04-06 00:43:08]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in cc/paint
instead of using SkPicture directly.  This new class cribs heavily from
SkLiteDL.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that it was fine.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-04-06 00:54:51]

On 2017/04/06 at 00:25:22, enne wrote:
>
https://codereview.chromium.org/2768143002/diff/440001/cc/paint/display_item_...
> File cc/paint/display_item_list.cc (right):
> 
>
https://codereview.chromium.org/2768143002/diff/440001/cc/paint/display_item_...
> cc/paint/display_item_list.cc:104: // was needed?  It's not clear whether
these save/restore semantics
> On 2017/04/05 at 15:26:43, mtklein_C wrote:
> > If you don't save and restore, you may want to at least getSaveCount() and
restoreToCount() when done, or getSaveCount() and assert that you don't need to
restoreToCount() when done.  Things get gross if you don't balance your save()s,
especially saveLayer()s where the restore() makes drawing happen.
> 
> That's fair.  The recorder does restoreToCount for sure, and a lot yelled when
it didn't need to.

Er, a lot yelled when it didn't restore, I mean.

[enne (OOO), 2017-04-06 01:02:17]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 01:03:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-06 01:03:50]

Ok, I think this is ready for a more serious look and then landing.  I think
I've addressed all the test issues I found.

There's ~10 layout tests failing because of differences in opacity
optimizations, but they're all off-by-one and I'm going to rebase (or turn
reftests into pixel tests and then rebase).

https://codereview.chromium.org/2768143002/diff/480001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp
(right):

https://codereview.chromium.org/2768143002/diff/480001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebScrollbarThemePainter.cpp:37: //
WebScrollbarThemeMac uses GraphicsContextCanvas which doesn't quite support
These changes here and in the WebScrollbarThemePainter are from
https://chromium-review.googlesource.com/c/469073/, and included for try jobs,
so I can rebase a few layout tests.

[mtklein_C, 2017-04-06 01:11:07]

(Didn't mean to put myself on R=.  But, 👍 👍 👍 👍 👍.  I love this, I support this,
please let me know if I can assist.)

[mtklein_C, 2017-04-06 01:12:12]

On 2017/04/06 at 01:11:31, mtklein_C wrote:
> 

Ok I clearly don't remember how to Reitveld.  I'll just go away and be quiet
now... please ignore my presence in the reviewers list.

[enne (OOO), 2017-04-06 01:12:40]

enne@chromium.org changed reviewers:
- mtklein@chromium.org

[commit-bot: I haz the power, 2017-04-06 01:41:50]

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

[commit-bot: I haz the power, 2017-04-06 01:41:51]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[enne (OOO), 2017-04-06 02:31:57]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 02:32:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-06 02:34:51]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that it was fine.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-04-06 03:13:54]

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

[commit-bot: I haz the power, 2017-04-06 03:13:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[vmpstr, 2017-04-06 19:16:13]

(mostly nits)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
cc/paint/display_item_list.cc:195: // TODO(enne): I believe that
lcd_text_requires_opaque_layer is not
Is there a way to test this assumption via unittests/pixel tests? Or are we
punting?

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
cc/paint/display_item_list.cc:199: if (record->approximateOpCount() != 1)
Can you please leave a comment here that approximate here is good enough,
because it's not really approximate? Every time I look at this I'm thinking too
much about whether this can break...

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
cc/paint/display_item_list.cc:248: if (callback && callback->abort())
nit: can you move this check to the top of the loop? You could technically end
up with composite/draw/endcomposite several times in a row, so you wouldn't be
checking the abort

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:407: : annotation_type(annotation_type), rect(rect),
data(data) {}
std::move(data)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:429: : image(image), src(src), dst(dst),
constraint(constraint) {
std::move(image)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:443: DrawRecordOp::DrawRecordOp(sk_sp<const
PaintRecord> record) : record(record) {}
std::move(record)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:455: : blob(blob), x(x), y(y), flags(flags) {}
std::move(blob)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:494: const PaintOp* second = iter.peek1();
future thought: maybe peek can return an iterator, then below instead of peek2,
you can just peek on the iterator returned by this peek; and instead of ++iters,
you can iter = the_iter_returned_by_peek instead

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:27: ThreadsafeMatrix() : SkMatrix() {
(void)getType(); }
I think techincally this SkMatrix is not initialized to anything, so it will be
using that to determine the type :P Probably OK?

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:88: size_t AdditionalBytesUsed() const { return 0; }
Comment what "Additional" means.. like the data stuff?

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:149: return SkTAddOffset<const M>(op + 1, op->bytes);
|bytes| here is the size of "data" here right? (Just confirming that I'm
understanding this correctly)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:465: struct NoopOp final : PaintOp {
:D

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:524: // This is the only op that needs the original
matrix.
Can you comment on what original means here? Does this also mean that this op is
kind of uncacheable (since adding/removing items before this one affects this
one?)

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:540: using LargestPaintOp = DrawDRRectOp;
Is there a way to make this future proof?

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:555: // TODO(enne): These are no longer approximate. 
Rename these.
Cool! You can then ignore my previous comment about adding a comment

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:691: const PaintOpBuffer* buffer_;
= nullptr for consistency?

[mtklein_C, 2017-04-06 19:58:47]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2017-04-06 19:58:48]

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:27: ThreadsafeMatrix() : SkMatrix() {
(void)getType(); }
On 2017/04/06 at 19:16:12, vmpstr wrote:
> I think techincally this SkMatrix is not initialized to anything, so it will
be using that to determine the type :P Probably OK?

It's probably fine, but MSAN might hate you.  Might be simpler to default to
SkMatrix::I().

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:524: // This is the only op that needs the original
matrix.
On 2017/04/06 at 19:16:12, vmpstr wrote:
> Can you comment on what original means here? Does this also mean that this op
is kind of uncacheable (since adding/removing items before this one affects this
one?)

When you record a setMatrix(), you're doing it in the context of the space the
recording started in (think, identity matrix).

When you play back a setMatrix(), in order to get the same effect as setMatrix()
in that record-time identity space, you need to take the matrix you recorded and
concatenate it with the matrix of the destination canvas at the time you started
playing back the recording.

So, let's say you recorded a setMatrix() to [1 0 20, 0 1 20, 0 0 1]  (identity
translated +20,+20).  Let's say the canvas you're playing back to started
translated +10,+10, i.e. [1 0 10, 0 1 10, 0 0 1].  You want to end up with a
matrix [1 0 30, 0 1 30, 0 0 1] after you play back the setMatrix(), not [1 0 20,
0 1 20, 0 0 1].

[vmpstr, 2017-04-06 20:37:47]

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:524: // This is the only op that needs the original
matrix.
On 2017/04/06 19:58:48, mtklein_C wrote:
> On 2017/04/06 at 19:16:12, vmpstr wrote:
> > Can you comment on what original means here? Does this also mean that this
op
> is kind of uncacheable (since adding/removing items before this one affects
this
> one?)
> 
> When you record a setMatrix(), you're doing it in the context of the space the
> recording started in (think, identity matrix).
> 
> When you play back a setMatrix(), in order to get the same effect as
setMatrix()
> in that record-time identity space, you need to take the matrix you recorded
and
> concatenate it with the matrix of the destination canvas at the time you
started
> playing back the recording.
> 
> So, let's say you recorded a setMatrix() to [1 0 20, 0 1 20, 0 0 1]  (identity
> translated +20,+20).  Let's say the canvas you're playing back to started
> translated +10,+10, i.e. [1 0 10, 0 1 10, 0 0 1].  You want to end up with a
> matrix [1 0 30, 0 1 30, 0 0 1] after you play back the setMatrix(), not [1 0
20,
> 0 1 20, 0 0 1].

Hmm ok. I guess my question is then if we try and modify a previous item that
translated it to +10, +10 in your example, then that means it affects future
items like this set matrix. I don't think it matters for const records as it is
now, but it might affect any plans we might have for making this modifiable
(although I think we might run into other issues if we try that)

[vmpstr, 2017-04-06 20:40:50]

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:524: // This is the only op that needs the original
matrix.
On 2017/04/06 20:37:47, vmpstr wrote:
> On 2017/04/06 19:58:48, mtklein_C wrote:
> > On 2017/04/06 at 19:16:12, vmpstr wrote:
> > > Can you comment on what original means here? Does this also mean that this
> op
> > is kind of uncacheable (since adding/removing items before this one affects
> this
> > one?)
> > 
> > When you record a setMatrix(), you're doing it in the context of the space
the
> > recording started in (think, identity matrix).
> > 
> > When you play back a setMatrix(), in order to get the same effect as
> setMatrix()
> > in that record-time identity space, you need to take the matrix you recorded
> and
> > concatenate it with the matrix of the destination canvas at the time you
> started
> > playing back the recording.
> > 
> > So, let's say you recorded a setMatrix() to [1 0 20, 0 1 20, 0 0 1] 
(identity
> > translated +20,+20).  Let's say the canvas you're playing back to started
> > translated +10,+10, i.e. [1 0 10, 0 1 10, 0 0 1].  You want to end up with a
> > matrix [1 0 30, 0 1 30, 0 0 1] after you play back the setMatrix(), not [1 0
> 20,
> > 0 1 20, 0 0 1].
> 
> Hmm ok. I guess my question is then if we try and modify a previous item that
> translated it to +10, +10 in your example, then that means it affects future
> items like this set matrix. I don't think it matters for const records as it
is
> now, but it might affect any plans we might have for making this modifiable
> (although I think we might run into other issues if we try that)

Oh sorry, I completely misunderstood that this is at raster, not at record time.
Disregard me :P

[mtklein_C, 2017-04-06 20:44:17]

On 2017/04/06 at 20:37:47, vmpstr wrote:
>
https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
> File cc/paint/paint_op_buffer.h (right):
> 
>
https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
> cc/paint/paint_op_buffer.h:524: // This is the only op that needs the original
matrix.
> On 2017/04/06 19:58:48, mtklein_C wrote:
> > On 2017/04/06 at 19:16:12, vmpstr wrote:
> > > Can you comment on what original means here? Does this also mean that this
op
> > is kind of uncacheable (since adding/removing items before this one affects
this
> > one?)
> > 
> > When you record a setMatrix(), you're doing it in the context of the space
the
> > recording started in (think, identity matrix).
> > 
> > When you play back a setMatrix(), in order to get the same effect as
setMatrix()
> > in that record-time identity space, you need to take the matrix you recorded
and
> > concatenate it with the matrix of the destination canvas at the time you
started
> > playing back the recording.
> > 
> > So, let's say you recorded a setMatrix() to [1 0 20, 0 1 20, 0 0 1] 
(identity
> > translated +20,+20).  Let's say the canvas you're playing back to started
> > translated +10,+10, i.e. [1 0 10, 0 1 10, 0 0 1].  You want to end up with a
> > matrix [1 0 30, 0 1 30, 0 0 1] after you play back the setMatrix(), not [1 0
20,
> > 0 1 20, 0 0 1].
> 
> Hmm ok. I guess my question is then if we try and modify a previous item that
translated it to +10, +10 in your example, then that means it affects future
items like this set matrix. I don't think it matters for const records as it is
now, but it might affect any plans we might have for making this modifiable
(although I think we might run into other issues if we try that)

Are you asking something like,
   Op 1    Save
   ...
   Op 20   Translate +10,+10
   ...
   Op 40   SetMatrix ...
   ...
   Op 50   Restore

Does modifying Op 20 mean you need to change Op 40?  Not necessarily, but it
depends on what you're doing whether that makes any sense.  SetMatrix by design
is oblivious of all the matrix changes that come before it.  If you want to
change Translate +10,+10 to +15,+15, SetMatrix could make sense for some use
cases to stay unchanged, and could make sense in some use cases to want to get a
+5,+5.  But, in all cases, you do need to think of Op 40's SetMatrix as being
relative to the original matrix, i.e. the current matrix at the time of the Op 1
Save (== the same matrix after the Op 50 Restore).

[vmpstr, 2017-04-06 20:46:24]

On 2017/04/06 20:44:17, mtklein_C wrote:
> On 2017/04/06 at 20:37:47, vmpstr wrote:
> >
>
https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
> > File cc/paint/paint_op_buffer.h (right):
> > 
> >
>
https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
> > cc/paint/paint_op_buffer.h:524: // This is the only op that needs the
original
> matrix.
> > On 2017/04/06 19:58:48, mtklein_C wrote:
> > > On 2017/04/06 at 19:16:12, vmpstr wrote:
> > > > Can you comment on what original means here? Does this also mean that
this
> op
> > > is kind of uncacheable (since adding/removing items before this one
affects
> this
> > > one?)
> > > 
> > > When you record a setMatrix(), you're doing it in the context of the space
> the
> > > recording started in (think, identity matrix).
> > > 
> > > When you play back a setMatrix(), in order to get the same effect as
> setMatrix()
> > > in that record-time identity space, you need to take the matrix you
recorded
> and
> > > concatenate it with the matrix of the destination canvas at the time you
> started
> > > playing back the recording.
> > > 
> > > So, let's say you recorded a setMatrix() to [1 0 20, 0 1 20, 0 0 1] 
> (identity
> > > translated +20,+20).  Let's say the canvas you're playing back to started
> > > translated +10,+10, i.e. [1 0 10, 0 1 10, 0 0 1].  You want to end up with
a
> > > matrix [1 0 30, 0 1 30, 0 0 1] after you play back the setMatrix(), not [1
0
> 20,
> > > 0 1 20, 0 0 1].
> > 
> > Hmm ok. I guess my question is then if we try and modify a previous item
that
> translated it to +10, +10 in your example, then that means it affects future
> items like this set matrix. I don't think it matters for const records as it
is
> now, but it might affect any plans we might have for making this modifiable
> (although I think we might run into other issues if we try that)
> 
> Are you asking something like,
>    Op 1    Save
>    ...
>    Op 20   Translate +10,+10
>    ...
>    Op 40   SetMatrix ...
>    ...
>    Op 50   Restore
> 
> Does modifying Op 20 mean you need to change Op 40?  Not necessarily, but it
> depends on what you're doing whether that makes any sense.  SetMatrix by
design
> is oblivious of all the matrix changes that come before it.  If you want to
> change Translate +10,+10 to +15,+15, SetMatrix could make sense for some use
> cases to stay unchanged, and could make sense in some use cases to want to get
a
> +5,+5.  But, in all cases, you do need to think of Op 40's SetMatrix as being
> relative to the original matrix, i.e. the current matrix at the time of the Op
1
> Save (== the same matrix after the Op 50 Restore).

Yeah that's what I was asking. I erroneously thought that this ctm is the
current recording ctm that affects things. Enne clarified that this is at
raster, which means my example works regardless of how you change previous
items. Thanks for the explanation!

[enne (OOO), 2017-04-06 21:30:34]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-06 21:31:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-06 22:55:06]

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

[commit-bot: I haz the power, 2017-04-06 22:55:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2017-04-06 23:17:05]

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
File cc/paint/display_item_list.cc (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
cc/paint/display_item_list.cc:195: // TODO(enne): I believe that
lcd_text_requires_opaque_layer is not
On 2017/04/06 at 19:16:12, vmpstr wrote:
> Is there a way to test this assumption via unittests/pixel tests? Or are we
punting?

I am punting!

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/display_item_...
cc/paint/display_item_list.cc:199: if (record->approximateOpCount() != 1)
On 2017/04/06 at 19:16:12, vmpstr wrote:
> Can you please leave a comment here that approximate here is good enough,
because it's not really approximate? Every time I look at this I'm thinking too
much about whether this can break...

Sorry about this.  I was just trying to not change the interface of
SkPicture/PaintRecord here as I made this change.  I will follow up with this
right after.

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:494: const PaintOp* second = iter.peek1();
On 2017/04/06 at 19:16:12, vmpstr wrote:
> future thought: maybe peek can return an iterator, then below instead of
peek2, you can just peek on the iterator returned by this peek; and instead of
++iters, you can iter = the_iter_returned_by_peek instead

If it's ever the case that n > 2, I'll change this to return an iterator.

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:27: ThreadsafeMatrix() : SkMatrix() {
(void)getType(); }
On 2017/04/06 at 19:58:48, mtklein_C wrote:
> On 2017/04/06 at 19:16:12, vmpstr wrote:
> > I think techincally this SkMatrix is not initialized to anything, so it will
be using that to determine the type :P Probably OK?
> 
> It's probably fine, but MSAN might hate you.  Might be simpler to default to
SkMatrix::I().

Apparently I don't need this constructor!  Problem solved.  <_<

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:88: size_t AdditionalBytesUsed() const { return 0; }
On 2017/04/06 at 19:16:12, vmpstr wrote:
> Comment what "Additional" means.. like the data stuff?

Like referenced sub-records and display lists.

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:149: return SkTAddOffset<const M>(op + 1, op->bytes);
On 2017/04/06 at 19:16:12, vmpstr wrote:
> |bytes| here is the size of "data" here right? (Just confirming that I'm
understanding this correctly)

Correct.

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:465: struct NoopOp final : PaintOp {
On 2017/04/06 at 19:16:12, vmpstr wrote:
> :D

I know, I know.  NoOp just looked weird as an alternative, and even then is its
type just "No"??

Sorry :C

https://codereview.chromium.org/2768143002/diff/500001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:540: using LargestPaintOp = DrawDRRectOp;
On 2017/04/06 at 19:16:12, vmpstr wrote:
> Is there a way to make this future proof?

There's static asserts that tell you which op is bigger if you break this.  Is
that future proof enough for you?

https://codereview.chromium.org/2768143002/diff/520001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.cc (right):

https://codereview.chromium.org/2768143002/diff/520001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.cc:31:
canvas_.clipRect(SkRect::Make(cull_rect.roundOut()), SkClipOp::kIntersect,
Fixed the mac layout test issues with this.

If https://skia-review.googlesource.com/c/11320/ lands, then we can just use
resetForNextPicture on SkNoDrawCanvas, but I think this should work in practice.

[mtklein_C, 2017-04-06 23:22:18]

https://codereview.chromium.org/2768143002/diff/520001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.cc (right):

https://codereview.chromium.org/2768143002/diff/520001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.cc:31:
canvas_.clipRect(SkRect::Make(cull_rect.roundOut()), SkClipOp::kIntersect,
On 2017/04/06 at 23:17:05, enne wrote:
> Fixed the mac layout test issues with this.
> 
> If https://skia-review.googlesource.com/c/11320/ lands, then we can just use
resetForNextPicture on SkNoDrawCanvas, but I think this should work in practice.

Yeah, this actually doesn't seem too crazy.  I think you'll get the same effect
as calling resetForNextPicture() with this.  (resetForNextPicture() is a bit
more powerful later in a canvas' life, but not inside the constructor.)

"but why" indeed.

[enne (OOO), 2017-04-07 00:33:41]

On 2017/04/06 at 23:22:18, mtklein wrote:
>
https://codereview.chromium.org/2768143002/diff/520001/cc/paint/record_paint_...
> File cc/paint/record_paint_canvas.cc (right):
> 
>
https://codereview.chromium.org/2768143002/diff/520001/cc/paint/record_paint_...
> cc/paint/record_paint_canvas.cc:31:
canvas_.clipRect(SkRect::Make(cull_rect.roundOut()), SkClipOp::kIntersect,
> On 2017/04/06 at 23:17:05, enne wrote:
> > Fixed the mac layout test issues with this.
> > 
> > If https://skia-review.googlesource.com/c/11320/ lands, then we can just use
resetForNextPicture on SkNoDrawCanvas, but I think this should work in practice.
> 
> Yeah, this actually doesn't seem too crazy.  I think you'll get the same
effect as calling resetForNextPicture() with this.  (resetForNextPicture() is a
bit more powerful later in a canvas' life, but not inside the constructor.)
> 
> "but why" indeed.

<_<

I don't really understand the need for size on a recording canvas, because most
of the time it doesn't seem relevant to the process of recording, so far as I
can tell.  It seems like they're mostly metadata for some other caller in the
stack to then use to size a bitmap to play the recording back into.  And, in
this case, it's initializing the device clip that some other code is depending
on and so it can't be wrong (but it could be empty, alternatively).

[mtklein_C, 2017-04-07 00:41:58]

> I don't really understand the need for size on a recording canvas, because
most of the time it doesn't seem relevant to the process of recording, so far as
I can tell.  It seems like they're mostly metadata for some other caller in the
stack to then use to size a bitmap to play the recording back into.  And, in
this case, it's initializing the device clip that some other code is depending
on and so it can't be wrong (but it could be empty, alternatively).

I agree completely.  There isn't logically any need for a size when recording if
you treat the thing you're recording into as write-only.

It's only for queries like getLocalClipBounds(), getDeviceClipBounds(), or
getTotalMatrix() that we need a size, or more generally that we need to inherit
from SkCanvas at all.

If you just force the clip to {0,0,0,0}, you'll record and draw almost
everything correctly, because almost everything is write-only.  But some things
do query the canvas and they'll get confused.

[chrishtr, 2017-04-07 00:43:48]

Looks pretty good in third_party/WebKit.

https://codereview.chromium.org/2768143002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp
(left):

https://codereview.chromium.org/2768143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:32: //
If the end of the current display list is of the form
Could you point me at where this heuristic is re-implemented?
Is there a unittest for the new behavior?

[enne (OOO), 2017-04-07 00:54:06]

https://codereview.chromium.org/2768143002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp
(left):

https://codereview.chromium.org/2768143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:32: //
If the end of the current display list is of the form
On 2017/04/07 at 00:43:47, chrishtr wrote:
> Could you point me at where this heuristic is re-implemented?
> Is there a unittest for the new behavior?

There's not a unit test, but there's PLENTY of layout test and cc pixel test
coverage.  I'm happy to add a directed unit test in this patch if you'd like.

This optimization is replicated in both DisplayItemList::Raster and
PaintOpBuffer::playback (since a save could be either a DisplayItem or a
PaintOp).

[chrishtr, 2017-04-07 16:10:28]

https://codereview.chromium.org/2768143002/diff/520001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp
(left):

https://codereview.chromium.org/2768143002/diff/520001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/paint/CompositingRecorder.cpp:32: //
If the end of the current display list is of the form
On 2017/04/07 at 00:54:06, enne wrote:
> On 2017/04/07 at 00:43:47, chrishtr wrote:
> > Could you point me at where this heuristic is re-implemented?
> > Is there a unittest for the new behavior?
> 
> There's not a unit test, but there's PLENTY of layout test and cc pixel test
coverage.  I'm happy to add a directed unit test in this patch if you'd like.

Yes please.

> 
> This optimization is replicated in both DisplayItemList::Raster and
PaintOpBuffer::playback (since a save could be either a DisplayItem or a
PaintOp).

Ah great. Very nice.

[enne (OOO), 2017-04-07 22:02:26]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 22:03:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 22:14:08]

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

[commit-bot: I haz the power, 2017-04-07 22:14:10]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_layout_tests_slimming_paint_v2 on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_layout_te...)

[enne (OOO), 2017-04-07 22:32:44]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 22:33:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 23:12:44]

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

[commit-bot: I haz the power, 2017-04-07 23:12:45]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[enne (OOO), 2017-04-09 19:39:32]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-09 19:39:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-09 19:55:52]

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

[commit-bot: I haz the power, 2017-04-09 19:55:53]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2017-04-10 22:19:44]

Added some unit tests for that optimization.  Rebaselined the layout tests.

The only open issue at the moment is that this uses more memory than the
previous path.  It could just be accounting wrong.  I'm going to look into it
and see if there's an easy solution.

[enne (OOO), 2017-04-11 00:37:15]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 00:37:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 01:28:47]

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

[commit-bot: I haz the power, 2017-04-11 01:28:49]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[enne (OOO), 2017-04-11 16:58:33]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:59:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 19:10:11]

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

[commit-bot: I haz the power, 2017-04-11 19:10:13]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2017-04-11 20:09:41]

Looks good to me, modulo your comment already about memory use.

[enne (OOO), 2017-04-12 17:00:56]

Latest patch resizes the PaintOpBuffer when recording is done.  That gives a 10%
memory loss over the previous version which seems good enough for landing.  That
resize doesn't appear to affect recording perf.

[chrishtr, 2017-04-12 17:02:10]

lgtm

LGTM for third_party/WebKit.

[enne (OOO), 2017-04-12 17:31:08]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-12 17:31:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-12 17:32:40]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

TBR=thakis@chromium.org,vitalybuka@chromium.org

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-04-12 17:33:31]

TBRing thakis (ui) and vitalybuka (printing) for one line #include additions

[commit-bot: I haz the power, 2017-04-12 21:30:43]

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

[commit-bot: I haz the power, 2017-04-12 21:30:44]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-04-12 21:47:12]

mostly nits

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
File cc/paint/display_item_list_unittest.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
cc/paint/display_item_list_unittest.cc:762: EXPECT_EQ(0, canvas.restore_count_);
This is a bit interesting for me. Specifically, for the case of exactly two
items (Compositing/EndCompositing), the if on line 230 in
https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
fails, because i == 0 which is not < 2 - 2. So, we should be sending a save
layer and a restore?

Am I missing something or is skia optimizing this situation under the covers?

I see the optimization in a single op buffer and that would handle this no
problem, but afaict this test is supposed to test the DisplayItemList::Raster
code path.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:139: #define M(T) 1 +
Magic!

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:143: typedef void (*RasterFunction)(const PaintOp*
op,
using RasterFunction = ...

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:154: typedef void (*RasterAlphaFunction)(const
PaintOp* op,
using

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:168: typedef void (*VoidFunction)(PaintOp* op);
using

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:172: : static_cast<VoidFunction>(nullptr),
Maybe this can be [](PaintOp* op) {} and not have a check for validity when
invoking?

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:390: if (paintStyle == PaintFlags::kStroke_Style &&
nit: this is kind of a weird construct. Why this instead of a super complicated
if? <_<

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:430: if (flags)
You have this pattern a few places. I don't really mind, but is this better than

flags(flags ? *flags : PaintFlags()) in the ctor?

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:536: // can remove the entire notion of an abort
callback.
+1

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:544: void PaintOpBuffer::Compact() {
Maybe ShrinkToFit (it's kind of a common name I think)

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:568: void Compact();
nit: comment

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:724: // Internal first_op buffer doesn't have room
for extra data.
Can you fit another comment maybe higher up (or maybe around first_op_
declaration) that explains that this is an optimization for a common case of a
single item

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.h:46: void restoreToCount(int save_count) override;
Is there a plan to also rename these to chromium style? (RestoreToCount for
example?)

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas.h
File cc/paint/test_skcanvas.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
cc/paint/test_skcanvas.h:1: // Copyright 2017 The Chromium Authors. All rights
reserved.
nit: Should this be in cc/test?

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
cc/paint/test_skcanvas.h:17: // Note: willSave() does not appear to be called
from SkCanvas on a
There's a getSaveLayerStrategy() that gets called once for that (willSave will
be called for regular saves).

[mtklein_C, 2017-04-12 22:32:00]

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
File cc/paint/display_item_list_unittest.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
cc/paint/display_item_list_unittest.cc:762: EXPECT_EQ(0, canvas.restore_count_);
> Am I missing something or is skia optimizing this situation under the covers?

For what it's worth, SkCanvas does internally delay save()s until they're
needed.  E.g.

 save()
   drawRect()
   translate()
   drawRect()
 restore()

does become

 drawRect()
 save()
   translate()
   drawRect()
 restore()

and of course save()-restore() annihilates without doing anything.

saveLayer() is a different story, and I don't think there's any sort of
trickiness going on in there.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:139: #define M(T) 1 +
On 2017/04/12 at 21:47:12, vmpstr wrote:
> Magic!

Sure, but if you want real magic, write this as +1.  That way you don't need the
0 after TYPES(M).

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:430: if (flags)
On 2017/04/12 at 21:47:12, vmpstr wrote:
> You have this pattern a few places. I don't really mind, but is this better
than
> 
> flags(flags ? *flags : PaintFlags()) in the ctor?

(No, it's probably better to put it in the initializer list if all the others
are there.  This style is just how we happened to fill in all the fields
originally in SkLiteDL...)

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas.h
File cc/paint/test_skcanvas.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
cc/paint/test_skcanvas.h:17: // Note: willSave() does not appear to be called
from SkCanvas on a
On 2017/04/12 at 21:47:12, vmpstr wrote:
> There's a getSaveLayerStrategy() that gets called once for that (willSave will
be called for regular saves).

Right.  save() != saveLayer().  The virtual hook for saveLayer() is
getSaveLayerStrategy().

[Vitaly Buka (NO REVIEWS), 2017-04-12 22:39:26]

vitalybuka@chromium.org changed reviewers:
+ vitalybuka@chromium.org

[Vitaly Buka (NO REVIEWS), 2017-04-12 22:39:28]

printing/ LGTM

[enne (OOO), 2017-04-12 22:41:31]

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
File cc/paint/display_item_list_unittest.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
cc/paint/display_item_list_unittest.cc:762: EXPECT_EQ(0, canvas.restore_count_);
On 2017/04/12 at 21:47:12, vmpstr wrote:
> This is a bit interesting for me. Specifically, for the case of exactly two
items (Compositing/EndCompositing), the if on line 230 in
https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
fails, because i == 0 which is not < 2 - 2. So, we should be sending a save
layer and a restore?
> 
> Am I missing something or is skia optimizing this situation under the covers?
> 
> I see the optimization in a single op buffer and that would handle this no
problem, but afaict this test is supposed to test the DisplayItemList::Raster
code path.

OOPS.  The unit test appears to pass without this, so am removing this, and
leaving a comment on the unit test.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/display_item_...
cc/paint/display_item_list_unittest.cc:762: EXPECT_EQ(0, canvas.restore_count_);
On 2017/04/12 at 22:32:00, mtklein_C wrote:
> > Am I missing something or is skia optimizing this situation under the
covers?
> 
> For what it's worth, SkCanvas does internally delay save()s until they're
needed.  E.g.
> 
>  save()
>    drawRect()
>    translate()
>    drawRect()
>  restore()
> 
> does become
> 
>  drawRect()
>  save()
>    translate()
>    drawRect()
>  restore()
> 
> and of course save()-restore() annihilates without doing anything.
> 
> saveLayer() is a different story, and I don't think there's any sort of
trickiness going on in there.

Skia does appear to optimize out noop saveLayer restores, so far as I can tell.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:139: #define M(T) 1 +
On 2017/04/12 at 22:32:00, mtklein_C wrote:
> On 2017/04/12 at 21:47:12, vmpstr wrote:
> > Magic!
> 
> Sure, but if you want real magic, write this as +1.  That way you don't need
the 0 after TYPES(M).

Oh unary plus, I always forget you.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:172: : static_cast<VoidFunction>(nullptr),
On 2017/04/12 at 21:47:12, vmpstr wrote:
> Maybe this can be [](PaintOp* op) {} and not have a check for validity when
invoking?

Left a TODO to evaluate the performance of this at all.  This is cribbing from
SkLiteDL, and maybe we don't even need to check is_trivially_destructible and
can just call the dtor on everything and it'll be just as good.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:390: if (paintStyle == PaintFlags::kStroke_Style &&
On 2017/04/12 at 21:47:12, vmpstr wrote:
> nit: this is kind of a weird construct. Why this instead of a super
complicated if? <_<

This comes from SkPathCounter.  I think there's three kinds of paths, one that's
not optimized and is "slow" and the reason the other two aren't counted as slow
is for different reasons.

I personally don't like super complicated ifs, because they're hard to read. 
Also, when there are actual different cases, having separate lines for
conditionals make it possible for a debugger or printfs to differentiate these
cases more easily without changing the final code output.

I added some more early returns here, and made this more readable.  If you have
a more specific suggestion that you think is more readable, I'm happy to change
this to whatever.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:430: if (flags)
On 2017/04/12 at 21:47:12, vmpstr wrote:
> You have this pattern a few places. I don't really mind, but is this better
than
> 
> flags(flags ? *flags : PaintFlags()) in the ctor?

Sure, can do.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.h:46: void restoreToCount(int save_count) override;
On 2017/04/12 at 21:47:12, vmpstr wrote:
> Is there a plan to also rename these to chromium style? (RestoreToCount for
example?)

My plan was to let this stick for a while (maybe at least until a branch) in
case we needed a last minute revert for some reason.  And then think about
changing style.  It's a pain to do, so I'd like to make sure things stick before
churning all the code.

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas.h
File cc/paint/test_skcanvas.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
cc/paint/test_skcanvas.h:1: // Copyright 2017 The Chromium Authors. All rights
reserved.
On 2017/04/12 at 21:47:12, vmpstr wrote:
> nit: Should this be in cc/test?

If anything outside of cc/paint uses it, sure? Do you think it should move now?

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
cc/paint/test_skcanvas.h:17: // Note: willSave() does not appear to be called
from SkCanvas on a
On 2017/04/12 at 21:47:12, vmpstr wrote:
> There's a getSaveLayerStrategy() that gets called once for that (willSave will
be called for regular saves).

Ah thanks, I didn't quite see that.  I added this in and added a save_count
variable as well.  I was mostly just surprised that willRestore got called but
willSave didn't.

[mtklein_C, 2017-04-12 23:17:06]

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.cc (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.cc:172: : static_cast<VoidFunction>(nullptr),
On 2017/04/12 at 22:41:30, enne wrote:
> On 2017/04/12 at 21:47:12, vmpstr wrote:
> > Maybe this can be [](PaintOp* op) {} and not have a check for validity when
invoking?
> 
> Left a TODO to evaluate the performance of this at all.  This is cribbing from
SkLiteDL, and maybe we don't even need to check is_trivially_destructible and
can just call the dtor on everything and it'll be just as good.

I have been thinking about doing the same for SkLiteDL itself.  It's certainly
correct to just always call the destructors.

[enne (OOO), 2017-04-13 00:35:29]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 00:37:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 02:37:19]

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

[commit-bot: I haz the power, 2017-04-13 02:37:21]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nico, 2017-04-13 15:43:04]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2017-04-13 15:43:05]

ui/ lgtm

[danakj, 2017-04-13 15:56:27]

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas.h
File cc/paint/test_skcanvas.h (right):

https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
cc/paint/test_skcanvas.h:1: // Copyright 2017 The Chromium Authors. All rights
reserved.
On 2017/04/12 22:41:30, enne wrote:
> On 2017/04/12 at 21:47:12, vmpstr wrote:
> > nit: Should this be in cc/test?
> 
> If anything outside of cc/paint uses it, sure? Do you think it should move
now?

Ya test-only files should be in cc/test, the cc_test_support target is all in
there and it's more confusing if that changes.

[enne (OOO), 2017-04-13 17:11:15]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

TBR=thakis@chromium.org,vitalybuka@chromium.org

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-04-13 17:44:16]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 17:44:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-13 18:06:47]

On 2017/04/13 at 15:56:27, danakj wrote:
>
https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas.h
> File cc/paint/test_skcanvas.h (right):
> 
>
https://codereview.chromium.org/2768143002/diff/710001/cc/paint/test_skcanvas...
> cc/paint/test_skcanvas.h:1: // Copyright 2017 The Chromium Authors. All rights
reserved.
> On 2017/04/12 22:41:30, enne wrote:
> > On 2017/04/12 at 21:47:12, vmpstr wrote:
> > > nit: Should this be in cc/test?
> > 
> > If anything outside of cc/paint uses it, sure? Do you think it should move
now?
> 
> Ya test-only files should be in cc/test, the cc_test_support target is all in
there and it's more confusing if that changes.

Oh, ok! Done.

[commit-bot: I haz the power, 2017-04-13 19:54:14]

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

[commit-bot: I haz the power, 2017-04-13 19:54:15]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-04-13 20:25:54]

cc lgtm, good luck!

https://codereview.chromium.org/2768143002/diff/750001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer_unittest.cc (right):

https://codereview.chromium.org/2768143002/diff/750001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer_unittest.cc:66: char text[] = "asdf";
xD nice sample text!

[enne (OOO), 2017-04-13 21:23:10]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-04-13 21:23:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, vitalybuka@chromium.org, thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/2768143002/#ps750001
(title: "Rebase, move test skcanvas to cc/test")

[commit-bot: I haz the power, 2017-04-13 21:24:07]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-13 21:38:57]

CQ is committing da patch.

Bot data: {"patchset_id": 750001, "attempt_start_ts": 1492118590950640,
"parent_rev": "6bfb96f0eb4350010c8d9c29fb40408212536bcc", "commit_rev":
"a4274d73edf32429d703d27520cf6f650346956f"}

[commit-bot: I haz the power, 2017-04-13 21:44:58]

CQ is committing da patch.

Bot data: {"patchset_id": 750001, "attempt_start_ts": 1492118590950640,
"parent_rev": "9ddf0ddf5dc89355220874dc29cc9ca1e3501839", "commit_rev":
"09670a9d1f03c414f8b7bb67c4bd27ccf46a84e1"}

[commit-bot: I haz the power, 2017-04-13 21:45:55]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464555}
Committed:
https://chromium.googlesource.com/chromium/src/+/09670a9d1f03c414f8b7bb67c4bd...
==========

[commit-bot: I haz the power, 2017-04-13 21:45:58]

Committed patchset #39 (id:750001) as
https://chromium.googlesource.com/chromium/src/+/09670a9d1f03c414f8b7bb67c4bd...

[findit-for-me, 2017-04-13 21:57:51]

A revert of this CL (patchset #39 id:750001) has been created in
https://codereview.chromium.org/2819553004/ by
findit-for-me@appspot.gserviceaccount.com.

The reason for reverting is: 
Findit(https://goo.gl/kROfz5) identified CL at revision 464555 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....

[cmumford, 2017-04-13 22:06:09]

A revert of this CL (patchset #39 id:750001) has been created in
https://codereview.chromium.org/2810363004/ by cmumford@chromium.org.

The reason for reverting is: Triggered automatic tree closure. Failure on mac.

https://uberchromegw.corp.google.com/i/chromium.mac/builders/Mac%20Builder.

[enne (OOO), 2017-04-13 23:48:41]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464555}
Committed:
https://chromium.googlesource.com/chromium/src/+/09670a9d1f03c414f8b7bb67c4bd...
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-04-13 23:50:22]

Apparently the unit test doesn't build without dchecks on, but apparently none
of the bots have non-dcheck builds.

Made the unit test just #ifdef out some of the checks for SkRefCnt::getRefCnt
and verified locally that it now compiles.

[enne (OOO), 2017-04-13 23:51:46]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-13 23:52:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-14 00:34:14]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2017-04-14 00:34:17]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-04-14 00:34:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, vmpstr@chromium.org, thakis@chromium.org,
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2768143002/#ps770001
(title: "Fix release builds")

[commit-bot: I haz the power, 2017-04-14 00:35:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 02:08:50]

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

[commit-bot: I haz the power, 2017-04-14 02:08:53]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[enne (OOO), 2017-04-14 06:06:18]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-04-14 06:06:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, vmpstr@chromium.org, thakis@chromium.org,
vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2768143002/#ps790001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-14 06:06:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 07:42:40]

CQ is committing da patch.

Bot data: {"patchset_id": 790001, "attempt_start_ts": 1492149978509930,
"parent_rev": "d1040f6d95719c52a3ec442c76be29d5c0630bf4", "commit_rev":
"e594588348ea08e4cdc15dc587efa5645db5023e"}

[commit-bot: I haz the power, 2017-04-14 07:43:42]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464701}
Committed:
https://chromium.googlesource.com/chromium/src/+/e594588348ea08e4cdc15dc587ef...
==========

[commit-bot: I haz the power, 2017-04-14 07:43:45]

Committed patchset #41 (id:790001) as
https://chromium.googlesource.com/chromium/src/+/e594588348ea08e4cdc15dc587ef...

[findit-for-me, 2017-04-14 09:44:19]

Findit(https://goo.gl/kROfz5) identified this CL at revision 464701 as the
culprit for
failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

[alexmos, 2017-04-14 17:18:09]

On 2017/04/14 09:44:19, findit-for-me wrote:
> Findit(https://goo.gl/kROfz5) identified this CL at revision 464701 as the
> culprit for
> failures in the build cycles as shown on:
>
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...

I've just filed https://bugs.chromium.org/p/chromium/issues/detail?id=711705 for
this failure.

[enne (OOO), 2017-04-14 17:20:53]

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

The reason for reverting is: Likely making snapshot unit tests time out.

[enne (OOO), 2017-04-14 18:46:22]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464701}
Committed:
https://chromium.googlesource.com/chromium/src/+/e594588348ea08e4cdc15dc587ef...
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[enne (OOO), 2017-04-14 18:50:01]

Ok, take 3.  Thanks again, bots on the waterfall that aren't try jobs.  T_T

It turns out that snapshot_unittests allocates 800x600 draw rects in a
PaintOpBuffer:
https://cs.chromium.org/chromium/src/ui/snapshot/snapshot_aura_unittest.cc?l=55.
 This makes the test timeout while recording because the PaintOpBuffer reallocs
itself every 40 draw rects or so.  It does finish successfully, just not very
speedily.

I think this is maybe legit and something a display list canvas could do.  So,
instead, change PaintOpBuffer to behave more like std::vector and reallocate
itself at 2x the size.  This won't affect most page content which super easily
fits in 4k PaintOpBuffers (and already has ShrinkToFit called on it at the end),
but will save on a lot of reallocs for large PaintOpBuffers.

[enne (OOO), 2017-04-14 18:50:30]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 18:51:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vmpstr, 2017-04-14 19:03:23]

lgtm

[vmpstr, 2017-04-14 19:03:41]

The CQ bit was unchecked by vmpstr@chromium.org

[vmpstr, 2017-04-14 19:03:53]

The CQ bit was checked by vmpstr@chromium.org

[vmpstr, 2017-04-14 19:03:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, thakis@chromium.org, vitalybuka@chromium.org
Link to the patchset: https://codereview.chromium.org/2768143002/#ps810001
(title: "Change allocation behavior of PaintOpBuffer to x2")

[commit-bot: I haz the power, 2017-04-14 19:04:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 22:27:45]

CQ is committing da patch.

Bot data: {"patchset_id": 810001, "attempt_start_ts": 1492196633998910,
"parent_rev": "e5ff978c98c235da0dfb6f6d8dc45f5e0c764eec", "commit_rev":
"e49d70a44bf12043cc781390f7fc53ce7b4ac807"}

[commit-bot: I haz the power, 2017-04-14 22:29:01]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464809}
Committed:
https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc...
==========

[commit-bot: I haz the power, 2017-04-14 22:29:04]

Committed patchset #42 (id:810001) as
https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc...

[hidehiko, 2017-04-18 09:32:10]

A revert of this CL (patchset #42 id:810001) has been created in
https://codereview.chromium.org/2820133005/ by hidehiko@chromium.org.

The reason for reverting is: Reverting due to CrOS breakage.
crbug.com/712102
.

[danakj, 2017-04-19 22:02:19]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464809}
Committed:
https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc...
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464809}
Committed:
https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc...
==========

[danakj, 2017-04-20 16:05:21]

https://codereview.chromium.org/2768143002/diff/810001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/810001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:723: if (!op_count_) {
I'm trying to understand why this is nested in the other if condition which
checks the same thing, and not getting it. Can this be outside and then remove
!op_count_ from the other if?

[vmpstr, 2017-04-20 16:51:46]

https://codereview.chromium.org/2768143002/diff/810001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/810001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:723: if (!op_count_) {
On 2017/04/20 at 16:05:21, danakj wrote:
> I'm trying to understand why this is nested in the other if condition which
checks the same thing, and not getting it. Can this be outside and then remove
!op_count_ from the other if?

Yeah, it can. skip should be > 0, so "used_ + skip > reserved_" is enough to
trigger the reserve path if this if pushes NoopOp (now on line 729). Otherwise,
this block would return.

[danakj, 2017-04-20 17:32:33]

https://codereview.chromium.org/2768143002/diff/830001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/830001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:770: char first_op_[sizeof(LargestPaintOp)];
This needs to be aligned or parts of the op can fall out of the array I believe.
Here's a diff:

diff --git a/cc/paint/paint_op_buffer.h b/cc/paint/paint_op_buffer.h
index b5f60a4dff15..d625153eb05e 100644
--- a/cc/paint/paint_op_buffer.h
+++ b/cc/paint/paint_op_buffer.h
@@ -8,6 +8,7 @@
 #include <stdint.h>
 
 #include "base/logging.h"
+#include "base/memory/aligned_memory.h"
 #include "cc/paint/paint_canvas.h"
 #include "cc/paint/paint_export.h"
 #include "cc/paint/paint_flags.h"
@@ -571,7 +572,7 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
   const SkRect& cullRect() const { return cull_rect_; }
 
   PaintOp* GetFirstOp() const {
-    return reinterpret_cast<PaintOp*>(const_cast<char*>(&first_op_[0]));
+    return const_cast<PaintOp*>(first_op_.data_as<PaintOp>());
   }
 
   template <typename T, typename... Args>
@@ -728,7 +729,13 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
           // to be a common case.
           push<NoopOp>();
         } else {
-          T* op = reinterpret_cast<T*>(&first_op_[0]);
+          // |first_op_| is aligned to LargestPaintOp. If T needs a smaller
+          // alignment, this is okay because it will be a factor of the actual
+          // alignment being used (as they are always a power of 2). If T needs
+          // a larger alignment, that is bad and we should use T to choose the
+          // alignment of |first_op_| instead.
+          static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
+          auto* op = reinterpret_cast<T*>(first_op_.data_as<T>());
           new (op) T{std::forward<Args>(args)...};
           op->type = static_cast<uint32_t>(T::kType);
           op->skip = 0;
@@ -762,7 +769,8 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
 
   // As a performance optimization because n=1 is an extremely common case just
   // store the first op in the PaintOpBuffer itself to avoid an extra alloc.
-  char first_op_[sizeof(LargestPaintOp)];
+  base::AlignedMemory<sizeof(LargestPaintOp), ALIGNOF(LargestPaintOp)>
+      first_op_;
   SkAutoTMalloc<char> data_;
   size_t used_ = 0;
   size_t reserved_ = 0;

[danakj, 2017-04-20 17:33:13]

On 2017/04/20 17:32:33, danakj wrote:
>
https://codereview.chromium.org/2768143002/diff/830001/cc/paint/paint_op_buff...
> File cc/paint/paint_op_buffer.h (right):
> 
>
https://codereview.chromium.org/2768143002/diff/830001/cc/paint/paint_op_buff...
> cc/paint/paint_op_buffer.h:770: char first_op_[sizeof(LargestPaintOp)];
> This needs to be aligned or parts of the op can fall out of the array I
believe.
> Here's a diff:
> 
> diff --git a/cc/paint/paint_op_buffer.h b/cc/paint/paint_op_buffer.h
> index b5f60a4dff15..d625153eb05e 100644
> --- a/cc/paint/paint_op_buffer.h
> +++ b/cc/paint/paint_op_buffer.h
> @@ -8,6 +8,7 @@
>  #include <stdint.h>
>  
>  #include "base/logging.h"
> +#include "base/memory/aligned_memory.h"
>  #include "cc/paint/paint_canvas.h"
>  #include "cc/paint/paint_export.h"
>  #include "cc/paint/paint_flags.h"
> @@ -571,7 +572,7 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
>    const SkRect& cullRect() const { return cull_rect_; }
>  
>    PaintOp* GetFirstOp() const {
> -    return reinterpret_cast<PaintOp*>(const_cast<char*>(&first_op_[0]));
> +    return const_cast<PaintOp*>(first_op_.data_as<PaintOp>());
>    }
>  
>    template <typename T, typename... Args>
> @@ -728,7 +729,13 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
>            // to be a common case.
>            push<NoopOp>();
>          } else {
> -          T* op = reinterpret_cast<T*>(&first_op_[0]);
> +          // |first_op_| is aligned to LargestPaintOp. If T needs a smaller
> +          // alignment, this is okay because it will be a factor of the
actual
> +          // alignment being used (as they are always a power of 2). If T
needs
> +          // a larger alignment, that is bad and we should use T to choose
the
> +          // alignment of |first_op_| instead.
> +          static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
> +          auto* op = reinterpret_cast<T*>(first_op_.data_as<T>());

Oops the reinterpret_cast is not needed tho thanks to data_as.

>            new (op) T{std::forward<Args>(args)...};
>            op->type = static_cast<uint32_t>(T::kType);
>            op->skip = 0;
> @@ -762,7 +769,8 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
>  
>    // As a performance optimization because n=1 is an extremely common case
just
>    // store the first op in the PaintOpBuffer itself to avoid an extra alloc.
> -  char first_op_[sizeof(LargestPaintOp)];
> +  base::AlignedMemory<sizeof(LargestPaintOp), ALIGNOF(LargestPaintOp)>
> +      first_op_;
>    SkAutoTMalloc<char> data_;
>    size_t used_ = 0;
>    size_t reserved_ = 0;

[Khushal, 2017-04-21 00:28:34]

https://codereview.chromium.org/2768143002/diff/850001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/850001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:748: return op;
Are we missing this op for the  slow path and additional bytes usage accounting?

[enne (OOO), 2017-04-21 00:48:16]

https://codereview.chromium.org/2768143002/diff/850001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/850001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:748: return op;
On 2017/04/21 at 00:28:34, Khushal wrote:
> Are we missing this op for the  slow path and additional bytes usage
accounting?

Oh quite right.

[danakj, 2017-04-21 15:43:26]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 15:43:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2017-04-21 17:27:19]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2017-04-21 17:29:58]

I'm sending the latest patchset to: tools/perf/run_benchmark try winx64-high-dpi
blink_perf.svg

[enne (OOO), 2017-04-25 18:51:38]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 18:52:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-25 19:02:41]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-25 19:03:41]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-25 19:47:14]

https://codereview.chromium.org/2768143002/diff/910001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/910001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:747: AnalyzeAddedOp(op);
Added khushal's AnalyzeAddedOp function from
https://codereview.chromium.org/2830243002/ here to fix the missing slow path
counting.

https://codereview.chromium.org/2768143002/diff/910001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.cc (right):

https://codereview.chromium.org/2768143002/diff/910001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.cc:79: if (!canvas_) {
Since PaintRecorder does a restoreToCount(1) here, this deliberately doesn't
create the canvas if it doesn't exist yet.

https://codereview.chromium.org/2768143002/diff/910001/cc/paint/record_paint_...
File cc/paint/record_paint_canvas.h (right):

https://codereview.chromium.org/2768143002/diff/910001/cc/paint/record_paint_...
cc/paint/record_paint_canvas.h:152: mutable base::Optional<SkNoDrawCanvas>
canvas_;
It looks like all the blink_perf.paint regressions were due to the SkCanvas
initialization for every RecordPaintCanvas.

One approach which helped quite a bit was to have PaintRecorder own the canvas
and then resetForNextPicture (via https://skia-review.googlesource.com/c/11320/)
between recordings (when the PaintRecorder is reused).

The approach used here is to make the canvas optional per RecordPaintCanvas,
which addresses perf issues when the canvas isn't needed (such as on very common
single ops) and should fix the blink_perf.paint issues.

I think a followup is to combine these approaches (keep it optional, but store
it on the PaintRecorder) if the skia patch lands.

[enne (OOO), 2017-04-25 19:47:32]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#464809}
Committed:
https://chromium.googlesource.com/chromium/src/+/e49d70a44bf12043cc781390f7fc...
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-04-25 19:49:21]

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

[commit-bot: I haz the power, 2017-04-25 19:49:23]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)

[danakj, 2017-04-25 20:12:51]

LGTM

[enne (OOO), 2017-04-25 21:51:46]

Am going to rebaseline another test or two and then reland.  Windows compile
issues seem fixed now that I DISALLOW_ETC'd all the canvases.

[enne (OOO), 2017-04-26 00:38:58]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-26 00:39:47]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 03:23:07]

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

[commit-bot: I haz the power, 2017-04-26 03:23:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2017-04-26 15:53:30]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-04-26 15:53:31]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, vmpstr@chromium.org, thakis@chromium.org,
vitalybuka@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/2768143002/#ps950001
(title: "more layout tests")

[commit-bot: I haz the power, 2017-04-26 15:53:52]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-26 16:04:17]

CQ is committing da patch.

Bot data: {"patchset_id": 950001, "attempt_start_ts": 1493222010786350,
"parent_rev": "7de4101a99fb5d1d2a03dc7bb9a6f18e30b04269", "commit_rev":
"f5a5ed867ba06ae56f8fc299a9eb194471277eec"}

[commit-bot: I haz the power, 2017-04-26 16:04:31]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#467335}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5a5ed867ba06ae56f8fc299a9eb...
==========

[commit-bot: I haz the power, 2017-04-26 16:04:35]

Committed patchset #49 (id:950001) as
https://chromium.googlesource.com/chromium/src/+/f5a5ed867ba06ae56f8fc299a9eb...

[Qiang(Joe) Xu, 2017-04-26 22:57:17]

A revert of this CL (patchset #49 id:950001) has been created in
https://codereview.chromium.org/2839343002/ by warx@chromium.org.

The reason for reverting is: Breaks peach_pit-tot-chrome-pfq-informational Build
#5967:
https://uberchromegw.corp.google.com/i/chromeos.chrome/builders/peach_pit-tot....

[danakj, 2017-04-27 15:41:08]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#467335}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5a5ed867ba06ae56f8fc299a9eb...
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#467335}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5a5ed867ba06ae56f8fc299a9eb...
==========

[danakj, 2017-04-27 17:29:43]

https://codereview.chromium.org/2768143002/diff/950001/cc/paint/paint_op_buff...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/950001/cc/paint/paint_op_buff...
cc/paint/paint_op_buffer.h:726: size_t skip = SkAlignPtr(sizeof(T) + bytes);
After much confusion about alignment, I think this needs to be changes for 32bit
arm maybe:

diff --git a/cc/paint/paint_op_buffer.h b/cc/paint/paint_op_buffer.h
index c87459067642..18f768b5c920 100644
--- a/cc/paint/paint_op_buffer.h
+++ b/cc/paint/paint_op_buffer.h
@@ -723,7 +723,15 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
 
   template <typename T, typename... Args>
   T* push_internal(size_t bytes, Args&&... args) {
-    size_t skip = SkAlignPtr(sizeof(T) + bytes);
+    // |first_op_| and each element in |data_| is aligned to LargestPaintOp. If
+    // T needs a smaller alignment, this is okay because it will be a factor of
+    // the actual alignment being used (as they are always a power of 2). If T
+    // needs a larger alignment, that is bad and we should use T to choose the
+    // alignment of |first_op_| and elements in |data_| instead.
+    static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
+
+    size_t skip = MathUtil::UncheckedRoundUp(sizeof(T) + bytes,
+                                             ALIGNOF(LargestPaintOp));
     DCHECK_LT(skip, static_cast<size_t>(1) << 24);
     if (used_ + skip > reserved_ || !op_count_) {
       if (!op_count_) {
@@ -734,12 +742,6 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
           // to be a common case.
           push<NoopOp>();
         } else {
-          // |first_op_| is aligned to LargestPaintOp. If T needs a smaller
-          // alignment, this is okay because it will be a factor of the actual
-          // alignment being used (as they are always a power of 2). If T needs
-          // a larger alignment, that is bad and we should use T to choose the
-          // alignment of |first_op_| instead.
-          static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
           auto* op = reinterpret_cast<T*>(first_op_.data_as<T>());
           new (op) T{std::forward<Args>(args)...};
           op->type = static_cast<uint32_t>(T::kType);

[enne (OOO), 2017-04-28 23:32:33]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[enne (OOO), 2017-04-28 23:32:43]

On 2017/04/27 at 17:29:43, danakj wrote:
>
https://codereview.chromium.org/2768143002/diff/950001/cc/paint/paint_op_buff...
> File cc/paint/paint_op_buffer.h (right):
> 
>
https://codereview.chromium.org/2768143002/diff/950001/cc/paint/paint_op_buff...
> cc/paint/paint_op_buffer.h:726: size_t skip = SkAlignPtr(sizeof(T) + bytes);
> After much confusion about alignment, I think this needs to be changes for
32bit arm maybe:
> 
> diff --git a/cc/paint/paint_op_buffer.h b/cc/paint/paint_op_buffer.h
> index c87459067642..18f768b5c920 100644
> --- a/cc/paint/paint_op_buffer.h
> +++ b/cc/paint/paint_op_buffer.h
> @@ -723,7 +723,15 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
>  
>    template <typename T, typename... Args>
>    T* push_internal(size_t bytes, Args&&... args) {
> -    size_t skip = SkAlignPtr(sizeof(T) + bytes);
> +    // |first_op_| and each element in |data_| is aligned to LargestPaintOp.
If
> +    // T needs a smaller alignment, this is okay because it will be a factor
of
> +    // the actual alignment being used (as they are always a power of 2). If
T
> +    // needs a larger alignment, that is bad and we should use T to choose
the
> +    // alignment of |first_op_| and elements in |data_| instead.
> +    static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
> +
> +    size_t skip = MathUtil::UncheckedRoundUp(sizeof(T) + bytes,
> +                                             ALIGNOF(LargestPaintOp));
>      DCHECK_LT(skip, static_cast<size_t>(1) << 24);
>      if (used_ + skip > reserved_ || !op_count_) {
>        if (!op_count_) {
> @@ -734,12 +742,6 @@ class CC_PAINT_EXPORT PaintOpBuffer : public SkRefCnt {
>            // to be a common case.
>            push<NoopOp>();
>          } else {
> -          // |first_op_| is aligned to LargestPaintOp. If T needs a smaller
> -          // alignment, this is okay because it will be a factor of the
actual
> -          // alignment being used (as they are always a power of 2). If T
needs
> -          // a larger alignment, that is bad and we should use T to choose
the
> -          // alignment of |first_op_| instead.
> -          static_assert(ALIGNOF(T) <= ALIGNOF(LargestPaintOp), "");
>            auto* op = reinterpret_cast<T*>(first_op_.data_as<T>());
>            new (op) T{std::forward<Args>(args)...};
>            op->type = static_cast<uint32_t>(T::kType);

[commit-bot: I haz the power, 2017-04-28 23:33:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-04-28 23:33:50]

Ok, included the UncheckedRoundUp here, and changed all allocs/reallocs of the
buffer to AlignedAlloc/AlignedFree.  I also rearranged the data and the array in
PaintOpArray since the array needs to be aligned on ARM.  I added a static
assert that it's aligned.

vmpstr, danakj, do you want to re-review?

[enne (OOO), 2017-04-28 23:35:14]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#467335}
Committed:
https://chromium.googlesource.com/chromium/src/+/f5a5ed867ba06ae56f8fc299a9eb...
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[commit-bot: I haz the power, 2017-04-29 01:06:59]

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

[commit-bot: I haz the power, 2017-04-29 01:07:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[enne (OOO), 2017-04-30 21:37:35]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-30 21:37:43]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-01 02:44:22]

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

[commit-bot: I haz the power, 2017-05-01 02:44:25]

Dry run: This issue passed the CQ dry run.

[vmpstr, 2017-05-01 16:32:36]

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:128: return op + 1;
nit: Can you do a similar trick as on line 157, just for the benefit of a dcheck
and any future modifications

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:157: return const_cast<const
void*>(GetDataForThis(const_cast<T*>(op)));
It doesn't really matter here, I don't think, but it's typically safer to
implement the function that takes non-const using the one that takes a const,
not the other way around (since the initial caller passes a non-const, it's safe
to cast to const always).

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:165: sizeof(T) % ALIGNOF(M) == 0,
Just confirming, this is equivalent to checking

(op + 1) % ALIGNOF(M) since op should be aligned and if size is aligned, then op
+ 1 is aligned?

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:580: static constexpr size_t PaintOpAlign =
ALIGNOF(DrawDRRectOp);
LargestPaintOp (I know it's static asserted, but feels nicer not to call out a
specific op)

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:748: size_t skip =
MathUtil::UncheckedRoundUp(sizeof(T) + bytes, PaintOpAlign);
Can you leave a comment here explaining why we do this?

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:780: new (op) T(std::forward<Args>(args)...);
It's not too clear to me if T is aligned, why the data is also aligned... Can
you maybe explain in a comment on line 748?

[enne (OOO), 2017-05-01 20:29:16]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 20:29:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-05-01 20:32:55]

Left a bunch of comments.

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
File cc/paint/paint_op_buffer.h (right):

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:157: return const_cast<const
void*>(GetDataForThis(const_cast<T*>(op)));
On 2017/05/01 at 16:32:36, vmpstr wrote:
> It doesn't really matter here, I don't think, but it's typically safer to
implement the function that takes non-const using the one that takes a const,
not the other way around (since the initial caller passes a non-const, it's safe
to cast to const always).

Yeah, I couldn't make it work, because the non-const version would always call
itself (with T='const SomeOp') and it'd infinite loop.  This is really just a
one-liner that's clearly const, so I don't feel too worried about it.

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:165: sizeof(T) % ALIGNOF(M) == 0,
On 2017/05/01 at 16:32:36, vmpstr wrote:
> Just confirming, this is equivalent to checking
> 
> (op + 1) % ALIGNOF(M) since op should be aligned and if size is aligned, then
op + 1 is aligned?

Yeah, it's making sure M[] placed at (op+1) would be properly aligned if op is
aligned.  I added an additional check that alignof(M) <= alignof(T) and some
comments.

https://codereview.chromium.org/2768143002/diff/1010001/cc/paint/paint_op_buf...
cc/paint/paint_op_buffer.h:580: static constexpr size_t PaintOpAlign =
ALIGNOF(DrawDRRectOp);
On 2017/05/01 at 16:32:36, vmpstr wrote:
> LargestPaintOp (I know it's static asserted, but feels nicer not to call out a
specific op)

Left some comments here, as I don't think largest op and largest alignment are
the same thing.

[commit-bot: I haz the power, 2017-05-01 21:11:36]

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

[commit-bot: I haz the power, 2017-05-01 21:11:38]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[vmpstr, 2017-05-01 21:14:05]

lgtm thanks for the comments

[enne (OOO), 2017-05-01 21:49:43]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-01 21:50:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2017-05-01 22:45:56]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2017-05-01 22:46:01]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2017-05-01 22:46:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org, danakj@chromium.org, thakis@chromium.org,
vitalybuka@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/2768143002/#ps1070001
(title: "more const casting")

[commit-bot: I haz the power, 2017-05-01 22:47:04]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 03:23:49]

CQ is committing da patch.

Bot data: {"patchset_id": 1070001, "attempt_start_ts": 1493678761776240,
"parent_rev": "4b3d57d3066c29135644cf93bd3b3e8092aeeed2", "commit_rev":
"59df29deed59592d42ff7e600f33faf035e00b46"}

[commit-bot: I haz the power, 2017-05-02 03:24:05]

Description was changed from

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Back PaintRecord with PaintOpBuffer instead of SkPicture

Change the backing of PaintRecord to be a data structure implemented in
cc/paint instead of using SkPicture directly.  This new class cribs heavily
from SkLiteDL.

PaintRecord used to be a typedef to an SkPicture but now is a typedef to
a PaintOpBuffer.  (This leaves some flexibility to change this in the
future to an interface without having to modify all of Chromium again.)

PaintOpBuffer stores a contiguous array of ops, with the ops stored
in place.  As an optimization, the first op is stored locally in the
PaintOpBuffer itself to avoid extra allocations for small pictures.

This patch moves slow path counting from a gpu analysis canvas into
PaintOpBuffer directly.  As ops are recorded, slow paths are counted, and
a PaintRecord now knows how many ops it has.  This is about a 1.5% savings
for record time (gpu analysis was 2% and 0.5% overhead to record later).

This patch also implements the SkRecordNoopSaveLayerDrawRestores
optimization from Skia at raster time.  This takes save layer (just
opacity) / draw / restore commands and turns them into draws with
opacity.  It moves that optimization from Blink at record time inside of
CompositingRecorder and moves it to both DisplayItemList::RasterItem
and PaintOpBuffer::playback (since a save could be either a DisplayItem
or a PaintOp).  It's not as robust as Skia's solution and so misses
a few cases that Skia catches, but the rasterize and record on 10k
page agreed that performance was good enough.

CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2768143002
Cr-Commit-Position: refs/heads/master@{#468548}
Committed:
https://chromium.googlesource.com/chromium/src/+/59df29deed59592d42ff7e600f33...
==========

[commit-bot: I haz the power, 2017-05-02 03:24:09]

Committed patchset #55 (id:1070001) as
https://chromium.googlesource.com/chromium/src/+/59df29deed59592d42ff7e600f33...