Updated shader program caching to account for transform feedback varyings

Updated shader program caching to account for transform feedback varyings

BUG=429053
Committed: https://crrev.com/63ead731bc142b8c0f202e07bc2879fcc2984fb9
Cr-Commit-Position: refs/heads/master@{#320607}

[bajones, 2015-03-12 23:18:21]

bajones@chromium.org changed reviewers:
+ piman@chromium.org, zmo@chromium.org

[bajones, 2015-03-12 23:18:22]

PTAL. Transform feedback varyings must be taken into account for program binary
caching since they affect the linking process.

[Zhenyao Mo, 2015-03-12 23:50:58]

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
File gpu/command_buffer/service/program_cache.cc (right):

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_cache.cc:99: size_t
CalculateVaryingsSize(const std::vector<std::string>* varyings) {
nit: a line before this function.

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_cache.cc:103:
std::vector<std::string>::const_iterator it;
No need to expose iterator outside for loop.

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_cache.cc:124: + sizeof(GLenum);
nit: sizeof(transform_feedback_buffer_mode)

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_cache.cc:149:
std::vector<std::string>::const_iterator it;
iterator should not be exposed outside the for loop here.  You could use c++11
auto& for simplicity.

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_cache.cc:154:
memcpy(&buffer.get()[current_pos], it->c_str(), name_size);
You need to insert a ' ' in between two varyings.  Otherwise ["ax", "b"] will be
identified as a hit as ["a", "xb"].  You need to modify the
CalculateVaryingsSize() to count in such whitespaces.

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
File gpu/command_buffer/service/program_manager.h (right):

https://codereview.chromium.org/998423005/diff/1/gpu/command_buffer/service/p...
gpu/command_buffer/service/program_manager.h:254: const GLenum
transform_feedback_buffer_mode() const {
no need for const before GLenum

[bajones, 2015-03-13 17:55:05]

Thanks for the feedback, zmo@! Addressed everything in the latest patch.

[Zhenyao Mo, 2015-03-13 18:01:42]

LGTM

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_cache.cc (right):

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_cache.cc:150: const size_t name_size =
it->length();
you could just set name_size to it->length() + 1, because c_str() ends with the
'\0', so you don't have to add it manually.

[Zhenyao Mo, 2015-03-13 18:02:11]

On 2015/03/13 18:01:42, Zhenyao Mo wrote:
> LGTM
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/program_cache.cc (right):
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_cache.cc:150: const size_t name_size =
> it->length();
> you could just set name_size to it->length() + 1, because c_str() ends with
the
> '\0', so you don't have to add it manually.

I mean, whitespace or null, really it doesn't matter.

[bajones, 2015-03-13 18:20:36]

The CQ bit was checked by bajones@chromium.org

[commit-bot: I haz the power, 2015-03-13 18:21:15]

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

[piman, 2015-03-13 18:32:10]

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_cache.cc (right):

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_cache.cc:100: if (!varyings) {
It's never NULL in production. Remove (and fix tests)?

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_cache.cc:104: for (auto it =
varyings->begin(); it != varyings->end(); ++it) {
nit: you can use range loops:
for (auto& varying : *varyings) {
  total += 1 + varying.length();
}

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_cache.cc:149: ++it) {
nit: same here, range loops would help readability:
for (auto& varying : transform_feedback_varyings) {
  ...
}

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_cache.h (right):

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_cache.h:46: const std::vector<std::string>*
transform_feedback_varyings,
nit: you can use const std::vector<std::string>& (here and below). It's never
NULL in production.

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
File gpu/command_buffer/service/program_cache_unittest.cc (right):

https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/program_cache_unittest.cc:137: }
Can you add a test for the new code? If you change the varyings, cache should be
invalidated.

[bajones, 2015-03-13 18:35:16]

The CQ bit was unchecked by bajones@chromium.org

[bajones, 2015-03-13 21:51:51]

On 2015/03/13 at 18:32:10, piman wrote:
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/program_cache.cc (right):
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_cache.cc:100: if (!varyings) {
> It's never NULL in production. Remove (and fix tests)?
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_cache.cc:104: for (auto it =
varyings->begin(); it != varyings->end(); ++it) {
> nit: you can use range loops:
> for (auto& varying : *varyings) {
>   total += 1 + varying.length();
> }
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_cache.cc:149: ++it) {
> nit: same here, range loops would help readability:
> for (auto& varying : transform_feedback_varyings) {
>   ...
> }
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/program_cache.h (right):
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_cache.h:46: const std::vector<std::string>*
transform_feedback_varyings,
> nit: you can use const std::vector<std::string>& (here and below). It's never
NULL in production.
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> File gpu/command_buffer/service/program_cache_unittest.cc (right):
> 
>
https://codereview.chromium.org/998423005/diff/20001/gpu/command_buffer/servi...
> gpu/command_buffer/service/program_cache_unittest.cc:137: }
> Can you add a test for the new code? If you change the varyings, cache should
be invalidated.

Thanks for the feedback! Addressed all of the above, please take another look.

[piman, 2015-03-13 22:31:31]

LGTM, thanks!

[bajones, 2015-03-13 22:37:16]

The CQ bit was checked by bajones@chromium.org

[bajones, 2015-03-13 22:37:17]

The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org
Link to the patchset: https://codereview.chromium.org/998423005/#ps40001 (title:
"Addressed piman@'s feedback")

[commit-bot: I haz the power, 2015-03-13 22:38:04]

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

[commit-bot: I haz the power, 2015-03-14 00:12:56]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-03-14 00:14:03]

Patchset 3 (id:??) landed as
https://crrev.com/63ead731bc142b8c0f202e07bc2879fcc2984fb9
Cr-Commit-Position: refs/heads/master@{#320607}