Souped-up SkTextBlob (reland)

Souped-up SkTextBlob.

Refactored text blob backend for improved performance: instead of using
separate buffers for runs/positions/glyphs, everything is now packed in
a consolidated slab (including the SkTextBlob object itself!).

Benefits:

 * number of allocations per blob construction reduced from ~4 to 1
   (also minimizes internal fragmentation)
 * run record size reduced by 8 bytes

This takes the blob construction overhead down to negligible levels
(for the current Blink uncached textblob implementation).

Unfortunately, the code is much more finicky (run merging in
particular) -- hence the assert spree.

Multi-run blobs are vulnerable to realloc storms but this is not a
problem at the moment because Blink is using one-run blobs 99% of the
time. Will be addressed in the future.


R=reed@google.com,mtklein@google.com,robertphillips@google.com

Committed: https://skia.googlesource.com/skia/+/13645ea0ea87038ebd71be3bd6d53b313069a9e4

Committed: https://skia.googlesource.com/skia/+/3c196def91726913a417e703ac482bb2dbbfff27

[mtklein, 2014-09-19 14:33:03]

https://codereview.chromium.org/581173003/diff/20001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/581173003/diff/20001/include/core/SkTextBlob....
include/core/SkTextBlob.h:95: 
// There are more fields following this, see the .cpp.

https://codereview.chromium.org/581173003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/581173003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:62: static size_t StorageSize(int glyphCount,
SkTextBlob::GlyphPositioning positioning) {
Let's use SkAlignPtr.  RunRecord contains pointers, so SkAlign4 might only be
half-aligned.  This is usually fine, but chrome's LSAN setup checks for leaks
before we're destroyed, and it won't see the pointers as live pointers.

https://codereview.chromium.org/581173003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:344: fStorage.realloc(fStorageSize);
// FYI: This relies on everything we store being relocatable, particularly
SkPaint.

[f(malita), 2014-09-19 15:42:36]

https://codereview.chromium.org/581173003/diff/20001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/581173003/diff/20001/include/core/SkTextBlob....
include/core/SkTextBlob.h:95: 
On 2014/09/19 14:33:02, mtklein wrote:
> // There are more fields following this, see the .cpp.

Done.

https://codereview.chromium.org/581173003/diff/20001/src/core/SkTextBlob.cpp
File src/core/SkTextBlob.cpp (right):

https://codereview.chromium.org/581173003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:62: static size_t StorageSize(int glyphCount,
SkTextBlob::GlyphPositioning positioning) {
On 2014/09/19 14:33:02, mtklein wrote:
> Let's use SkAlignPtr.  RunRecord contains pointers, so SkAlign4 might only be
> half-aligned.  This is usually fine, but chrome's LSAN setup checks for leaks
> before we're destroyed, and it won't see the pointers as live pointers.

Done.

https://codereview.chromium.org/581173003/diff/20001/src/core/SkTextBlob.cpp#...
src/core/SkTextBlob.cpp:344: fStorage.realloc(fStorageSize);
On 2014/09/19 14:33:02, mtklein wrote:
> // FYI: This relies on everything we store being relocatable, particularly
> SkPaint.

Done.

[mtklein, 2014-09-19 15:43:50]

lgtm

[jbroman, 2014-09-19 15:47:50]

Out of curiosity, any numbers on what the impact of this is
on performance or memory in practice?

[f(malita), 2014-09-19 15:53:21]

On 2014/09/19 15:47:50, jbroman wrote:
> Out of curiosity, any numbers on what the impact of this is
> on performance or memory in practice?

For local wikipedia telemetry runs, blob recording overhead went down from 3-4%
to ~1%. I don't have memory data yet.

[reed1, 2014-09-19 16:49:02]

lgtm

https://codereview.chromium.org/581173003/diff/40001/include/core/SkTextBlob.h
File include/core/SkTextBlob.h (right):

https://codereview.chromium.org/581173003/diff/40001/include/core/SkTextBlob....
include/core/SkTextBlob.h:38: void flatten(SkWriteBuffer&) const;
Does this have to be public? Same question for CreateFromBuffer...

[f(malita), 2014-09-19 18:08:11]

On 2014/09/19 16:49:02, reed1 wrote:
> https://codereview.chromium.org/581173003/diff/40001/include/core/SkTextBlob.h
> File include/core/SkTextBlob.h (right):
> 
>
https://codereview.chromium.org/581173003/diff/40001/include/core/SkTextBlob....
> include/core/SkTextBlob.h:38: void flatten(SkWriteBuffer&) const;
> Does this have to be public?

We could hide it by making 2-3 more friends.

> Same question for CreateFromBuffer...

This one is used in a static function (SkGPipeRead:drawTextBlob_rp) so it's
harder to hide.

We can take a look at these in a follow up.

[f(malita), 2014-09-20 01:32:33]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-09-20 01:32:51]

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

[commit-bot: I haz the power, 2014-09-20 01:37:04]

Committed patchset #3 (id:40001) as 13645ea0ea87038ebd71be3bd6d53b313069a9e4

[f(malita), 2014-09-20 02:02:24]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/588853002/ by fmalita@chromium.org.

The reason for reverting is: Broke the new blobshader gm..

[f(malita), 2014-09-20 12:33:17]

On 2014/09/20 02:02:24, Florin Malita wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.chromium.org/588853002/ by mailto:fmalita@chromium.org.
> 
> The reason for reverting is: Broke the new blobshader gm.

Had forgotten to update a size-related assert after ptr-aligning runrecords.
Relanding.

[f(malita), 2014-09-20 12:34:04]

The CQ bit was checked by fmalita@chromium.org

[commit-bot: I haz the power, 2014-09-20 12:34:40]

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

[commit-bot: I haz the power, 2014-09-20 12:40:27]

Committed patchset #4 (id:60001) as 3c196def91726913a417e703ac482bb2dbbfff27