Add a GPU paths to resource cache of the context

Make GrContext cache the gpu paths

Creating paths for nv_path_rendering is costly. Try to reduce this
cost by caching paths based on the SkPath "hash" (i.e. SkPathRef
generation id) and stroke properties.

Adds the paths to GrContext::fTextureCache instance. Later this should
be renamed and the GrContext API should reflect the nature of the cache
better.

Committed: http://code.google.com/p/skia/source/detail?r=12083

[Kimmo Kinnunen, 2013-10-15 10:57:23]

Ready for review

[bsalomon, 2013-10-15 13:27:34]

On 2013/10/15 10:57:23, kkinnunen wrote:
> Ready for review

Hi Kimmo, there are some issues with the code review site that makes some
uploaded diffs unviewable. If you try to view the diff you get "error: old chunk
mismatch." I encountered this yesterday and wound up uploading my patch several
times before I got one that worked.

[mtklein, 2013-10-15 15:29:50]

https://chromiumcodereview.appspot.com/26557003/diff/3001/include/core/SkPath...
File include/core/SkPathRef.h (right):

https://chromiumcodereview.appspot.com/26557003/diff/3001/include/core/SkPath...
include/core/SkPathRef.h:237: 
nit: this->genID()

https://chromiumcodereview.appspot.com/26557003/diff/3001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://chromiumcodereview.appspot.com/26557003/diff/3001/src/gpu/GrContext.c...
src/gpu/GrContext.cpp:138: 
Seems to me that if we're merging the resource limits here anyway, there's not a
big reason to separate out the MAX_PATH_CACHE_{COUNT,BYTES} from the existing
TEXTURE_CACHE ones.

If we merged the four back into two values, one for count, one for bytes, and
then followed up at some point by renaming them to
MAX_GPU_RESOURCE_CACHE_{COUNT,BYTES} or similar, would that work for you?  Are
there places where we really plan (or want) to treat the path limits separately
from the textures or whatever else we shove in here?

https://chromiumcodereview.appspot.com/26557003/diff/3001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://chromiumcodereview.appspot.com/26557003/diff/3001/src/gpu/gl/GrGpuGL....
src/gpu/gl/GrGpuGL.cpp:1220: 
I think we might want to rearrange where this logic lives.  I'd prefer it if the
knowledge of the cache were restricted to the context.  Adding
getResourceCache() is now requiring the presence of what was an implementation
detail before.

What do you guys think about making this code here a simple
  return getContext()->createGrGLPath(inPath, stroke);
and pushing the rest into the new createGrGLPath?

https://chromiumcodereview.appspot.com/26557003/diff/3001/src/gpu/gl/GrGpuGL....
src/gpu/gl/GrGpuGL.cpp:1223: 
I think we might be able to avoid the call to isEqualTo by using more of the
bytes in the cache key.  If we use 32 bits of the cache key for the pathref's
genID, and <= 96 bits to encode the stroke, doesn't that mean if the cache
returns something, it's the right path?

[Kimmo Kinnunen, 2013-10-16 06:14:28]

Thanks for the comments. A new version is up.

https://codereview.chromium.org/26557003/diff/3001/include/core/SkPathRef.h
File include/core/SkPathRef.h (right):

https://codereview.chromium.org/26557003/diff/3001/include/core/SkPathRef.h#n...
include/core/SkPathRef.h:237: 
On 2013/10/15 15:29:50, mtklein wrote:
> nit: this->genID()

Done.

https://codereview.chromium.org/26557003/diff/3001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/26557003/diff/3001/src/gpu/GrContext.cpp#newc...
src/gpu/GrContext.cpp:138: 
On 2013/10/15 15:29:50, mtklein wrote:
> Seems to me that if we're merging the resource limits here anyway, there's not
a
> big reason to separate out the MAX_PATH_CACHE_{COUNT,BYTES} from the existing
> TEXTURE_CACHE ones.

The reason is that this is not enabled by default and not supported by all
platforms at the moment. Increasing one configuration variable would create
unneeded change of functionality for platforms not needing this.

Also, I didn't want to change the API, as it seems it is intended to really
textures. However, you're right that the implementation did not at all match
what was expected. I fixed that.

https://codereview.chromium.org/26557003/diff/3001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/26557003/diff/3001/src/gpu/gl/GrGpuGL.cpp#new...
src/gpu/gl/GrGpuGL.cpp:1220: 
On 2013/10/15 15:29:50, mtklein wrote:
> I think we might want to rearrange where this logic lives.  I'd prefer it if
the
> knowledge of the cache were restricted to the context.  Adding
> getResourceCache() is now requiring the presence of what was an implementation
> detail before.

Fixed this.

