4x allocation in PipeController is probably overkill.

4x allocation in PipeController is probably overkill.

When verylargebitmap GM runs in cross-process pipe mode, we're
requestBlock()ing ~200M to carry the bitmaps.  The current
implementation ends up allocating ~800M, which is a bit wasteful.

SkGPipeWrite already rounds up to 16K, so just rely on that.

This change exposed several bugs in pipe:
  - we don't reserve enough space in drawVertices
  - we don't reserve enough space for factory names in cross-process mode
  - we don't quite have the right check in needOpBytes to see if we needed to send off the current block and allocate a new one

SETUP_NOTIFY and generally calling doNotify() more often than necessary made things hard to debug and understand.  Now the pipe always waits to send off its current block until it needs more space than that block can provide, or it's the final block.  We can put these back if we need the proactive flushing, but it seems not necessary?

Removed an assert in DeferredCanvasTest, which is somtimes 2 (Debug), sometimes 3 (Release).  It seemed like the other asserts were more essential, and this one was more of a white-box assertion.  Still sound if we remove it?

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

[mtklein, 2014-05-02 15:01:42]

On 2014/05/02 14:50:04, mtklein wrote:

Actually, 4 extra bytes doesn't look like it's enough.  Digging in now to see
what's up.  ASAN FTW.

[mtklein, 2014-05-02 15:24:28]

On 2014/05/02 15:01:42, mtklein wrote:
> On 2014/05/02 14:50:04, mtklein wrote:
> 
> Actually, 4 extra bytes doesn't look like it's enough.  Digging in now to see
> what's up.  ASAN FTW.

128 bytes of extra space makes ASAN happy with DM, but I'm not really feeling
warm and snuggly.  We must be missing some calls to needOpBytes?

This wasn't hitting us before because:
  - SkGPipeWrite first maxed its request up to 16K if it was small
  - PipeController multiplied that 16K by 4
  - so there were always at least 3*16K == 48K bytes of padding, well above 128.

I've moved the 128 byte padding to SkGPipeWrite, where the problem is more
likely to be, and I've removed the double-round up check in PipeController as
SkGPipeWrite's already doing it.

[scroggo, 2014-05-02 16:15:42]

lgtm

https://codereview.chromium.org/267863002/diff/40001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/267863002/diff/40001/src/pipe/SkGPipeWrite.cp...
src/pipe/SkGPipeWrite.cpp:482: blockSize += 128;  // TODO(mtklein, scroggo): why
do we need this extra padding?
Might we be passing too small a number into needOpBytes?

[mtklein, 2014-05-02 19:02:01]

Please take (another) look.  I've got ASAN happy now.

[scroggo, 2014-05-02 20:02:59]

> SETUP_NOTIFY and generally calling doNotify() more often
> than necessary made things hard to debug and understand.
> Now the pipe always waits to send off its current block
> until it needs more space than that block can provide,
> or it's the final block. We can put these back if we
> need the proactive flushing, but it seems not necessary? 

I'm not sure how often is necessary. Maybe it's best to
leave that question up to the client, as you've effectively
done here. DeferredCanvas is the only existing client, so if
this is fine with junov, this lgtm.

https://codereview.chromium.org/267863002/diff/100001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/267863002/diff/100001/src/pipe/SkGPipeWrite.c...
src/pipe/SkGPipeWrite.cpp:751: // This needs to run first so it's calls to
needOpBytes and doNotify
its*

[mtklein, 2014-05-02 20:20:55]

https://codereview.chromium.org/267863002/diff/100001/src/pipe/SkGPipeWrite.cpp
File src/pipe/SkGPipeWrite.cpp (right):

https://codereview.chromium.org/267863002/diff/100001/src/pipe/SkGPipeWrite.c...
src/pipe/SkGPipeWrite.cpp:751: // This needs to run first so it's calls to
needOpBytes and doNotify
On 2014/05/02 20:02:59, scroggo wrote:
> its*

Done.

[Justin Novosad, 2014-05-06 17:08:13]

https://codereview.chromium.org/267863002/diff/120001/tests/DeferredCanvasTes...
File tests/DeferredCanvasTest.cpp (left):

https://codereview.chromium.org/267863002/diff/120001/tests/DeferredCanvasTes...
tests/DeferredCanvasTest.cpp:550: REPORTER_ASSERT(reporter, 2 ==
notificationCounter.fStorageAllocatedChangedCount);
Could we keep this assert and put it inside "#ifdef SK_DEBUG"?

[mtklein, 2014-05-06 17:36:42]

https://codereview.chromium.org/267863002/diff/120001/tests/DeferredCanvasTes...
File tests/DeferredCanvasTest.cpp (left):

https://codereview.chromium.org/267863002/diff/120001/tests/DeferredCanvasTes...
tests/DeferredCanvasTest.cpp:550: REPORTER_ASSERT(reporter, 2 ==
notificationCounter.fStorageAllocatedChangedCount);
On 2014/05/06 17:08:14, junov wrote:
> Could we keep this assert and put it inside "#ifdef SK_DEBUG"?

You bet.  Now testing both sides.

[mtklein, 2014-05-07 15:16:39]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-05-07 15:17:42]

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

[commit-bot: I haz the power, 2014-05-07 15:27:12]

Change committed as 14613

[mtklein, 2014-05-14 13:33:01]

A revert of this CL has been created in
https://codereview.chromium.org/285943002/ by mtklein@google.com.

The reason for reverting is: ~29% perf regression in Chrome's
Animation_balls-canvas bench on Macs.  Going to cut up and reland in pieces.

https://code.google.com/p/chromium/issues/detail?id=372671.