SkPDF: Memory improvements for PDF Streams

SkPDF: Memory improvements for PDF Streams

    ΔMEM = -15%
    ΔCPU = -7%

BUG=skia:3030
Committed: https://skia.googlesource.com/skia/+/725c62054354dc0c7fb699bcd69687ec78083ae8

[hal.canary, 2015-07-10 21:10:54]

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

[hal.canary, 2015-07-10 21:10:54]

You asked for something to review.

[hal.canary, 2015-07-10 21:56:17]

The CQ bit was checked by halcanary@google.com

[commit-bot: I haz the power, 2015-07-10 21:56:37]

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

[commit-bot: I haz the power, 2015-07-10 21:56:39]

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 2015-07-11 03:56 UTC

[commit-bot: I haz the power, 2015-07-11 03:56:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-11 03:56:02]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[hal.canary, 2015-08-14 21:53:14]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-14 21:53:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227913008/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1227913008/40001

[hal.canary, 2015-08-14 21:53:49]

On 2015/07/11 03:56:02, commit-bot: I haz the power wrote:
> No LGTM from a valid reviewer yet. Please ask for an LGTM from a
> full Skia committer from
> https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

Ping.

[commit-bot: I haz the power, 2015-08-14 22:00:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-14 22:00:29]

Dry run: This issue passed the CQ dry run.

[hal.canary, 2015-08-18 20:38:24]

The CQ bit was checked by halcanary@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-08-18 20:38:33]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227913008/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1227913008/60001

[hal.canary, 2015-08-18 20:46:41]

There.  I made this CL much smaller now.

[commit-bot: I haz the power, 2015-08-19 13:07:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-19 13:07:28]

Dry run: This issue passed the CQ dry run.

[tomhudson, 2015-08-19 19:27:51]

lgtm

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:1087: descriptor->insertObjRef("FontFile2",
fontStream.detach());
The code duplication between here and (canSubset() && rawStream) is unfortunate.

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:1096: fontStream->dict()->insertName("Subtype", "Type1C");
Is the addition of dict() a bugfix? A noop? An intentional change that is part
of this CL?

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFStream.cpp...
src/pdf/SkPDFStream.cpp:24: SkAutoTDelete<SkStreamRewindable>
dup(fCompressedData->duplicate());
This looks expensive, but I assume the writing is non-const? Maybe the duplicate
is copy-on-write?

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFStream.h
File src/pdf/SkPDFStream.h (left):

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFStream.h#o...
src/pdf/SkPDFStream.h:52: kCompressed_State,     //!< The stream's already been
compressed.
Presumably we're always keeping the stream compressed now? Would be nice to have
some information in the commit message about the mechanism of the change, as
well as the deltas to footprint & performance.

[hal.canary, 2015-08-19 19:34:20]

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFFont.cpp
File src/pdf/SkPDFFont.cpp (right):

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFFont.cpp#n...
src/pdf/SkPDFFont.cpp:1096: fontStream->dict()->insertName("Subtype", "Type1C");
On 2015/08/19 19:27:51, tomhudson wrote:
> Is the addition of dict() a bugfix? A noop? An intentional change that is part
> of this CL?

I intentionally do not inherit from SkPDFDict anymore.

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFStream.cpp
File src/pdf/SkPDFStream.cpp (right):

https://codereview.chromium.org/1227913008/diff/60001/src/pdf/SkPDFStream.cpp...
src/pdf/SkPDFStream.cpp:24: SkAutoTDelete<SkStreamRewindable>
dup(fCompressedData->duplicate());
On 2015/08/19 19:27:51, tomhudson wrote:
> This looks expensive, but I assume the writing is non-const? Maybe the
duplicate
> is copy-on-write?

The duplicate just refs the underlying SkBlockMemoryRefCnt.  See
SkBlockMemoryStream::duplicate().  This is cheap.

[tomhudson, 2015-08-20 15:02:42]

The CQ bit was checked by tomhudson@google.com

[tomhudson, 2015-08-20 15:02:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tomhudson@google.com
Link to the patchset: https://codereview.chromium.org/1227913008/#ps80001
(title: "2015-08-19 (Wednesday) 17:47:45 EDT")

[commit-bot: I haz the power, 2015-08-20 15:02:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1227913008/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1227913008/80001

[commit-bot: I haz the power, 2015-08-20 15:09:40]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/725c62054354dc0c7fb699bcd69687ec78083ae8