https://codereview.chromium.org/26557003/diff/3001/src/gpu/gl/GrGpuGL.cpp#new...
src/gpu/gl/GrGpuGL.cpp:1223: 
On 2013/10/15 15:29:50, mtklein wrote:
> I think we might be able to avoid the call to isEqualTo by using more of the
> bytes in the cache key.  If we use 32 bits of the cache key for the pathref's
> genID, and <= 96 bits to encode the stroke, doesn't that mean if the cache
> returns something, it's the right path?

I interpreted that above would not be enough. There are more path+stroke
combinations in the world than 2^96 (or 2^128 for that matter). I tried to
communicate this by adding "SkPath::getHash()" instead of
"SkPath::getIdentity()" or something.

Probably would be warranted if this was intended limitation in the library. For
this to be the case, the SkPath::operator== should reflect this.

[mtklein, 2013-10-17 13:38:26]

I see you've done a lot of refactoring that's somewhat incidental to caching GPU
paths.  I think most of it can land right away (e.g. the global
s/fTextureCache/fResourceCache) and some of it will need a little more care
(e.g. removing methods, changing method names).  Can you split these off into
one or more other changelists to review independently?  I'd like this one to
focus as much as possible on how we cache GPU paths.

https://codereview.chromium.org/26557003/diff/3001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/26557003/diff/3001/src/gpu/GrContext.cpp#newc...
src/gpu/GrContext.cpp:138: 
On 2013/10/16 06:14:28, kkinnunen wrote:
> On 2013/10/15 15:29:50, mtklein wrote:
> > Seems to me that if we're merging the resource limits here anyway, there's
not
> a
> > big reason to separate out the MAX_PATH_CACHE_{COUNT,BYTES} from the
existing
> > TEXTURE_CACHE ones.
> 
> The reason is that this is not enabled by default and not supported by all
> platforms at the moment. Increasing one configuration variable would create
> unneeded change of functionality for platforms not needing this.
> 
> Also, I didn't want to change the API, as it seems it is intended to really
> textures. However, you're right that the implementation did not at all match
> what was expected. I fixed that.

My perspective here is that we probably don't want to increase the existing
configuration variables.  Where set, they're there to put a cap on how much GPU
RAM we'll use.  We'd want paths to be a part of that existing limit, rather than
bumping it when we're going to use paths.

The API _was_ intended to be about textures, yes, but at this point these
#defines are just absurd misnomers.  We're definitely storing more than textures
in the cache, and using MAX_TEXTURE_CACHE_COUNT and MAX_TEXTURE_CACHE_BYTES to
manage them all.

If you're interested, I think I've pinned down the change from 2011 when these
first lost their precise meaning.  We switched the class from GrTextureCache to
GrResourceCache without changing MAX_TEXTURE_CACHE_FOO or fTextureCache:
https://codereview.appspot.com/4815055/diff/10001/gpu/src/GrContext.cpp?conte...

https://codereview.chromium.org/26557003/diff/3001/src/gpu/gl/GrGpuGL.cpp
File src/gpu/gl/GrGpuGL.cpp (right):

https://codereview.chromium.org/26557003/diff/3001/src/gpu/gl/GrGpuGL.cpp#new...
src/gpu/gl/GrGpuGL.cpp:1223: 
On 2013/10/16 06:14:28, kkinnunen wrote:
> On 2013/10/15 15:29:50, mtklein wrote:
> > I think we might be able to avoid the call to isEqualTo by using more of the
> > bytes in the cache key.  If we use 32 bits of the cache key for the
pathref's
> > genID, and <= 96 bits to encode the stroke, doesn't that mean if the cache
> > returns something, it's the right path?
> 
> I interpreted that above would not be enough. There are more path+stroke
> combinations in the world than 2^96 (or 2^128 for that matter). I tried to
> communicate this by adding "SkPath::getHash()" instead of
> "SkPath::getIdentity()" or something.
> 
> Probably would be warranted if this was intended limitation in the library.
For
> this to be the case, the SkPath::operator== should reflect this.

Ah, I may have been seeing SkPath and thinking SkPathRef, where equality really
is just checked by checking gen IDs.  It's an interesting question whether or
not we can pack a unique ID for an SkPath and a SkStrokeRec in 128 bits.  I took
a look and sorted through it and it seems possible (I think we need around 103
bits), but to make it work as things are laid out now would require some pretty
fragile code.

Let's leave things as they are for now.  I hear we've got plans to refactor how
SkPath and SkPathRef distribute their data in a way that may make this easier
for me to circle back around on later.

[Kimmo Kinnunen, 2013-10-18 09:20:05]

On 2013/10/17 13:38:26, mtklein wrote:
> I see you've done a lot of refactoring that's somewhat incidental to caching
GPU
> paths.  I think most of it can land right away (e.g. the global
> s/fTextureCache/fResourceCache) and some of it will need a little more care
> (e.g. removing methods, changing method names).  Can you split these off into
> one or more other changelists to review independently?  I'd like this one to
> focus as much as possible on how we cache GPU paths.

