Start using type traits in src/record instead of macros.

Start using type traits in src/record instead of macros.

Simplified skip logic by always running clip commands.  No performance difference on bot or silk SKPs.

BUG=skia:2378
Committed: http://code.google.com/p/skia/source/detail?r=14410

[mtklein, 2014-04-25 19:10:59]

Ben, can you look at SkTLogic, and Florin, at the rest?

[bungeman-skia, 2014-04-25 21:45:55]

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h
File src/utils/SkTLogic.h (right):

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:61: /** SkTEnableIf is a pre-C++11 std::enable_if. */
The first time I wrote this:
https://codereview.chromium.org/18503009/diff/37001/src/utils/SkTLogic.h

In particular, I like the comments there, which state how to use it. FOr example
here

/** SkTEnableIf::type = (Condition::value) ? T : [does not exist]; */

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:69: // template <typaname T> void foo(T a,
SK_ENABLE_IF(condition)) { ... }
'condition' here seems misleading, since this is actually a 'type'. Note that
the first time I wrote this I made two (following the boost pattern, see
http://www.boost.org/doc/libs/1_55_0/libs/utility/enable_if.html), one with _c
(for condition) and the other without (which was for type). c++11 traits get
around this by using constexpr conversion functions.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:70: // template <typaname T> void foo(T a,
SK_ENABLE_IF(!condition)) { ... }
For example this is somewhat odd, in that this really is !type::value.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:71: #define SK_ENABLE_IF(cond) typename
SkTEnableIf<cond::value, bool>::type = false
#define SK_FUNCTION_ARGUMENT_REQUIRES(type) typename SkTEnableIf<type::value,
bool>::type = false

enable_if already has a meaning, this is the application as a function argument.

Can we use the 'return' version instead of the 'parameter' version?

#define SK_FUNCTION_REQUIRES(cond_type, return_type) typename
SkTEnableIf<cond_type::value, return_type>::type

used like 

template <typename T> SK_FUNCTION_REQUIRES(ChangesClip<T>, void) updateClip()

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:84: enum { value = sizeof(func<Derived>(NULL)) == 2 }; \
We don't normally write it like this, something more like

static const bool value = sizeof(test<Derived>(NULL)) == sizeof(uint16_t);

seems clearer.

[f(malita), 2014-04-28 14:20:44]

