save/restore the canvas around every bench draw call

save/restore the canvas around every bench draw call

BUG=

Committed: http://code.google.com/p/skia/source/detail?r=11728

Committed: http://code.google.com/p/skia/source/detail?r=11755

[djsollen, 2013-10-11 14:39:05]

won't this just mask the real issue that this particular bench eventually scales
itself to zero?  If we get a faster processor or we increase the number of ms
per frame we will still hit this case and crash.  Seems like we should just fix
the bench to handle this itself.

[mtklein, 2013-10-11 14:52:56]

On 2013/10/11 14:39:05, djsollen wrote:
> won't this just mask the real issue that this particular bench eventually
scales
> itself to zero?  If we get a faster processor or we increase the number of ms
> per frame we will still hit this case and crash.  Seems like we should just
fix
> the bench to handle this itself.

Oh, reply by email doesn't post here does it?  For posterity:

"""
Nah, the bench only scales itself once per draw call, then runs the number of
loops you tell it to.  What happened here is when we added the inner loop for
each frame, it now scaled itself once per frame, which if we got "lucky" could
get itself down to ~0.

We _could_ make the bench reset the canvas itself, but I think it's better to
restore the old contract that the canvas coming into onDraw is pristine.
"""

[djsollen, 2013-10-11 14:54:24]

thanks for the explanation. lgtm

[commit-bot: I haz the power, 2013-10-11 14:57:27]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/26848013/1

[commit-bot: I haz the power, 2013-10-11 15:20:30]

Change committed as 11728

[reed1, 2013-10-14 13:18:04]

https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp
File bench/benchmain.cpp (right):

https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp#newcode601
bench/benchmain.cpp:601: if (NULL != canvas) {
We have SkAutoCanvasRestore. One if its smarts is that it records the
save-count, and then in its destructor calls restoreToCount(), ensuring that we
achieve balance (and harmony). Can we use this idea here too?

[mtklein, 2013-10-14 14:42:39]

On 2013/10/14 13:18:04, reed1 wrote:
> https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp
> File bench/benchmain.cpp (right):
> 
> https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp#newcode601
> bench/benchmain.cpp:601: if (NULL != canvas) {
> We have SkAutoCanvasRestore. One if its smarts is that it records the
> save-count, and then in its destructor calls restoreToCount(), ensuring that
we
> achieve balance (and harmony). Can we use this idea here too?

Is this what you're thinking?  Now that you've pointed it out, I do prefer using
SkAutoCanvasRestore too.

Note though that this wouldn't have made a difference here in bench.  Saves and
restores were balanced before this CL; they were happening insufficiently often.

[reed1, 2013-10-14 14:51:17]

On 2013/10/14 14:42:39, mtklein wrote:
> On 2013/10/14 13:18:04, reed1 wrote:
> > https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp
> > File bench/benchmain.cpp (right):
> > 
> >
https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp#newcode601
> > bench/benchmain.cpp:601: if (NULL != canvas) {
> > We have SkAutoCanvasRestore. One if its smarts is that it records the
> > save-count, and then in its destructor calls restoreToCount(), ensuring that
> we
> > achieve balance (and harmony). Can we use this idea here too?
> 
> Is this what you're thinking?  Now that you've pointed it out, I do prefer
using
> SkAutoCanvasRestore too.
> 
> Note though that this wouldn't have made a difference here in bench.  Saves
and
> restores were balanced before this CL; they were happening insufficiently
often.

Ah, that dern null-check.

I guess I was just looking for a call to restoreToCount... in case the client
code left the stack unbalanced by more than 1

[mtklein, 2013-10-14 14:58:18]

On 2013/10/14 14:51:17, reed1 wrote:
> On 2013/10/14 14:42:39, mtklein wrote:
> > On 2013/10/14 13:18:04, reed1 wrote:
> > > https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp
> > > File bench/benchmain.cpp (right):
> > > 
> > >
> https://codereview.chromium.org/26848013/diff/1/bench/benchmain.cpp#newcode601
> > > bench/benchmain.cpp:601: if (NULL != canvas) {
> > > We have SkAutoCanvasRestore. One if its smarts is that it records the
> > > save-count, and then in its destructor calls restoreToCount(), ensuring
that
> > we
> > > achieve balance (and harmony). Can we use this idea here too?
> > 
> > Is this what you're thinking?  Now that you've pointed it out, I do prefer
> using
> > SkAutoCanvasRestore too.
> > 
> > Note though that this wouldn't have made a difference here in bench.  Saves
> and
> > restores were balanced before this CL; they were happening insufficiently
> often.
> 
> Ah, that dern null-check.
> 
> I guess I was just looking for a call to restoreToCount... in case the client
> code left the stack unbalanced by more than 1

Another look?

[reed1, 2013-10-14 15:01:58]

lgtm

[commit-bot: I haz the power, 2013-10-14 15:03:53]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/26848013/19001

[commit-bot: I haz the power, 2013-10-14 15:13:28]

Retried try job too often on Build-Ubuntu12-GCC-x86_64-Release-Trybot for
step(s) BuildBench, BuildEverything, BuildGm, BuildSkiaLib, BuildTests,
BuildTools
http://skiabot-master.pogerlabs.com:10117/buildstatus?builder=Build-Ubuntu12-...

[commit-bot: I haz the power, 2013-10-14 15:18:33]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/mtklein@google.com/26848013/17001

[commit-bot: I haz the power, 2013-10-14 15:28:04]

Change committed as 11755