Defer saves() until they're needed

Defer saves() until they're needed

patch from issue 759443006 at patchset 40001 (http://crrev.com/759443006#ps40001)

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/2ff1fcede1e9525285c5de1f35fb2dcb0fab32bd

[reed1, 2014-12-01 20:34:38]

reed@google.com changed reviewers:
+ fmalita@google.com, mtklein@google.com, robertphillips@google.com

[reed1, 2014-12-01 20:34:39]

I understand Florin's alternative, and will try to refactor this one to isolate
just the differences between his counter approach and this stack approach.

[reed1, 2014-12-01 20:49:25]

My motivation (last week) for trying this was a perceived perf hit in sampleapp
when using more pictures. The profile showed a huge cost for all of the
mallocs/news. This CL seems to cut that perf hit down by half!

That said, the above measurement was (accidentally) done with gobs of
guard-pages/scribble options on in XCode, magnifying the cost of each malloc.
With these disabled, the perceived hit for mallocs is greatly diminished.

Summary: I think lazy saves are still interesting, but perhaps not in any
obvious perf way, so I am relaxing the priority of pursuing this for now.

[reed1, 2014-12-10 18:30:56]

PTAL

should I just remove the #if 0 tests, or can they robustly be redone?

[reed1, 2014-12-10 18:31:05]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-12-10 18:32:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/767333002/40001

[commit-bot: I haz the power, 2014-12-10 18:32:05]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-12-11 00:32 UTC

[reed1, 2014-12-10 18:32:20]

The CQ bit was unchecked by reed@google.com

[robertphillips, 2014-12-10 18:43:53]

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:823: fCallingWillSave = false;
Do we need this cast ?

https://codereview.chromium.org/767333002/diff/40001/tests/RecordDrawTest.cpp
File tests/RecordDrawTest.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:57: recorder.save();  // We won't balance this, but
SkRecordDraw will for us.
Add a draw here to bring this test back?

https://codereview.chromium.org/767333002/diff/40001/tests/RecordDrawTest.cpp...
tests/RecordDrawTest.cpp:204: 
The main point of this test is that only r2 gets drawn. We can just remove the
checks for the Save and Restore and re-enable this test.

[robertphillips, 2014-12-10 19:04:22]

https://codereview.chromium.org/767333002/diff/40001/tests/RecordOptsTest.cpp
File tests/RecordOptsTest.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/tests/RecordOptsTest.cpp...
tests/RecordOptsTest.cpp:19: #if 0
Probably remove & add explicit deferred save test

[f(malita), 2014-12-10 19:49:32]

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

[f(malita), 2014-12-10 19:49:33]

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:184: int             fDeferredSaveCount;
I always imagined this to be an unsigned, but yeah, I know - Skia counts are
ints :)

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:799: int SkCanvas::getSaveCount() const {
To avoid perf surprises, we should probably track save_count using a dedicated
counter. Then we could use something like this to validate the stack.

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:803: const MCRec* rec = (const MCRec*)iter.next();
I think this will miss all deferred saves on the initial MCRec, no?

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:809: return count;
Would it be simpler to just iterate over all MCRecs and then just return (count
- 1) here?

[reed1, 2014-12-10 20:06:36]

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:799: int SkCanvas::getSaveCount() const {
On 2014/12/10 19:49:33, Florin Malita wrote:
> To avoid perf surprises, we should probably track save_count using a dedicated
> counter. Then we could use something like this to validate the stack.

Probably true :(

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:803: const MCRec* rec = (const MCRec*)iter.next();
On 2014/12/10 19:49:33, Florin Malita wrote:
> I think this will miss all deferred saves on the initial MCRec, no?

next() returns the first one : its a funny pattern -- next is curr/next/done all
in one.

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:809: return count;
On 2014/12/10 19:49:33, Florin Malita wrote:
> Would it be simpler to just iterate over all MCRecs and then just return
(count
> - 1) here?

see above

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:823: fCallingWillSave = false;
On 2014/12/10 18:43:53, robertphillips wrote:
> Do we need this cast ?

Done.

[f(malita), 2014-12-10 20:13:58]

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:809: return count;
On 2014/12/10 20:06:35, reed1 wrote:
> On 2014/12/10 19:49:33, Florin Malita wrote:
> > Would it be simpler to just iterate over all MCRecs and then just return
> (count
> > - 1) here?
> 
> see above

Hmm, then something's not clicking: do we need to subtract 1 here?

(a fresh canvas starts with one MCRec and fDeferredSave == 0 => this method
returns 1?)

[f(malita), 2014-12-10 20:18:18]

On 2014/12/10 20:13:58, Florin Malita wrote:
> https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp
> File src/core/SkCanvas.cpp (right):
> 
>
https://codereview.chromium.org/767333002/diff/40001/src/core/SkCanvas.cpp#ne...
> src/core/SkCanvas.cpp:809: return count;
> On 2014/12/10 20:06:35, reed1 wrote:
> > On 2014/12/10 19:49:33, Florin Malita wrote:
> > > Would it be simpler to just iterate over all MCRecs and then just return
> > (count
> > > - 1) here?
> > 
> > see above
> 
> Hmm, then something's not clicking: do we need to subtract 1 here?
> 
> (a fresh canvas starts with one MCRec and fDeferredSave == 0 => this method
> returns 1?)

Never mind, my save count model was off (it's 1-based).

[reed1, 2014-12-10 20:42:45]

https://codereview.chromium.org/767333002/diff/40001/tests/RecordOptsTest.cpp
File tests/RecordOptsTest.cpp (right):

https://codereview.chromium.org/767333002/diff/40001/tests/RecordOptsTest.cpp...
tests/RecordOptsTest.cpp:19: #if 0
On 2014/12/10 19:04:22, robertphillips wrote:
> Probably remove & add explicit deferred save test

Done.

[f(malita), 2014-12-10 20:54:19]

lgtm % getSaveCount() perf anxiety :)

[reed1, 2014-12-10 21:26:18]

as many tests updated as I knew how to. The remaining I've left as #if 0 for
future archeologists.

[reed1, 2014-12-10 21:26:28]

ptal

[robertphillips, 2014-12-11 13:24:37]

https://codereview.chromium.org/767333002/diff/100001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:816: fMCRec->fDeferredSaveCount += 1;
rm this ?

https://codereview.chromium.org/767333002/diff/100001/tests/PictureBBHTest.cpp
File tests/PictureBBHTest.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/tests/PictureBBHTest.cp...
tests/PictureBBHTest.cpp:121: // Buggy implementations might return 1 (just
Clip) or 3 (Clip - Save - Restore).
Yes.

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
File tests/RecordPatternTest.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:92: 
I think this test is dead - remove it ?

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:129: 
This test should be pretty easy to bring back to life

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:140: 
Just remove this first test

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:145: recorder.save();
clipRect

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:146: recorder.save();
clipRect

https://codereview.chromium.org/767333002/diff/100001/tests/RecordReplaceDraw...
File tests/RecordReplaceDrawTest.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/tests/RecordReplaceDraw...
tests/RecordReplaceDrawTest.cpp:78: canvas->save();
Won't this not test anything unless we put a clipRect here?

https://codereview.chromium.org/767333002/diff/100001/tests/RecordReplaceDraw...
tests/RecordReplaceDrawTest.cpp:89: REPORTER_ASSERT(r, (rerecord.count() & 1) ==
0);
remove this dead code?

[reed1, 2014-12-11 14:54:07]

reed@google.com changed required reviewers:
+ robertphillips@google.com

[reed1, 2014-12-11 14:54:08]

https://codereview.chromium.org/767333002/diff/100001/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:816: fMCRec->fDeferredSaveCount += 1;
On 2014/12/11 13:24:36, robertphillips wrote:
> rm this ?

Done.

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
File tests/RecordPatternTest.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:92: 
On 2014/12/11 13:24:36, robertphillips wrote:
> I think this test is dead - remove it ?

Done.

https://codereview.chromium.org/767333002/diff/100001/tests/RecordPatternTest...
tests/RecordPatternTest.cpp:129: 
On 2014/12/11 13:24:36, robertphillips wrote:
> This test should be pretty easy to bring back to life

Done.

https://codereview.chromium.org/767333002/diff/100001/tests/RecordReplaceDraw...
File tests/RecordReplaceDrawTest.cpp (right):

https://codereview.chromium.org/767333002/diff/100001/tests/RecordReplaceDraw...
tests/RecordReplaceDrawTest.cpp:78: canvas->save();
On 2014/12/11 13:24:37, robertphillips wrote:
> Won't this not test anything unless we put a clipRect here?

Done.

https://codereview.chromium.org/767333002/diff/100001/tests/RecordReplaceDraw...
tests/RecordReplaceDrawTest.cpp:89: REPORTER_ASSERT(r, (rerecord.count() & 1) ==
0);
On 2014/12/11 13:24:37, robertphillips wrote:
> remove this dead code?

Done.

[reed1, 2014-12-11 14:54:12]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-12-11 14:54:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/767333002/120001

[commit-bot: I haz the power, 2014-12-11 14:54:34]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2014-12-11 20:54 UTC

[robertphillips, 2014-12-11 15:07:07]

lgtm

[commit-bot: I haz the power, 2014-12-11 15:07:42]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/2ff1fcede1e9525285c5de1f35fb2dcb0fab32bd

[scroggo, 2014-12-11 15:31:35]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/798593003/ by scroggo@google.com.

The reason for reverting is: Failing tests:
http://chromegw.corp.google.com/i/client.skia/builders/Test-Win8-ShuttleA-GTX....