rest lgtm + ramblings

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordDraw.cpp
File src/record/SkRecordDraw.cpp (right):

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordDraw....
src/record/SkRecordDraw.cpp:63: void updateClip(SK_ENABLE_IF(ChangesClip<T>)) {
fClipEmpty = fCanvas->isClipEmpty(); }
I was thinking we could defer updating the clip until needed (as opposed to on
every clip-changing cmd), but looking at SkCanvas::isClipEmpty() it seems really
cheap - plus in practice I think we rarely tweak the clip repeatedly with no
intervening draws. So probably not worth pursuing, just making a note.

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordOpts.cpp
File src/record/SkRecordOpts.cpp (right):

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordOpts....
src/record/SkRecordOpts.cpp:63: fSave = kInactive;
This seems to only noop the top-most save/restore frame - can we extend it to
apply to parent frames also? (I'm guessing we'll need a stack for tracking
fSave, so not sure whether it's going to be a net gain in the end).

[mtklein, 2014-04-28 15:47:04]

all done.  ptal!

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordDraw.cpp
File src/record/SkRecordDraw.cpp (right):

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordDraw....
src/record/SkRecordDraw.cpp:63: void updateClip(SK_ENABLE_IF(ChangesClip<T>)) {
fClipEmpty = fCanvas->isClipEmpty(); }
On 2014/04/28 14:20:44, Florin Malita wrote:
> I was thinking we could defer updating the clip until needed (as opposed to on
> every clip-changing cmd), but looking at SkCanvas::isClipEmpty() it seems
really
> cheap - plus in practice I think we rarely tweak the clip repeatedly with no
> intervening draws. So probably not worth pursuing, just making a note.

Figure it's cheap enough that it's pointless to track the extra state myself?  I
could just call fCanvas->isClipEmpty() there in skip()?  I'll measure this as a
follow up.

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordOpts.cpp
File src/record/SkRecordOpts.cpp (right):

https://codereview.chromium.org/258693006/diff/60001/src/record/SkRecordOpts....
src/record/SkRecordOpts.cpp:63: fSave = kInactive;
On 2014/04/28 14:20:44, Florin Malita wrote:
> This seems to only noop the top-most save/restore frame - can we extend it to
> apply to parent frames also? (I'm guessing we'll need a stack for tracking
> fSave, so not sure whether it's going to be a net gain in the end).

Yep, as written this only noops away innermost save-restore blocks.

I'm hoping we can get by with just running the optimization repeatedly until it
reaches its fixpoint, as we're doing in SkRecordNoopSaveRestores().  It sort of
peels back noop save-restore blocks one layer at a time.

As we've got things right now, this is the only optimization that's really able
to enable other optimizations, and even then, it can really only enable itself. 
That'll change when we have 4 or 5 of these pattern matchers interacting at
once, and we'll probably want to fixpoint them all as a batch rather than one
after the other.  The future is fancy.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h
File src/utils/SkTLogic.h (right):

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:61: /** SkTEnableIf is a pre-C++11 std::enable_if. */
On 2014/04/25 21:45:55, bungeman1 wrote:
> The first time I wrote this:
> https://codereview.chromium.org/18503009/diff/37001/src/utils/SkTLogic.h
> 
> In particular, I like the comments there, which state how to use it. FOr
example
> here
> 
> /** SkTEnableIf::type = (Condition::value) ? T : [does not exist]; */

Done.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:69: // template <typaname T> void foo(T a,
SK_ENABLE_IF(condition)) { ... }
On 2014/04/25 21:45:55, bungeman1 wrote:
> 'condition' here seems misleading, since this is actually a 'type'. Note that
> the first time I wrote this I made two (following the boost pattern, see
> http://www.boost.org/doc/libs/1_55_0/libs/utility/enable_if.html), one with _c
> (for condition) and the other without (which was for type). c++11 traits get
> around this by using constexpr conversion functions.

Done.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:70: // template <typaname T> void foo(T a,
SK_ENABLE_IF(!condition)) { ... }
On 2014/04/25 21:45:55, bungeman1 wrote:
> For example this is somewhat odd, in that this really is !type::value.

Done.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:71: #define SK_ENABLE_IF(cond) typename
SkTEnableIf<cond::value, bool>::type = false
On 2014/04/25 21:45:55, bungeman1 wrote:
> #define SK_FUNCTION_ARGUMENT_REQUIRES(type) typename SkTEnableIf<type::value,
> bool>::type = false
> 
> enable_if already has a meaning, this is the application as a function
argument.
> 
> Can we use the 'return' version instead of the 'parameter' version?
> 
> #define SK_FUNCTION_REQUIRES(cond_type, return_type) typename
> SkTEnableIf<cond_type::value, return_type>::type
> 
> used like 
> 
> template <typename T> SK_FUNCTION_REQUIRES(ChangesClip<T>, void) updateClip()
> 

Done.

https://codereview.chromium.org/258693006/diff/60001/src/utils/SkTLogic.h#new...
src/utils/SkTLogic.h:84: enum { value = sizeof(func<Derived>(NULL)) == 2 }; \
On 2014/04/25 21:45:55, bungeman1 wrote:
> We don't normally write it like this, something more like
> 
> static const bool value = sizeof(test<Derived>(NULL)) == sizeof(uint16_t);
> 
> seems clearer.

Done.

[bungeman-skia, 2014-04-28 16:10:34]

Logic stuff lgtm.

[mtklein, 2014-04-28 16:13:26]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-04-28 16:13:44]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/258693006/80001

[commit-bot: I haz the power, 2014-04-28 16:19:52]

Change committed as 14410