cc: Move computing borderless max texture size to function.

cc: Move computing borderless max texture size to function.

This patch moves computing borderless max texture size to its own
function and calls it when tiling gets updated, i.e. when
RecomputeNumTiles() gets called. This will save computing max texture
size without border each time in few frequently called functions and
makes code more readable.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[prashant.n, 2016-06-14 04:57:08]

Description was changed from

==========
cc: Compute borderless max texture size only when tiling gets updated.

This patch computes borderless max texture size when tiling gets
updated, i.e. when RecomputeNumTiles() gets called. This saves
computing each time in few frequently called functions.

BUG=None
==========

to

==========
cc: Compute borderless max texture size only when tiling gets updated.

This patch computes borderless max texture size when tiling gets
updated, i.e. when RecomputeNumTiles() gets called. This saves
computing each time in few frequently called functions.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-06-14 04:59:40]

prashant.n@samsung.com changed reviewers:
+ vmpstr@chromium.org

[prashant.n, 2016-06-14 04:59:41]

ptal.

[vmpstr, 2016-06-14 22:40:18]

On 2016/06/14 04:59:41, prashant.n wrote:
> ptal.

This looks fine. Do you have any perf examples of where this helps? I'm curious
about how much of a speed up this is.

[vmpstr, 2016-06-14 22:40:54]

https://codereview.chromium.org/2061103002/diff/1/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/2061103002/diff/1/cc/base/tiling_data.cc#newc...
cc/base/tiling_data.cc:25: int ComputeBorderlessTextureSize(int
max_texture_size, int border_texels) {
I don't think you need a separate function if you're just calling this in two
places next to each other.

https://codereview.chromium.org/2061103002/diff/1/cc/base/tiling_data.cc#newc...
cc/base/tiling_data.cc:56: TilingData::~TilingData() {}
= default as well

[prashant.n, 2016-06-15 00:37:05]

https://codereview.chromium.org/2061103002/diff/1/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/2061103002/diff/1/cc/base/tiling_data.cc#newc...
cc/base/tiling_data.cc:25: int ComputeBorderlessTextureSize(int
max_texture_size, int border_texels) {
On 2016/06/14 22:40:54, vmpstr wrote:
> I don't think you need a separate function if you're just calling this in two
> places next to each other. 

I thought having function would help signify the functionality. May be we can
move the DCHECK_GT to this function and remove from calling end.

https://codereview.chromium.org/2061103002/diff/1/cc/base/tiling_data.cc#newc...
cc/base/tiling_data.cc:56: TilingData::~TilingData() {}
On 2016/06/14 22:40:54, vmpstr wrote:
> = default as well

I'll make this change.

[prashant.n, 2016-06-15 10:07:30]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 10:07:46]

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

[prashant.n, 2016-06-15 10:09:18]

https://codereview.chromium.org/2061103002/diff/20001/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/2061103002/diff/20001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:292: DCHECK_GT(borderless_max_texture_size_.width(), 0);
Should we have CHECK_GT here instead, as with negative or zero renderable area,
it will crash somewhere else?

[commit-bot: I haz the power, 2016-06-15 10:36:19]

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

[commit-bot: I haz the power, 2016-06-15 10:36:20]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[prashant.n, 2016-06-15 16:34:35]

Looks few tests are broken. 'll take care tomorrow.

[vmpstr, 2016-06-15 20:37:53]

https://codereview.chromium.org/2061103002/diff/20001/cc/base/tiling_data.cc
File cc/base/tiling_data.cc (right):

https://codereview.chromium.org/2061103002/diff/20001/cc/base/tiling_data.cc#...
cc/base/tiling_data.cc:292: DCHECK_GT(borderless_max_texture_size_.width(), 0);
On 2016/06/15 10:09:18, prashant.n wrote:
> Should we have CHECK_GT here instead, as with negative or zero renderable
area,
> it will crash somewhere else?

DCHECK is fine I think. If the calling code needs a CHECK it can check
independently for valid state.

[prashant.n, 2016-07-15 10:36:52]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 10:37:00]

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, 2016-07-15 10:37:01]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-15 13:07:32]

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

[commit-bot: I haz the power, 2016-07-15 13:07:33]

Dry run: This issue passed the CQ dry run.

[prashant.n, 2016-07-15 13:17:24]

ptal

[prashant.n, 2016-07-18 04:43:24]

prashant.n@samsung.com changed reviewers:
+ enne@chromium.org

[prashant.n, 2016-07-18 04:43:25]

+ enne

[enne (OOO), 2016-07-18 18:00:18]

On 2016/06/14 at 22:40:18, vmpstr wrote:
> On 2016/06/14 04:59:41, prashant.n wrote:
> > ptal.
> 
> This looks fine. Do you have any perf examples of where this helps? I'm
curious about how much of a speed up this is.

I'll repeat vmpstr's question here.  I'm not sure that I'd intuitively believe
this was a performance improvement.

[prashant.n, 2016-07-19 00:53:04]

On 2016/07/18 18:00:18, enne wrote:
> On 2016/06/14 at 22:40:18, vmpstr wrote:
> > On 2016/06/14 04:59:41, prashant.n wrote:
> > > ptal.
> > 
> > This looks fine. Do you have any perf examples of where this helps? I'm
> curious about how much of a speed up this is.
> 
> I'll repeat vmpstr's question here.  I'm not sure that I'd intuitively believe
> this was a performance improvement.

This will not improve the perf, but will make computations easy and readable.

[prashant.n, 2016-07-19 02:54:33]

Read above comment as -
This will not improve the perf *much*, but will make computations easy and
readable.

[prashant.n, 2016-07-19 03:14:22]

Description was changed from

==========
cc: Compute borderless max texture size only when tiling gets updated.

This patch computes borderless max texture size when tiling gets
updated, i.e. when RecomputeNumTiles() gets called. This saves
computing each time in few frequently called functions.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Move computing borderless max texture size to function.

This patch moves computing borderless max texture size to its own
function and calls it when tiling gets updated, i.e. when
RecomputeNumTiles() gets called. This will save computing max texture
size without border each time in few frequently called functions and
makes code more readable.

BUG=None
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-07-19 03:15:22]

The commit msg updated to depict the intent of the patch.

[enne (OOO), 2016-07-19 18:46:05]

Sorry to quibble, but I don't know that this makes things more readable, at
least to my eyes.  For example, the TileSizeX/Y functions seemed easier to
understand when two cases were 1 or 2 border texels explicitly, and
FirstBorderTileXIndexFromSrcCoord now obscures some of the math via a member
variable.

[prashant.n, 2016-07-20 03:36:46]

On 2016/07/19 18:46:05, enne wrote:
> Sorry to quibble, but I don't know that this makes things more readable, at
> least to my eyes.  For example, the TileSizeX/Y functions seemed easier to
> understand when two cases were 1 or 2 border texels explicitly, and
> FirstBorderTileXIndexFromSrcCoord now obscures some of the math via a member
> variable.

Hmm. "1 or 2 border texels explicitly" seems reasonable. I would skip this
patch.