Optimized Texture::Update() function.

Optimized Texture::Update() function.

Texture::Update() was doing a lot of calculations that did not change
much per frame. Some of the calculations such as whether or not has
non-power of 2 dimensions has been moved to when the texture
levels are set since they do not change. Others are only recalculated
when a dirty bit is set.

BUG=420152
TEST=trybots

Committed: https://crrev.com/ecc73c25adfe05e947c2e4aa1f814cfcf19b75ba
Cr-Commit-Position: refs/heads/master@{#299181}

[David Yen, 2014-10-06 23:12:56]

dyen@chromium.org changed reviewers:
+ vmiura@chromium.org, zmo@chromium.org

[David Yen, 2014-10-06 23:12:57]

I've updated the Texture::Update() value to use precalculate values that are
lazily computed as the LevelInfos are set internally.

On my Nexus 5, the Texture::Update() function has gone from 0.0268ms to a
negligible 0.00291ms.

[vmiura, 2014-10-07 00:13:04]

In general I think the counting of states is a bit error prone, and I wonder if
we could instead keep a flag 'completeness_changed_' and lazily compute the
completeness as needed.

Although as a simpler thing, we could greatly cut the work of Texture::Update()
by splitting the min_filter_/mag_filter_ parts into a separate
Texture::UpdateFilterState(), and only calling that for SetParameteri()
GL_TEXTURE_MIN_FILTER & GL_TEXTURE_MAG_FILTER.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:455: const GLsizei mip_depth =
std::max(1, level0_face.depth >> level);
Do we need to handle the case of width/height/depth == 0?

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:600: for (size_t i = face_index; i
< level_infos_.size(); ++i) {
I think this compares face 0 to itself, so I think we could skip most of that.

Initially I guess a face is incomplete, so I wonder if this code will count
faces that are initialized to invalid values correctly, i.e when
prev_face_status == new_face_status == false.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:630: const GLsizei num_mip_levels
= num_mip_levels_[face_index];
If num_mip_levels changes, for example decreasing, I'm not sure we're correctly
updating num_incomplete_mips_.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:651: num_incomplete_mips_ +=
new_mip_status ? -1 : 1;
As above, I'm not sure we will count correctly when initial state is incomplete,
and new state is incomplete.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.h:358: std::vector<GLsizei>
num_mip_levels_;
Is this number of mip levels for each face?

If so I think it would be nice to add a FaceInfo struct and move level infos to
there.

struct FaceInfo {
  std::vector<LevelInfo> level_infos;
  GLsizei num_mip_levels_;
};

[David Yen, 2014-10-09 20:20:17]

I reverted to the old Texture::Update() function and modified it to use my new
complete functions and also use a dirty bit. I didn't end up separating the
Texture::Update() function since the dirty bit will always be off during
SetParamateri().

It does kind of bother me that the mips get recalculated over and over again
when we are setting each level. It would be nice if we could batch set all the
faces/levels and update the calculations in 1 go.

From the way it is structured now I don't think it is possible, current we
always call all the various Update() functions within SetLevelInfo(). Perhaps
someone can think of a better way.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:455: const GLsizei mip_depth =
std::max(1, level0_face.depth >> level);
On 2014/10/07 00:13:03, vmiura wrote:
> Do we need to handle the case of width/height/depth == 0?

That check is already done before this function is called, but technically it
would be more correct if we checked in here I suppose... although that would add
an extra branch.

Another way to look at it is that we are assuming level0_face is a proper face
at least.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:600: for (size_t i = face_index; i
< level_infos_.size(); ++i) {
On 2014/10/07 00:13:03, vmiura wrote:
> I think this compares face 0 to itself, so I think we could skip most of that.
> 
> Initially I guess a face is incomplete, so I wonder if this code will count
> faces that are initialized to invalid values correctly, i.e when
> prev_face_status == new_face_status == false.

No longer relevant, using dirty bit now.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:630: const GLsizei num_mip_levels
= num_mip_levels_[face_index];
On 2014/10/07 00:13:03, vmiura wrote:
> If num_mip_levels changes, for example decreasing, I'm not sure we're
correctly
> updating num_incomplete_mips_.

No longer relevant, using dirty bit now.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.cc:651: num_incomplete_mips_ +=
new_mip_status ? -1 : 1;
On 2014/10/07 00:13:03, vmiura wrote:
> As above, I'm not sure we will count correctly when initial state is
incomplete,
> and new state is incomplete.

No longer relevant, using dirty bit now.

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
File gpu/command_buffer/service/texture_manager.h (right):

https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
gpu/command_buffer/service/texture_manager.h:358: std::vector<GLsizei>
num_mip_levels_;
On 2014/10/07 00:13:03, vmiura wrote:
> Is this number of mip levels for each face?
> 
> If so I think it would be nice to add a FaceInfo struct and move level infos
to
> there.
> 
> struct FaceInfo {
>   std::vector<LevelInfo> level_infos;
>   GLsizei num_mip_levels_;
> };

Done.

[David Yen, 2014-10-10 17:01:19]

On 2014/10/09 20:20:17, David Yen wrote:
> I reverted to the old Texture::Update() function and modified it to use my new
> complete functions and also use a dirty bit. I didn't end up separating the
> Texture::Update() function since the dirty bit will always be off during
> SetParamateri().
> 
> It does kind of bother me that the mips get recalculated over and over again
> when we are setting each level. It would be nice if we could batch set all the
> faces/levels and update the calculations in 1 go.
> 
> From the way it is structured now I don't think it is possible, current we
> always call all the various Update() functions within SetLevelInfo(). Perhaps
> someone can think of a better way.
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> File gpu/command_buffer/service/texture_manager.cc (right):
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> gpu/command_buffer/service/texture_manager.cc:455: const GLsizei mip_depth =
> std::max(1, level0_face.depth >> level);
> On 2014/10/07 00:13:03, vmiura wrote:
> > Do we need to handle the case of width/height/depth == 0?
> 
> That check is already done before this function is called, but technically it
> would be more correct if we checked in here I suppose... although that would
add
> an extra branch.
> 
> Another way to look at it is that we are assuming level0_face is a proper face
> at least.
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> gpu/command_buffer/service/texture_manager.cc:600: for (size_t i = face_index;
i
> < level_infos_.size(); ++i) {
> On 2014/10/07 00:13:03, vmiura wrote:
> > I think this compares face 0 to itself, so I think we could skip most of
that.
> > 
> > Initially I guess a face is incomplete, so I wonder if this code will count
> > faces that are initialized to invalid values correctly, i.e when
> > prev_face_status == new_face_status == false.
> 
> No longer relevant, using dirty bit now.
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> gpu/command_buffer/service/texture_manager.cc:630: const GLsizei
num_mip_levels
> = num_mip_levels_[face_index];
> On 2014/10/07 00:13:03, vmiura wrote:
> > If num_mip_levels changes, for example decreasing, I'm not sure we're
> correctly
> > updating num_incomplete_mips_.
> 
> No longer relevant, using dirty bit now.
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> gpu/command_buffer/service/texture_manager.cc:651: num_incomplete_mips_ +=
> new_mip_status ? -1 : 1;
> On 2014/10/07 00:13:03, vmiura wrote:
> > As above, I'm not sure we will count correctly when initial state is
> incomplete,
> > and new state is incomplete.
> 
> No longer relevant, using dirty bit now.
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> File gpu/command_buffer/service/texture_manager.h (right):
> 
>
https://codereview.chromium.org/633773002/diff/1/gpu/command_buffer/service/t...
> gpu/command_buffer/service/texture_manager.h:358: std::vector<GLsizei>
> num_mip_levels_;
> On 2014/10/07 00:13:03, vmiura wrote:
> > Is this number of mip levels for each face?
> > 
> > If so I think it would be nice to add a FaceInfo struct and move level infos
> to
> > there.
> > 
> > struct FaceInfo {
> >   std::vector<LevelInfo> level_infos;
> >   GLsizei num_mip_levels_;
> > };
> 
> Done.

After getting the profiler working again, it looks like this change reduces the
Texture::Update() command from 0.0174ms~0.0258ms to 0.00804ms~0.0128ms. I'm not
sure why it's not as significant as my previous change, in theory when calling
Texture::SetParameteri() none of the texture bits should be dirty.

PTAL.

[vmiura, 2014-10-10 17:14:18]

> After getting the profiler working again, it looks like this change reduces
the
> Texture::Update() command from 0.0174ms~0.0258ms to 0.00804ms~0.0128ms. I'm
not
> sure why it's not as significant as my previous change, in theory when calling
> Texture::SetParameteri() none of the texture bits should be dirty.

It's possible you're getting different results as the MLB page changes each day.

I made an archive of the page 2 weeks ago to make sure we measure the same
content
each time.  I'll see if I can upload it, or just send you a copy.

[vmiura, 2014-10-10 17:40:37]

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

https://codereview.chromium.org/633773002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_manager.cc:840: cube_complete_ = false;
I think you need to save the result, so if Texture::Update is called repeatedly
we won't lose the state.

i.e.

if cube_complete_ && texture_level0_dirty_:
  texture_level0_dirty_ = false
  texture_level0_complete_ = true
  if level0s not complete:
    texture_level0_complete_ = false

cube_complete_ &= texture_level0_complete_

https://codereview.chromium.org/633773002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_manager.cc:862: texture_complete_ = false;
Same as above.

if texture_complete_ && texture_mips_dirty_:
  texture_mips_dirty_ = false
  texture_mips_complete_ = true
  for each mip:
    if mip not complete:
      texture_mips_complete_ = false

texture_complete_ &= texture_mips_complete_

[David Yen, 2014-10-10 17:51:00]

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

https://codereview.chromium.org/633773002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_manager.cc:840: cube_complete_ = false;
On 2014/10/10 17:40:37, vmiura wrote:
> I think you need to save the result, so if Texture::Update is called
repeatedly
> we won't lose the state.
> 
> i.e.
> 
> if cube_complete_ && texture_level0_dirty_:
>   texture_level0_dirty_ = false
>   texture_level0_complete_ = true
>   if level0s not complete:
>     texture_level0_complete_ = false
> 
> cube_complete_ &= texture_level0_complete_

Done.

https://codereview.chromium.org/633773002/diff/20001/gpu/command_buffer/servi...
gpu/command_buffer/service/texture_manager.cc:862: texture_complete_ = false;
On 2014/10/10 17:40:37, vmiura wrote:
> Same as above.
> 
> if texture_complete_ && texture_mips_dirty_:
>   texture_mips_dirty_ = false
>   texture_mips_complete_ = true
>   for each mip:
>     if mip not complete:
>       texture_mips_complete_ = false
> 
> texture_complete_ &= texture_mips_complete_

Done.

[vmiura, 2014-10-10 18:04:30]

lgtm, but please wait for Mo's check too.

Thanks!

[Zhenyao Mo, 2014-10-10 19:38:54]

LGTM

Please also update CL description to reflect the new change.

[David Yen, 2014-10-10 19:45:57]

On 2014/10/10 19:38:54, Zhenyao Mo wrote:
> LGTM
> 
> Please also update CL description to reflect the new change.

done.

[David Yen, 2014-10-10 19:46:04]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2014-10-10 19:47:07]

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

[commit-bot: I haz the power, 2014-10-10 20:44:10]

Committed patchset #3 (id:200001)

[commit-bot: I haz the power, 2014-10-10 20:44:47]

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