SkData to SkStreamAsset to avoid unneeded copying

SkData to SkStreamAsset to avoid unneeded copying

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

[hal.canary, 2014-08-25 21:34:10]

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

[hal.canary, 2014-08-25 21:34:10]

ptal

[mtklein, 2014-08-25 21:44:34]

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFRasterizeTask.h
File dm/DMPDFRasterizeTask.h (right):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFRasterizeTask.h#newc...
dm/DMPDFRasterizeTask.h:24: // takes ownership of SkStreamAsset.
I'm finding this hard to think about.

Can we not just use SkRef and SkAutoTUnref as normal?

If you need to pass exclusive SkStreamAssets around, let's do that consistently
everywhere and use SkAutoTDelete instead of SkAutoTUnref.

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp
File dm/DMPDFTask.cpp (left):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp#oldcode57
dm/DMPDFTask.cpp:57: return fWriteStream.copyToData();
Is this the copy we're avoiding?

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp
File dm/DMPDFTask.cpp (right):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp#newcode89
dm/DMPDFTask.cpp:89: this->spawnChild(SkNEW_ARGS(WriteTask, (*this,
pdfData.detach(), ".pdf")));
What's the cost of duplicate()?  It's weird one kid gets a duplicate and the
other gets the original.

[hal.canary, 2014-08-26 15:20:51]

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFRasterizeTask.h
File dm/DMPDFRasterizeTask.h (right):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFRasterizeTask.h#newc...
dm/DMPDFRasterizeTask.h:24: // takes ownership of SkStreamAsset.
On 2014/08/25 21:44:33, mtklein wrote:
> I'm finding this hard to think about.

SkStreams should not be reference counted, since it doesn't make sense to share
them.  Ask Ben about his attempt to change that.

> Can we not just use SkRef and SkAutoTUnref as normal?

It doesn't make sense because we are asking for exclusive ownership.  I do this
on every function I write that takes a SkStream and produces a possibly
long-lived object.  And I always note this in the documentation.
 
> If you need to pass exclusive SkStreamAssets around, let's do 
> that consistently everywhere and use SkAutoTDelete instead of
> SkAutoTUnref.

I would be happy with that change, but the last time I tried that, the reviewer
asked me to change it back to AutoUnref.

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp
File dm/DMPDFTask.cpp (left):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp#oldcode57
dm/DMPDFTask.cpp:57: return fWriteStream.copyToData();
On 2014/08/25 21:44:34, mtklein wrote:
> Is this the copy we're avoiding?

Yes.  copyToData() is O(N) while detachAsStream() is O(1).

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp
File dm/DMPDFTask.cpp (right):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp#newcode89
dm/DMPDFTask.cpp:89: this->spawnChild(SkNEW_ARGS(WriteTask, (*this,
pdfData.detach(), ".pdf")));
On 2014/08/25 21:44:33, mtklein wrote:
> What's the cost of duplicate()? 

The duplicate is cheap.  It's only a SkNEW SkBlockMemoryStream and a ref() on
the SkBlockMemoryRefCnt.

> It's weird one kid gets a duplicate and the
> other gets the original.

I had one object and I need two.  The simplest thing is to make one copy.  I
could make two copies and delete the original, but why?

[mtklein, 2014-08-26 15:30:54]

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFRasterizeTask.h
File dm/DMPDFRasterizeTask.h (right):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFRasterizeTask.h#newc...
dm/DMPDFRasterizeTask.h:24: // takes ownership of SkStreamAsset.
> I would be happy with that change, but the last time I tried that, the
reviewer
> asked me to change it back to AutoUnref.

Gotcha.  Let's make it SkAutoTDelete then.  Your explanation says that's the
right thing to do.

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp
File dm/DMPDFTask.cpp (right):

https://codereview.chromium.org/502193002/diff/1/dm/DMPDFTask.cpp#newcode89
dm/DMPDFTask.cpp:89: this->spawnChild(SkNEW_ARGS(WriteTask, (*this,
pdfData.detach(), ".pdf")));
> I had one object and I need two.  The simplest thing is to make one copy.  I
> could make two copies and delete the original, but why?

Just to make it a little more consistent and resilient to unrelated code
changes.  If I swapped the order of child calls or made WriteTask conditional,
things would break.  

We can certainly think through the code and determine it's safe now, but it'd be
even nicer for us to not have to think in the future and just glance to see it's
correct.

[hal.canary, 2014-08-26 15:59:16]

On 2014/08/26 15:30:54, mtklein wrote:
> Just to make it a little more consistent and resilient to unrelated code
> changes.  If I swapped the order of child calls or made WriteTask conditional,
> things would break.  

Done

> We can certainly think through the code and determine it's safe now, but it'd
be
> even nicer for us to not have to think in the future and just glance to see
it's
> correct.

[mtklein, 2014-08-26 17:10:57]

lgtm

https://codereview.chromium.org/502193002/diff/20001/dm/DMPDFTask.cpp
File dm/DMPDFTask.cpp (right):

https://codereview.chromium.org/502193002/diff/20001/dm/DMPDFTask.cpp#newcode69
dm/DMPDFTask.cpp:69: SkAutoTUnref<SkStreamAsset> pdfData;
AutoTDelete here too?

[hal.canary, 2014-08-26 17:30:48]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2014-08-26 17:31:05]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/halcanary@google.com/502193002/40001

[commit-bot: I haz the power, 2014-08-26 17:38:13]

Committed patchset #3 (40001) as a4c6094177ebde18c706cbcfbd2013d0a088e0ee