Set up cache in vulkan to reuse GrVkPrograms (aka VkPipelines)

Set up cache in vulkan to reuse GrVkPrograms (aka VkPipelines)


BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1816153002

Committed: https://skia.googlesource.com/skia/+/22281c13a13c4b4e275516e9fe02185a53a7e5aa

[egdaniel, 2016-03-21 16:24:20]

Description was changed from

==========
Set up cache in vulkan to reuse GrVkPrograms (aka VkPipelines)


BUG=skia:
==========

to

==========
Set up cache in vulkan to reuse GrVkPrograms (aka VkPipelines)


BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[egdaniel, 2016-03-21 20:14:00]

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

[jvanverth1, 2016-03-21 21:16:16]

A bunch of little stuff, plus one general question. Is the reason the stencil
settings and the attachment formats are included in the key because this program
can only be used with those? Is the stencil not in the Pipeline?

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram.h
File src/gpu/vk/GrVkProgram.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.h:88: static bool Less(const PipelineDesc& a, const
PipelineDesc& b) {
Is there a reason you didn't use operator<?

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgramC...
File src/gpu/vk/GrVkProgramCache.cpp (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgramC...
src/gpu/vk/GrVkProgramCache.cpp:131: GrCapsDebugf(fGpu->caps(), "Failed to vk
program descriptor!\n");
Failed to build?

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgramC...
src/gpu/vk/GrVkProgramCache.cpp:140: // Seed the checksum with the checksome of
the programDesc then add the vulkan key to it.
Checksum (sp)

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:85: //       typedef GrGpu::DrawArgs DrawArgs;
Still need this?

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:99: // We may actually have kMaxEntries+1
shaders in the GL context because we create a new
GL? And are these values good for Vulkan?

[egdaniel, 2016-03-21 22:23:52]

So you are correct that the stencil is in the pipeline, but this is really a
cache for the pipeline. Thus we need the stencil as part of the key for the
pipeline.

The attachment formats are for the render pass. When creating a pipeline, we
need to give it a render pass. However, we can actually reuse this pipeline with
different render passes as long as the contain the same attachment and formats.

On a side note, I think in general the name GrVkProgram is slightly confusing
things when viewing with a GL frame of mind. In vulkan, I am using the term
Program to mean the object that wraps all the information we need to do a draw.
This includes the pipeline and GrVkProgramDataManager (including methods for
setting data to be used for that draw). I do kind of want to keep GrVkPipeline
separate from this since that really is more the wrapper around a gpu resources
and not the program data manager doesn't really fit on it, but I'm up to be
convinced otherwise.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram.h
File src/gpu/vk/GrVkProgram.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.h:88: static bool Less(const PipelineDesc& a, const
PipelineDesc& b) {
On 2016/03/21 21:16:16, jvanverth1 wrote:
> Is there a reason you didn't use operator<?

Mostly just following the template from GL. SkTSearch does need to take a Less
functor, but we do wrap that in the PipelineDescLess struct. Using the Less
function just kind of follows along with the use of a functor, but really I we
could use the operator instead.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:99: // We may actually have kMaxEntries+1
shaders in the GL context because we create a new
On 2016/03/21 21:16:16, jvanverth1 wrote:
> GL? And are these values good for Vulkan?

When I previously looked at the cache stats we were getting good hit rate with
these values. However, I can play around with them to see if anything improves.
Was there any actual reasoning behind these numbers in GL besides a guesstimate
of what would be good?

[egdaniel, 2016-03-21 22:23:53]

So you are correct that the stencil is in the pipeline, but this is really a
cache for the pipeline. Thus we need the stencil as part of the key for the
pipeline.

The attachment formats are for the render pass. When creating a pipeline, we
need to give it a render pass. However, we can actually reuse this pipeline with
different render passes as long as the contain the same attachment and formats.

On a side note, I think in general the name GrVkProgram is slightly confusing
things when viewing with a GL frame of mind. In vulkan, I am using the term
Program to mean the object that wraps all the information we need to do a draw.
This includes the pipeline and GrVkProgramDataManager (including methods for
setting data to be used for that draw). I do kind of want to keep GrVkPipeline
separate from this since that really is more the wrapper around a gpu resources
and not the program data manager doesn't really fit on it, but I'm up to be
convinced otherwise.

[bsalomon, 2016-03-22 14:00:33]

I think it'd be good to clear up the program, pipeline, program+pipeline
distinctions. The code is so similar to the GL implementation but is slightly
different. Probably shouldn't call it a program cache.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/GrStencil.cpp
File src/gpu/GrStencil.cpp (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/GrStencil.cpp#n...
src/gpu/GrStencil.cpp:400: SkASSERT(sizeof(GrStencilSettings) <= 6 *
sizeof(uint32_t));
How about

static const int kCount = sizeof(GrStencilSettings) / sizeof(uint32_t);
GR_STATIC_ASSERT(0 == sizeof(GrStencilSettings) % sizeof(uint32_t));

Also, is this guy tightly packed or does it memset itself to zero?

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram.cpp
File src/gpu/vk/GrVkProgram.cpp (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:475: SkTArray<unsigned char, true>* key) {
uint8_t in the class and unsigned char here

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram.h
File src/gpu/vk/GrVkProgram.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.h:66: SkTArray<uint8_t, true> fVkKey;
Any idea what the typical size is? Wonder if we should use a SkSTArray here.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgramC...
File src/gpu/vk/GrVkProgramCache.cpp (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgramC...
src/gpu/vk/GrVkProgramCache.cpp:2: * Copyright 2016 Google Inc.
spaces?

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:99: // We may actually have kMaxEntries+1
shaders in the GL context because we create a new
On 2016/03/21 22:23:52, egdaniel wrote:
> On 2016/03/21 21:16:16, jvanverth1 wrote:
> > GL? And are these values good for Vulkan?
> 
> When I previously looked at the cache stats we were getting good hit rate with
> these values. However, I can play around with them to see if anything
improves.
> Was there any actual reasoning behind these numbers in GL besides a
guesstimate
> of what would be good?

At various times in the past this was tuned to improve hit rates, but not
anytime recently.

[egdaniel, 2016-03-22 20:06:15]

I've change GrVkProgram to be GrVkPipelineState. All related helper structs and
classes also had their names updated.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/GrStencil.cpp
File src/gpu/GrStencil.cpp (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/GrStencil.cpp#n...
src/gpu/GrStencil.cpp:400: SkASSERT(sizeof(GrStencilSettings) <= 6 *
sizeof(uint32_t));
On 2016/03/22 14:00:33, bsalomon wrote:
> How about
> 
> static const int kCount = sizeof(GrStencilSettings) / sizeof(uint32_t);
> GR_STATIC_ASSERT(0 == sizeof(GrStencilSettings) % sizeof(uint32_t));
> 
> Also, is this guy tightly packed or does it memset itself to zero?

Done. He does do a memset to zero in ctor. In general he is tightly packed
except for 16 bits of packing in middle which is also manually set to 0 to make
sure it is all nicely initialized.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram.cpp
File src/gpu/vk/GrVkProgram.cpp (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.cpp:475: SkTArray<unsigned char, true>* key) {
On 2016/03/22 14:00:33, bsalomon wrote:
> uint8_t in the class and unsigned char here

Done.

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram.h
File src/gpu/vk/GrVkProgram.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkProgram....
src/gpu/vk/GrVkProgram.h:66: SkTArray<uint8_t, true> fVkKey;
On 2016/03/22 14:00:33, bsalomon wrote:
> Any idea what the typical size is? Wonder if we should use a SkSTArray here.

Changed this to STArray

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:85: //       typedef GrGpu::DrawArgs DrawArgs;
On 2016/03/21 21:16:16, jvanverth1 wrote:
> Still need this?

deleted

https://codereview.chromium.org/1816153002/diff/60001/src/gpu/vk/GrVkResource...
src/gpu/vk/GrVkResourceProvider.h:99: // We may actually have kMaxEntries+1
shaders in the GL context because we create a new
On 2016/03/22 14:00:33, bsalomon wrote:
> On 2016/03/21 22:23:52, egdaniel wrote:
> > On 2016/03/21 21:16:16, jvanverth1 wrote:
> > > GL? And are these values good for Vulkan?
> > 
> > When I previously looked at the cache stats we were getting good hit rate
with
> > these values. However, I can play around with them to see if anything
> improves.
> > Was there any actual reasoning behind these numbers in GL besides a
> guesstimate
> > of what would be good?
> 
> At various times in the past this was tuned to improve hit rates, but not
> anytime recently.

Fixed comment. For now my rough testing shows decent values for this, but I will
punt on tuning this directly for this CL and do so in a follow on pass

[bsalomon, 2016-03-23 15:04:37]

lgtm, nits inline

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
File src/gpu/vk/GrVkPipelineState.h (right):

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:2: * Copyright 2015 Google Inc.
2016

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:28: class GrVkPipelineState : public SkRefCnt {
brief comment on what this is?

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:194: 
rm \n?

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:196: * We use the RT's size and origin to adjust
from Skia device space to OpenGL normalized device
OpenGL?

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:212: * Gets a vec4 that adjusts the position from
Skia device coords to GL's normalized device
GL

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:275: 
Inconsistent alignment in these members... I'd say either give up on aligning or
align all of them.

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkResourc...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkResourc...
src/gpu/vk/GrVkResourceProvider.h:136: SkSTArray<4, GrVkRenderPass*>   
fSimpleRenderPasses;
Can you either leave all the members staggered or align them all?

[egdaniel, 2016-03-23 19:12:07]

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
File src/gpu/vk/GrVkPipelineState.h (right):

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:2: * Copyright 2015 Google Inc.
On 2016/03/23 15:04:36, bsalomon wrote:
> 2016

Done.

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:28: class GrVkPipelineState : public SkRefCnt {
On 2016/03/23 15:04:37, bsalomon wrote:
> brief comment on what this is? 

Done.

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:194: 
On 2016/03/23 15:04:37, bsalomon wrote:
> rm \n?

Done.

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:196: * We use the RT's size and origin to adjust
from Skia device space to OpenGL normalized device
On 2016/03/23 15:04:37, bsalomon wrote:
> OpenGL?

Done.

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkPipelin...
src/gpu/vk/GrVkPipelineState.h:212: * Gets a vec4 that adjusts the position from
Skia device coords to GL's normalized device
On 2016/03/23 15:04:36, bsalomon wrote:
> GL

Done.

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkResourc...
File src/gpu/vk/GrVkResourceProvider.h (right):

https://codereview.chromium.org/1816153002/diff/100001/src/gpu/vk/GrVkResourc...
src/gpu/vk/GrVkResourceProvider.h:136: SkSTArray<4, GrVkRenderPass*>   
fSimpleRenderPasses;
On 2016/03/23 15:04:37, bsalomon wrote:
> Can you either leave all the members staggered or align them all?

Done.

[egdaniel, 2016-03-23 19:12:13]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-03-23 19:12:14]

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/1816153002/#ps120001
(title: "review nits")

[commit-bot: I haz the power, 2016-03-23 19:12:19]

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

[commit-bot: I haz the power, 2016-03-23 19:13:29]

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

[commit-bot: I haz the power, 2016-03-23 19:13:30]

Try jobs failed on following builders:
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[egdaniel, 2016-03-23 20:38:57]

The CQ bit was checked by egdaniel@google.com

[egdaniel, 2016-03-23 20:38:57]

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/1816153002/#ps140001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-23 20:39:02]

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

[commit-bot: I haz the power, 2016-03-23 20:49:42]

Description was changed from

==========
Set up cache in vulkan to reuse GrVkPrograms (aka VkPipelines)


BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Set up cache in vulkan to reuse GrVkPrograms (aka VkPipelines)


BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/22281c13a13c4b4e275516e9fe02185a53a7e5aa
==========

[commit-bot: I haz the power, 2016-03-23 20:49:43]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/22281c13a13c4b4e275516e9fe02185a53a7e5aa