Done. Made the combining the caches a TODO.
The other patch, which fixes the TODO, is in:
https://codereview.chromium.org/28693002

[bsalomon, 2013-10-18 13:25:24]

Mostly looking good to me, a couple comments inline.

https://codereview.chromium.org/26557003/diff/178001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/26557003/diff/178001/include/core/SkPath.h#ne...
include/core/SkPath.h:929: uint32_t getHash() const;
We already have a getGenerationID() method that is Android-only. Can we just
move that out from behind the macro (and it may have a different impl for the
Android build)?

https://codereview.chromium.org/26557003/diff/178001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/26557003/diff/178001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:276: GrPath* createPath(const SkPath& skPath, const
SkStrokeRec& stroke);
I'm wondering if there is any way to avoid this as public function on a client
facing class.

I think we will want to expose GrResourceCache, or really some narrower wrapper
on it, as a public interface available from GrContext for clients that want to
write their own GrEffects. That would serve the needs of this change I think,
but maybe that's another little project.

In the meantime maybe we could move this to private and friend GrPath? It'd be
ugly but I think I'd prefer that to exposing this function.

[Kimmo Kinnunen, 2013-10-21 13:40:48]

https://codereview.chromium.org/26557003/diff/178001/include/core/SkPath.h
File include/core/SkPath.h (right):

https://codereview.chromium.org/26557003/diff/178001/include/core/SkPath.h#ne...
include/core/SkPath.h:929: uint32_t getHash() const;
On 2013/10/18 13:25:24, bsalomon wrote:
> We already have a getGenerationID() method that is Android-only. Can we just
> move that out from behind the macro (and it may have a different impl for the
> Android build)?

There's the problem that this would be confusing for the reader, since the
functions mean totally different things.

Also the problem that the android builds would use the quite wrong behavior. 

SkPath generation id asserts that "paths with same generation id value have same
amount of modifications". Problematic for example since all rects have same
amount of modifications.

SkPathRef generation id asserts that "paths with same generation id value are
same paths".

https://codereview.chromium.org/26557003/diff/178001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/26557003/diff/178001/include/gpu/GrContext.h#...
include/gpu/GrContext.h:276: GrPath* createPath(const SkPath& skPath, const
SkStrokeRec& stroke);
On 2013/10/18 13:25:24, bsalomon wrote:
> I'm wondering if there is any way to avoid this as public function on a client
> facing class.
> 
> I think we will want to expose GrResourceCache, or really some narrower
wrapper
> on it, as a public interface available from GrContext for clients that want to
> write their own GrEffects. That would serve the needs of this change I think,
> but maybe that's another little project.
> 
> In the meantime maybe we could move this to private and friend GrPath? It'd be
> ugly but I think I'd prefer that to exposing this function.

Done.

[Kimmo Kinnunen, 2013-10-28 15:08:33]

Any other comments or any other modifications I could do to get this reviewed
again?

[bsalomon, 2013-10-28 19:59:09]

On 2013/10/28 15:08:33, kkinnunen wrote:
> Any other comments or any other modifications I could do to get this reviewed
> again?

Hey Kimmo, we're trying to remove the Android-specific generation ID:
https://codereview.chromium.org/49693002/

[bsalomon, 2013-10-31 15:43:05]

On 2013/10/28 19:59:09, bsalomon wrote:
> On 2013/10/28 15:08:33, kkinnunen wrote:
> > Any other comments or any other modifications I could do to get this
reviewed
> > again?
> 
> Hey Kimmo, we're trying to remove the Android-specific generation ID:
> https://codereview.chromium.org/49693002/

BTW this change has landed. So there is now a SkPath::getGenerationID() on all
platforms. It has slightly different behavior in Android builds (where it
considers the fill type). There is a TODO to fix this (skbug.com/1762).

[Kimmo Kinnunen, 2013-11-01 13:25:11]

On 2013/10/31 15:43:05, bsalomon wrote:
> On 2013/10/28 19:59:09, bsalomon wrote:
> > On 2013/10/28 15:08:33, kkinnunen wrote:
> > > Any other comments or any other modifications I could do to get this
> reviewed
> > > again?
> > 
> > Hey Kimmo, we're trying to remove the Android-specific generation ID:
> > https://codereview.chromium.org/49693002/
> 
> BTW this change has landed. So there is now a SkPath::getGenerationID() on all
> platforms. It has slightly different behavior in Android builds (where it
> considers the fill type). There is a TODO to fix this (skbug.com/1762).

Great. I updated the patch.

[bsalomon, 2013-11-01 14:46:18]

lgtm

[commit-bot: I haz the power, 2013-11-01 14:57:15]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/kkinnunen@nvidia.com/26557003/388001

[commit-bot: I haz the power, 2013-11-01 15:23:52]

Change committed as 12083