Switch to one single bitmap text blob cache allocation

Switch to one single bitmap text blob cache allocation

BUG=skia:

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

[joshualitt, 2015-03-31 20:01:10]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, joshualitt@google.com, jvanverth@google.com

[joshualitt, 2015-03-31 20:01:10]

This change drops down to more or less a single alloc for the whole cached text
blob.

[bsalomon, 2015-03-31 21:19:11]

initial thoughts

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkDescriptor.h
File src/core/SkDescriptor.h (right):

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkDescriptor.h...
src/core/SkDescriptor.h:149: if (size <= sizeof(fStorage)) {
don't you need to free the old one here?

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1722: SkDescriptor*       desc = ad->getDesc();
probably don't need the extra spaces anymore.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:113: inline void
GrBitmapTextContextB::BlobGlyphCount(int* glyphCount, int* runCount,
if this only uses public interfaces on textblob, does it need to be a member
function? should it be a helper on textblob?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:121: inline
GrBitmapTextContextB::BitmapTextBlob*
you really think this should be inlined?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:125: size_t verticesCount = glyphCount *
kVerticesPerGlyph * kGrayTextVASize;
static assert that kGrayTextVASize is the largest?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:217: SkGlyphCache* cache =
setupCache(&cacheBlob->fRuns[run], runPaint, viewMatrix);
this->

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:274: SkGlyphCache* cache =
setupCache(&blob->fRuns[0], skPaint, viewMatrix);
this->

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:385: SkGlyphCache* cache =
setupCache(&blob->fRuns[0], skPaint, viewMatrix);
this->

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:69: // One additional point, each run can contain
glyphs with any of the three mask formats.
specify in comments that these are "subruns" to make connection to code clearer?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:77: struct PerSubRunInfo {
SubRunInfo?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:91: SkSTArray<1, PerSubRunInfo, true> fSubRunInfo;
Does this max out at 3? Maybe just always have an array of 3?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:96: bool fInitialized;
can you use another param for this rather than adding a field?
fDescriptor.isInitialized() (which would be easy to write).

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:127: void* operator new(size_t, void* p) { return
p; }
some compilers will yell at you for not overriding delete if you override new.

[joshualitt, 2015-04-01 13:21:18]

Feedback incorporated

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkDescriptor.h
File src/core/SkDescriptor.h (right):

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkDescriptor.h...
src/core/SkDescriptor.h:149: if (size <= sizeof(fStorage)) {
On 2015/03/31 21:19:10, bsalomon wrote:
> don't you need to free the old one here?

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkPaint.cpp
File src/core/SkPaint.cpp (right):

https://codereview.chromium.org/1041953002/diff/80001/src/core/SkPaint.cpp#ne...
src/core/SkPaint.cpp:1722: SkDescriptor*       desc = ad->getDesc();
On 2015/03/31 21:19:10, bsalomon wrote:
> probably don't need the extra spaces anymore.

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.cpp (right):

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:113: inline void
GrBitmapTextContextB::BlobGlyphCount(int* glyphCount, int* runCount,
On 2015/03/31 21:19:10, bsalomon wrote:
> if this only uses public interfaces on textblob, does it need to be a member
> function? should it be a helper on textblob?

see comment in header

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:121: inline
GrBitmapTextContextB::BitmapTextBlob*
On 2015/03/31 21:19:10, bsalomon wrote:
> you really think this should be inlined?

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:125: size_t verticesCount = glyphCount *
kVerticesPerGlyph * kGrayTextVASize;
On 2015/03/31 21:19:10, bsalomon wrote:
> static assert that kGrayTextVASize is the largest?

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:217: SkGlyphCache* cache =
setupCache(&cacheBlob->fRuns[run], runPaint, viewMatrix);
On 2015/03/31 21:19:10, bsalomon wrote:
> this->

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:274: SkGlyphCache* cache =
setupCache(&blob->fRuns[0], skPaint, viewMatrix);
On 2015/03/31 21:19:10, bsalomon wrote:
> this->

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.cpp:385: SkGlyphCache* cache =
setupCache(&blob->fRuns[0], skPaint, viewMatrix);
On 2015/03/31 21:19:10, bsalomon wrote:
> this->

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:69: // One additional point, each run can contain
glyphs with any of the three mask formats.
On 2015/03/31 21:19:10, bsalomon wrote:
> specify in comments that these are "subruns" to make connection to code
clearer?

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:77: struct PerSubRunInfo {
On 2015/03/31 21:19:11, bsalomon wrote:
> SubRunInfo?

Acknowledged.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:91: SkSTArray<1, PerSubRunInfo, true> fSubRunInfo;
On 2015/03/31 21:19:11, bsalomon wrote:
> Does this max out at 3? Maybe just always have an array of 3?

No, we can have as many subruns as there are glyphs in a run.  Let me know if
the updated comment above does not clarify things.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:96: bool fInitialized;
On 2015/03/31 21:19:11, bsalomon wrote:
> can you use another param for this rather than adding a field?
> fDescriptor.isInitialized() (which would be easy to write).

Unfortunately I can't just use the descriptor because it is initialized before
the mask.  I could use fSubRunInfo.count(), but it would really complicate how I
have to handle 'handing off' from one run to the next, ie, if there are no
glyphs in a given run, the subrun could be empty, so when I go to write forward
start vertices / indices, I'd have to put in logic to handle the empty case.  I
could handle this by putting start / end vertices / indices on each run as well
as each subrun, but it seems easier to just use a bool.

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:127: void* operator new(size_t, void* p) { return
p; }
On 2015/03/31 21:19:11, bsalomon wrote:
> some compilers will yell at you for not overriding delete if you override new.

Acknowledged.  I'm not sure if I did the delete right though.

[bsalomon, 2015-04-01 15:41:46]

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:96: bool fInitialized;
On 2015/04/01 13:21:18, joshualitt wrote:
> On 2015/03/31 21:19:11, bsalomon wrote:
> > can you use another param for this rather than adding a field?
> > fDescriptor.isInitialized() (which would be easy to write).
> 
> Unfortunately I can't just use the descriptor because it is initialized before
> the mask.  I could use fSubRunInfo.count(), but it would really complicate how
I
> have to handle 'handing off' from one run to the next, ie, if there are no
> glyphs in a given run, the subrun could be empty, so when I go to write
forward
> start vertices / indices, I'd have to put in logic to handle the empty case. 
I
> could handle this by putting start / end vertices / indices on each run as
well
> as each subrun, but it seems easier to just use a bool.

I'm not sure I totally follow... but maybe this makes sense:

Why not skip the push_back in the constructor and have:

if (run.fSubRunInfo.isEmpty() || run.fSubRunInfo.back().fMaskFormat != format) {
    SubRunInfo* newSubRun = &run.fSubRunInfo.push_back();
    // ...
}
SubRunInfo* subRunInfo = &run.fSubRunInfo.back();

Seems simpler, no slower, and gets rid of the bool. Is it useful elsewhere to
have a subrun that has no glyphs?

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:127: void* operator new(size_t, void* p) { return
p; }
On 2015/04/01 13:21:18, joshualitt wrote:
> On 2015/03/31 21:19:11, bsalomon wrote:
> > some compilers will yell at you for not overriding delete if you override
new.
> 
> Acknowledged.  I'm not sure if I did the delete right though.

Now that I look more closely, why did you need to override new/delete at all?

https://codereview.chromium.org/1041953002/diff/120001/src/core/SkDescriptor.h
File src/core/SkDescriptor.h (right):

https://codereview.chromium.org/1041953002/diff/120001/src/core/SkDescriptor....
src/core/SkDescriptor.h:177: size_t          fHeapAllocSize;
Is this really worth the complexity? Seems much simpler to just have

reset(size_t size) {
    if (fDesc != (SkDescriptor*)(void*)fStorage) {
        SkDescriptor::Free(fDesc);
    }
    if (size <= sizeof(fStorage) {
        fDesc = (SkDescriptor*)(void*)fStorage;
    } else {
        fDesc = SkDescriptor::Alloc(size);
    }
}

and ~ can stay as it is. Do you expect to be reinit'ing these frequently where
the  size is larger than fStorage both before and after?

https://codereview.chromium.org/1041953002/diff/120001/src/gpu/GrBitmapTextCo...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1041953002/diff/120001/src/gpu/GrBitmapTextCo...
src/gpu/GrBitmapTextContext.h:70: // We call these SubRuns.  Because a subrun
must be a contiguous range, we have to create
Is it really true that we need to create a subrun at every transition rather
than one subrun per mask type? Are we concerned about preserving order within a
run?

[joshualitt, 2015-04-01 16:53:32]

On 2015/04/01 15:41:46, bsalomon wrote:
>
https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
> File src/gpu/GrBitmapTextContext.h (right):
> 
>
https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
> src/gpu/GrBitmapTextContext.h:96: bool fInitialized;
> On 2015/04/01 13:21:18, joshualitt wrote:
> > On 2015/03/31 21:19:11, bsalomon wrote:
> > > can you use another param for this rather than adding a field?
> > > fDescriptor.isInitialized() (which would be easy to write).
> > 
> > Unfortunately I can't just use the descriptor because it is initialized
before
> > the mask.  I could use fSubRunInfo.count(), but it would really complicate
how
> I
> > have to handle 'handing off' from one run to the next, ie, if there are no
> > glyphs in a given run, the subrun could be empty, so when I go to write
> forward
> > start vertices / indices, I'd have to put in logic to handle the empty case.

> I
> > could handle this by putting start / end vertices / indices on each run as
> well
> > as each subrun, but it seems easier to just use a bool.
> 
> I'm not sure I totally follow... but maybe this makes sense:
> 
> Why not skip the push_back in the constructor and have:
> 
> if (run.fSubRunInfo.isEmpty() || run.fSubRunInfo.back().fMaskFormat != format)
{
>     SubRunInfo* newSubRun = &run.fSubRunInfo.push_back();
>     // ...
> }
> SubRunInfo* subRunInfo = &run.fSubRunInfo.back();
> 
> Seems simpler, no slower, and gets rid of the bool. Is it useful elsewhere to
> have a subrun that has no glyphs?
> 
>
https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
> src/gpu/GrBitmapTextContext.h:127: void* operator new(size_t, void* p) {
return
> p; }
> On 2015/04/01 13:21:18, joshualitt wrote:
> > On 2015/03/31 21:19:11, bsalomon wrote:
> > > some compilers will yell at you for not overriding delete if you override
> new.
> > 
> > Acknowledged.  I'm not sure if I did the delete right though.
> 
> Now that I look more closely, why did you need to override new/delete at all?
> 
> https://codereview.chromium.org/1041953002/diff/120001/src/core/SkDescriptor.h
> File src/core/SkDescriptor.h (right):
> 
>
https://codereview.chromium.org/1041953002/diff/120001/src/core/SkDescriptor....
> src/core/SkDescriptor.h:177: size_t          fHeapAllocSize;
> Is this really worth the complexity? Seems much simpler to just have
> 
> reset(size_t size) {
>     if (fDesc != (SkDescriptor*)(void*)fStorage) {
>         SkDescriptor::Free(fDesc);
>     }
>     if (size <= sizeof(fStorage) {
>         fDesc = (SkDescriptor*)(void*)fStorage;
>     } else {
>         fDesc = SkDescriptor::Alloc(size);
>     }
> }
> 
> and ~ can stay as it is. Do you expect to be reinit'ing these frequently where
> the  size is larger than fStorage both before and after?
> 
>
https://codereview.chromium.org/1041953002/diff/120001/src/gpu/GrBitmapTextCo...
> File src/gpu/GrBitmapTextContext.h (right):
> 
>
https://codereview.chromium.org/1041953002/diff/120001/src/gpu/GrBitmapTextCo...
> src/gpu/GrBitmapTextContext.h:70: // We call these SubRuns.  Because a subrun
> must be a contiguous range, we have to create
> Is it really true that we need to create a subrun at every transition rather
> than one subrun per mask type? Are we concerned about preserving order within
a
> run?

feedback incorporated.

[joshualitt, 2015-04-01 16:54:23]

one response inline, Brian and I discussed most of this offline

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
File src/gpu/GrBitmapTextContext.h (right):

https://codereview.chromium.org/1041953002/diff/80001/src/gpu/GrBitmapTextCon...
src/gpu/GrBitmapTextContext.h:127: void* operator new(size_t, void* p) { return
p; }
On 2015/04/01 15:41:45, bsalomon wrote:
> On 2015/04/01 13:21:18, joshualitt wrote:
> > On 2015/03/31 21:19:11, bsalomon wrote:
> > > some compilers will yell at you for not overriding delete if you override
> new.
> > 
> > Acknowledged.  I'm not sure if I did the delete right though.
> 
> Now that I look more closely, why did you need to override new/delete at all?

Just to put the throw in new.  I guess its not really necessary, but anyways I
override them a few cls down the road to use the pool.

https://codereview.chromium.org/1041953002/diff/120001/src/core/SkDescriptor.h
File src/core/SkDescriptor.h (right):

https://codereview.chromium.org/1041953002/diff/120001/src/core/SkDescriptor....
src/core/SkDescriptor.h:177: size_t          fHeapAllocSize;
On 2015/04/01 15:41:45, bsalomon wrote:
> Is this really worth the complexity? Seems much simpler to just have
> 
> reset(size_t size) {
>     if (fDesc != (SkDescriptor*)(void*)fStorage) {
>         SkDescriptor::Free(fDesc);
>     }
>     if (size <= sizeof(fStorage) {
>         fDesc = (SkDescriptor*)(void*)fStorage;
>     } else {
>         fDesc = SkDescriptor::Alloc(size);
>     }
> }
> 
> and ~ can stay as it is. Do you expect to be reinit'ing these frequently where
> the  size is larger than fStorage both before and after?

If I'm going to be reusing the blobs, which I basically can now that I just have
the large vertex pools, then I'll be reiniting the descriptor every time I
reuse.  Seems like a shame to free the alloc in that case, but its your call.

[bsalomon, 2015-04-01 17:27:32]

lgtm

[joshualitt, 2015-04-01 18:16:32]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-01 18:16:33]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1041953002/#ps200001
(title: "third attempt")

[commit-bot: I haz the power, 2015-04-01 18:16:56]

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

[commit-bot: I haz the power, 2015-04-01 18:30:31]

Committed patchset #11 (id:200001) as
https://skia.googlesource.com/skia/+/2b6acb4ed52483bc2a89dbbaa6f0db4fdb217cd3

[joshualitt, 2015-04-02 12:48:30]

joshualitt@google.com changed reviewers:
+ robertphillips@google.com

[joshualitt, 2015-04-02 12:48:31]

Adding Robert in case he wants to take a look