Collapse saveLayer/drawBitmapRect/restore trios to drawBitmapRect when possible

This moves one of the optimizations in https://codereview.appspot.com/7294049/ from the filter tool to SkPictureRecord. This optimization removes saveLayer/restore from the opcode stream when a saveLayer/drawbitmaprect/restore trio is seen and the saveLayer's paint can be folded into the drawBitmapRect's paint.

The results are roughly the same as in the filter case:


Executing desk_gmailthread.skp              13
Executing tabl_onlinewsj.skp                12
Executing tabl_ukwsj.skp                    12

      tabl_ukwsj.skp tile_256x256_gpu c   40.14      39.44      +0.70    +1.7%
desk_gmailthread.skp tile_256x256_gpu c   12.47      12.20      +0.27    +2.2%
  tabl_onlinewsj.skp tile_256x256_gpu c   47.00      44.95      +2.05    +4.4%

desk_gmailthread.skp tile_256x256 c    4.98       4.94       +0.04    +0.8%
  tabl_onlinewsj.skp tile_256x256 c    26.46      26.03      +0.43    +1.6%
      tabl_ukwsj.skp tile_256x256 c    23.99      23.56      +0.43    +1.8%

[reed1, 2013-02-22 16:06:08]

Can you illuminate the checkin desc with the specifics of this change?

[robertphillips, 2013-02-25 13:58:08]

Updated description

[reed1, 2013-02-25 14:48:34]

sure suggestive of wanting to formalize a table of peekholes, moving them into a
separate file(s), etc... Tho not actually requesting that per-se

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:151: // Is the supplied paint simply a color?
Seems like we have this query in other places.

Might be faster always checking all fields w/o the short-circuit.

e.g.

intptr_t x = (intptr_t)p.getFoo() | (intptr_t)p.getBar() | ...;
return 0 == x;

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:183: if (SAVE_LAYER != op) {
can we add an assert that op is a known kSave variant here?

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:189: if (bmr2rOffset >= restoreOffset) {
shouldn't we optimize this guy as well?

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:195: if (DRAW_BITMAP_RECT_TO_RECT != op) {
Logically, can we apply this optimization to ALL draws, and not just
drawbitmaprect?

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:203: 
Is there a sensible way to break this into some helper functions, like
- is_single_draw_in_savelayer
- combine_layer_and_draw_paints

?

[robertphillips, 2013-02-27 18:05:06]

I have taken these follow-on TODOs:

Update SkPictureFlat to have array mapping from id to flat
Calve off subroutines from SkPictureRecord::remove_save_layer1
Make array of peephole optimizations & iterate rather than calling directly
Add a stack of save/restores in SkPictureRecord so we don’t have to loop back
Investigate adding killed op’s size to prior op rather than writing NOOP
Add helper to skip over NOOPs when getting next op
Store number of words (rather than bytes) in op-size field

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp
File src/core/SkPictureRecord.cpp (right):

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:151: // Is the supplied paint simply a color?
On 2013/02/25 14:48:34, reed1 wrote:
> Seems like we have this query in other places.
> 
> Might be faster always checking all fields w/o the short-circuit.
> 
> e.g.
> 
> intptr_t x = (intptr_t)p.getFoo() | (intptr_t)p.getBar() | ...;
> return 0 == x;

Done.

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:183: if (SAVE_LAYER != op) {
On 2013/02/25 14:48:34, reed1 wrote:
> can we add an assert that op is a known kSave variant here?

Done.

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:189: if (bmr2rOffset >= restoreOffset) {
On 2013/02/25 14:48:34, reed1 wrote:
> shouldn't we optimize this guy as well?

Done - surprisingly we are seeing a handful of these in the skps.

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:195: if (DRAW_BITMAP_RECT_TO_RECT != op) {
Done-ish - I have expanded handling to cover all drawBitmap* ops.

https://codereview.appspot.com/7387045/diff/7002/src/core/SkPictureRecord.cpp...
src/core/SkPictureRecord.cpp:203: 
I think there is but I would like to hold off until I move the next peephole
optimization over from the filter tool. That optimization shares a lot with this
one so it should become clear where the function boundaries should be drawn.

[reed1, 2013-02-27 18:52:06]

lgtm

[robertphillips, 2013-03-01 14:13:17]

committed as r7916

[robertphillips, 2013-03-01 19:44:46]

Relanded as 7931