The great shader refactor: Move all programs to a common cache

The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, which takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2613903002
Cr-Commit-Position: refs/heads/master@{#443362}
Committed: https://chromium.googlesource.com/chromium/src/+/4183067b5afba105939fef7dbd4082295ad2791f

[ccameron, 2017-01-05 10:25:50]

Description was changed from

==========
The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, whihc takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
==========

to

==========
The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, whihc takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-01-05 10:32:26]

ccameron@chromium.org changed reviewers:
+ enne@chromium.org

[ccameron, 2017-01-05 10:32:27]

Continuing to rain destruction...

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (left):

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h#old...
cc/output/gl_renderer.h:397: 
Avoiding adding another dimension to all of these arrays is the motivation for
this part of the cleanup.

https://codereview.chromium.org/2613903002/diff/1/cc/output/program_binding.h
File cc/output/program_binding.h (right):

https://codereview.chromium.org/2613903002/diff/1/cc/output/program_binding.h...
cc/output/program_binding.h:163: private:
This "public then private then public again" scheme here is just to keep the
diffs minimal for reviews.

Once I remove the template of ProgramBinding (once I merge in the YUV shader),
then I can throw this all in the .cc file.

[enne (OOO), 2017-01-09 19:46:49]

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.cc#ne...
cc/output/gl_renderer.cc:1329: const Program* program =
GetProgram(ProgramKey::RenderPass(
This cleanup is great.

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (left):

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h#old...
cc/output/gl_renderer.h:397: 
On 2017/01/05 at 10:32:27, ccameron wrote:
> Avoiding adding another dimension to all of these arrays is the motivation for
this part of the cleanup.

Thank you for continuing to clean up this mess.  It's kind of a precarious house
of cards that just doesn't need to get touched very often, soooooo

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h#new...
cc/output/gl_renderer.h:402: std::unordered_map<ProgramKey, Program*,
ProgramKeyHash> program_cache_;
Can you make the Program* a unique_ptr<Program>?

[ccameron, 2017-01-11 09:39:42]

Description was changed from

==========
The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, whihc takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, which takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[ccameron, 2017-01-11 09:43:17]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 09:43:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ccameron, 2017-01-11 09:46:40]

Updated (now with a working dependency patch)

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2613903002/diff/1/cc/output/gl_renderer.h#new...
cc/output/gl_renderer.h:402: std::unordered_map<ProgramKey, Program*,
ProgramKeyHash> program_cache_;
On 2017/01/09 19:46:48, enne wrote:
> Can you make the Program* a unique_ptr<Program>?

Yes, good point!

https://codereview.chromium.org/2613903002/diff/20001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2613903002/diff/20001/cc/output/gl_renderer.c...
cc/output/gl_renderer.cc:1310: void
GLRenderer::ChooseRPDQProgram(DrawRenderPassDrawQuadParams* params) {
I ended up leaving this untouched in this patch -- the next one purges
GLRenderer.

[commit-bot: I haz the power, 2017-01-11 11:17:52]

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

[commit-bot: I haz the power, 2017-01-11 11:17:52]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ccameron, 2017-01-12 00:37:39]

The CQ bit was checked by ccameron@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 00:38:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 03:41:27]

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

[commit-bot: I haz the power, 2017-01-12 03:41:28]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2017-01-12 18:29:12]

lgtm

https://codereview.chromium.org/2613903002/diff/40001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2613903002/diff/40001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:331: const Program* GetProgramForTesting(const
ProgramKey& key) const;
I hate to bikeshed on a name, but this was a little confusing to me.  I think I
wish that GetProgramForTesting was GetProgramIfInitialized() or something, with
a comment on GetProgram that it will get it and initialize it if it doesn't
exist already.

If renaming the world is too complicated in a patch chain, comments on each of
these in this header would go a long way.

[ccameron, 2017-01-12 20:26:05]

Thanks!

https://codereview.chromium.org/2613903002/diff/40001/cc/output/gl_renderer.h
File cc/output/gl_renderer.h (right):

https://codereview.chromium.org/2613903002/diff/40001/cc/output/gl_renderer.h...
cc/output/gl_renderer.h:331: const Program* GetProgramForTesting(const
ProgramKey& key) const;
On 2017/01/12 18:29:12, enne wrote:
> I hate to bikeshed on a name, but this was a little confusing to me.  I think
I
> wish that GetProgramForTesting was GetProgramIfInitialized() or something,
with
> a comment on GetProgram that it will get it and initialize it if it doesn't
> exist already.
> 
> If renaming the world is too complicated in a patch chain, comments on each of
> these in this header would go a long way.

Mmh, good idea.

Renamed and also added the comments.

[ccameron, 2017-01-12 20:26:06]

Thanks!

[ccameron, 2017-01-12 20:26:10]

The CQ bit was checked by ccameron@chromium.org

[ccameron, 2017-01-12 20:26:11]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org
Link to the patchset: https://codereview.chromium.org/2613903002/#ps60001
(title: "Incorporate review feedback")

[commit-bot: I haz the power, 2017-01-12 20:26:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:25:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484252770794310,
"parent_rev": "0e84ee428168105852608f4a900081e58f3084a2", "commit_rev":
"4183067b5afba105939fef7dbd4082295ad2791f"}

[commit-bot: I haz the power, 2017-01-12 21:25:33]

Description was changed from

==========
The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, which takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
The great shader refactor: Move all programs to a common cache

The various programs stored in GLRenderer are 5-dimensional arrays,
and adding color conversion to them would require adding a 6th
dimension.

Create a ProgramKey structure that uniquely identifies a program, and
create a single unordered_map to cache programs.

Gut all off the GLRenderer::Get*Program* functions, to just call a
common GLRenderer::GetProgram function, which takes this key.

Adding color conversion will now just consist of adding a new field to
the ProgramKey structure. Before we do that, we'll remove all of the
(now-stub) GLRenderer::Get*Program* functions, and have the callers
all call GLRenderer::GetProgram.

BUG=667966
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2613903002
Cr-Commit-Position: refs/heads/master@{#443362}
Committed:
https://chromium.googlesource.com/chromium/src/+/4183067b5afba105939fef7dbd40...
==========

[commit-bot: I haz the power, 2017-01-12 21:25:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4183067b5afba105939fef7dbd40...