Adding bulk plot reffer to cached textblobs

Adding bulk plot reffer to cached textblobs

This change will prevent the atlas from evicting glyphs the TextBlob
needs.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/7281c61e7bc689d484dcbda49be3cef4ce4f11c2

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

[joshualitt, 2015-04-01 19:22:16]

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

[joshualitt, 2015-04-01 19:22:16]

This change addresses the problem of the atlas evicting glyphs we might need
while flushing a cached textblob.

[bsalomon, 2015-04-01 20:21:31]

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.cpp#ne...
src/gpu/GrBatchAtlas.cpp:371: this->makeMRU(fPlotArray[index]);
Call setLastRefTokenInternal() here?

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.cpp#ne...
src/gpu/GrBatchAtlas.cpp:375: void GrBatchAtlas::setLastRefTokenInternal(int
index, BatchToken batchToken) {
tiny nit, I think we more often say "internalSetLastRefToken"

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h#newc...
src/gpu/GrBatchAtlas.h:25: // An AtlasID is an opaque handle which callers can
use to determine if the atlas contains
I think this comment should be updated to indicate that the atlas generates the
ID. Also, is it more of an "EntryHandle". AtlasID looks like it identifies an
atlas but really it identifies something in the atlas.

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h#newc...
src/gpu/GrBatchAtlas.h:61: class BulkTokenReffer {
I find the use of "Ref" confusing in this class. It doesn't mean what it means
everywhere else in Skia. Same for setLastRefToken above.

https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h#newc...
src/gpu/GrBatchAtlas.h:64: void setRefToken(GrBatchAtlas* atlas, AtlasID id,
BatchToken token) {
Does this mean it sets here and when setLastRefTokenBulk is called? Do you need
both? It seems a bit asymmetric that in here a separate token could be used per
plot but then when calling setLastRefTokenBulk no token is provided. I guess it
uses the last one passed here? Does that mean it can never be reused with a
later token unless you add another plot through this function?

[joshualitt, 2015-04-02 15:42:14]

On 2015/04/01 20:21:31, bsalomon wrote:
> https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.cpp
> File src/gpu/GrBatchAtlas.cpp (right):
> 
>
https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.cpp#ne...
> src/gpu/GrBatchAtlas.cpp:371: this->makeMRU(fPlotArray[index]);
> Call setLastRefTokenInternal() here?
> 
>
https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.cpp#ne...
> src/gpu/GrBatchAtlas.cpp:375: void GrBatchAtlas::setLastRefTokenInternal(int
> index, BatchToken batchToken) {
> tiny nit, I think we more often say "internalSetLastRefToken"
> 
> https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h
> File src/gpu/GrBatchAtlas.h (right):
> 
>
https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h#newc...
> src/gpu/GrBatchAtlas.h:25: // An AtlasID is an opaque handle which callers can
> use to determine if the atlas contains
> I think this comment should be updated to indicate that the atlas generates
the
> ID. Also, is it more of an "EntryHandle". AtlasID looks like it identifies an
> atlas but really it identifies something in the atlas.
> 
>
https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h#newc...
> src/gpu/GrBatchAtlas.h:61: class BulkTokenReffer {
> I find the use of "Ref" confusing in this class. It doesn't mean what it means
> everywhere else in Skia. Same for setLastRefToken above.
> 
>
https://codereview.chromium.org/1050113004/diff/1/src/gpu/GrBatchAtlas.h#newc...
> src/gpu/GrBatchAtlas.h:64: void setRefToken(GrBatchAtlas* atlas, AtlasID id,
> BatchToken token) {
> Does this mean it sets here and when setLastRefTokenBulk is called? Do you
need
> both? It seems a bit asymmetric that in here a separate token could be used
per
> plot but then when calling setLastRefTokenBulk no token is provided. I guess
it
> uses the last one passed here? Does that mean it can never be reused with a
> later token unless you add another plot through this function?

Feedback incorporated

[joshualitt, 2015-04-02 15:42:27]

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

[bsalomon, 2015-04-02 17:44:56]

lgtm, two suggestions:

1) Rename addGlyphToBulk to addGlyphToBulkAndSetLastUseToken (or whatever is
appropriate phrasing)

2) Add a comment to addToAtlas that setLastUseToken should be called when the
entry is referred to.

[robertphillips, 2015-04-07 12:18:33]

lgtm + nits

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchAtlas.cpp
File src/gpu/GrBatchAtlas.cpp (right):

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchAtlas.c...
src/gpu/GrBatchAtlas.cpp:354: bool GrBatchAtlas::hasID(AtlasID id) {
"this->" -> "GrBatchAtlas::" for all the newly static methods ?

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchAtlas.h
File src/gpu/GrBatchAtlas.h (right):

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchAtlas.h...
src/gpu/GrBatchAtlas.h:63: * A class which can be handed back to GrBatchAtlas
for updating in bulk last use tokens.
Maybe a comment re the maxplots limit (e.g., for the current use cases 32 is
enough. If in the future this is insufficient ...) ?

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchAtlas.h...
src/gpu/GrBatchAtlas.h:81: bool find(int index) const {
SkASSERT(index < kMaxPlots); ?

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchFontCac...
File src/gpu/GrBatchFontCache.h (right):

https://codereview.chromium.org/1050113004/diff/100001/src/gpu/GrBatchFontCac...
src/gpu/GrBatchFontCache.h:100: // the client must pass in the currentToken from
the GrBatchTarget along with the GrGlyph.
typo: BulUse... -> BulkUse...

[joshualitt, 2015-04-07 16:55:12]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-07 16:55:12]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1050113004/#ps110001
(title: "feedback inc")

[commit-bot: I haz the power, 2015-04-07 16:55:31]

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

[commit-bot: I haz the power, 2015-04-07 17:02:31]

Committed patchset #7 (id:110001) as
https://skia.googlesource.com/skia/+/7281c61e7bc689d484dcbda49be3cef4ce4f11c2

[joshualitt, 2015-04-07 17:21:08]

A revert of this CL (patchset #7 id:110001) has been created in
https://codereview.chromium.org/1061713003/ by joshualitt@google.com.

The reason for reverting is: Breaks linux builder.

[joshualitt, 2015-04-08 14:53:58]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2015-04-08 14:53:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com,
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1050113004/#ps170001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-08 14:54:07]

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

[commit-bot: I haz the power, 2015-04-08 15:08:04]

Committed patchset #10 (id:170001) as
https://skia.googlesource.com/skia/+/b4c507e03386f2105e33f0c4c09b4a9d0a23196d