add SkRecordOptimize2 and an experimental API for more aggressive opts

add SkRecordOptimize2 and an experimental API for more aggressive opts

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/f38b0d86b888692f7d703916114781be43249600

[reed1, 2015-11-19 16:14:14]

reed@google.com changed reviewers:
+ mtklein@google.com

[reed1, 2015-11-19 16:14:15]

WIP at the moment

[mtklein, 2015-11-19 16:26:56]

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp
File src/core/SkRecordOpts.cpp (right):

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp#n...
src/core/SkRecordOpts.cpp:40: // TODO: tried Star<Or<Is<Noop>, Is<SetMatrix>>>,
but it seemed to not match
After refamiliarizing myself with how these patterns and Star work, I think we
want
    Pattern3<Is<SetMatrix>,
             Star<Is<Noop>>,
             Is<SetMatrix>>
and keep
    record->replace<NoOp>(begin);

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp#n...
src/core/SkRecordOpts.cpp:50: static void multiple_set_matrices(SkRecord*
record) {
I've found it butt-saving to have each pass unit testable and unit tested.

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp#n...
src/core/SkRecordOpts.cpp:51: MultipleSetMatrixNooper pass;
Might want to move the struct in here too like with the other pass?

[reed1, 2015-11-19 17:29:44]

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp
File src/core/SkRecordOpts.cpp (right):

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp#n...
src/core/SkRecordOpts.cpp:40: // TODO: tried Star<Or<Is<Noop>, Is<SetMatrix>>>,
but it seemed to not match
On 2015/11/19 16:26:56, mtklein wrote:
> After refamiliarizing myself with how these patterns and Star work, I think we
> want
>     Pattern3<Is<SetMatrix>,
>              Star<Is<Noop>>,
>              Is<SetMatrix>>
> and keep
>     record->replace<NoOp>(begin);

Done.

Does this mean I should loop myself, or is that still not needed?

https://codereview.chromium.org/1462973002/diff/1/src/core/SkRecordOpts.cpp#n...
src/core/SkRecordOpts.cpp:51: MultipleSetMatrixNooper pass;
On 2015/11/19 16:26:56, mtklein wrote:
> Might want to move the struct in here too like with the other pass?

Done.

[reed1, 2015-11-19 17:29:51]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 17:29:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1462973002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1462973002/20001

[commit-bot: I haz the power, 2015-11-19 17:31:20]

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

[commit-bot: I haz the power, 2015-11-19 17:31:21]

Dry run: Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[mtklein, 2015-11-19 17:39:22]

lgtm

https://codereview.chromium.org/1462973002/diff/20001/src/core/SkRecordOpts.cpp
File src/core/SkRecordOpts.cpp (right):

https://codereview.chromium.org/1462973002/diff/20001/src/core/SkRecordOpts.c...
src/core/SkRecordOpts.cpp:40: typedef Pattern3<Is<SetMatrix>,
Sorry, moved the goalposts.

This is now

typedef Pattern<Is<SetMatrix>,
                Greedy<Is<NoOp>>,
                Is<SetMatrix>>
    Match;

bool onMatch(SkRecord* record, Match*, int begin, int) {
    record->replace<NoOp>(begin);  // first SetMatrix
    return true;
}

https://codereview.chromium.org/1462973002/diff/20001/src/core/SkRecordOpts.c...
src/core/SkRecordOpts.cpp:50: // No need to loop, as we never "open up"
opportunities for more of this type of optimization.
I do think this is still true that a single pass gets everything.  E.g.,

   [SetMatrix      NoOp   SetMatrix] SetMatrix  ...
->       NoOp      NoOp   SetMatrix  SetMatrix  ...
->       NoOp      NoOp  [SetMatrix  SetMatrix] ...
->       NoOp      NoOp      NoOp    SetMatrix  ...

But it probably does not hurt to wrap it in a while(...) anyway.
If nothing else, it means we don't have trust we've thought this through
correctly.

[reed1, 2015-11-19 18:08:34]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-11-19 18:08:44]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1462973002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1462973002/40001

[commit-bot: I haz the power, 2015-11-19 18:09:34]

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

[commit-bot: I haz the power, 2015-11-19 18:09:34]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)

[reed1, 2015-11-19 18:18:40]

The CQ bit was checked by reed@google.com

[reed1, 2015-11-19 18:18:40]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1462973002/#ps60001
(title: "fix bugs in manual merge")

[commit-bot: I haz the power, 2015-11-19 18:18:51]

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

[commit-bot: I haz the power, 2015-11-19 18:31:11]

Committed patchset #4 (id:60001) as
https://skia.googlesource.com/skia/+/f38b0d86b888692f7d703916114781be43249600