gpu: Perf test to check costs of texture upload at different stages.

gpu: Perf test to check costs of texture upload at different stages.

Simple test that uploads and draws a texture on an offscreen surface.
The upload/draw code is run multiple times, timings are collected and the
average elapsed milliseconds are printed out using the perf_test framework.

The test target has not been added to the perf tests the are automatically run yet
and has to be manually run.

BUG=423481
Committed: https://crrev.com/3e60aaa94331d8f7a80097b472bba3eb0b177116
Cr-Commit-Position: refs/heads/master@{#313756}

[reveman, 2015-01-27 13:33:42]

reveman@chromium.org changed reviewers:
+ reveman@chromium.org

[reveman, 2015-01-27 13:33:43]

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:172: const GLenum type =
GL_UNSIGNED_BYTE;
Should format, type be passed as parameters instead? They describe |pixels| so
it seems a bit awkward to make an assumption about the format of |pixels| here..

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:180:
timings.push_back(std::make_pair("upload_image",
Let's name this "teximage2d" instead as discussed.

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:193: std::make_pair("draw_image",
draw_timer.Elapsed().InMillisecondsF()));
nit: "drawarrays"

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:206: glReadPixels(0, 0, size_.width(),
size_.height(), format, type,
Is the format passed to read pixels really supposed to be the same as
TexImage2D? ReadPixels doesn't support all formats the TexImage2D does... I feel
like ReadPixels format should always be the same and not affected by the texture
format we're testing.

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:214: const gfx::Size size_;  // for the
offscreen surface and the texture
nit: s/size_/kSize/

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:229: std::map<std::string, float>
timings;
nit: s/timings/measurements/

and maybe add a "Measurement" struct instead of using std::pair in a number of
places. Wdyt?

[Daniele Castagna, 2015-01-27 19:00:42]

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:172: const GLenum type =
GL_UNSIGNED_BYTE;
On 2015/01/27 13:33:43, reveman (OOO Jan 28 - Feb 9) wrote:
> Should format, type be passed as parameters instead? They describe |pixels| so
> it seems a bit awkward to make an assumption about the format of |pixels|
here..

Done.

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:180:
timings.push_back(std::make_pair("upload_image",
On 2015/01/27 13:33:42, reveman (OOO Jan 28 - Feb 9) wrote:
> Let's name this "teximage2d" instead as discussed.

Done.

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:193: std::make_pair("draw_image",
draw_timer.Elapsed().InMillisecondsF()));
On 2015/01/27 13:33:42, reveman (OOO Jan 28 - Feb 9) wrote:
> nit: "drawarrays"

Done.

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:206: glReadPixels(0, 0, size_.width(),
size_.height(), format, type,
On 2015/01/27 13:33:43, reveman (OOO Jan 28 - Feb 9) wrote:
> Is the format passed to read pixels really supposed to be the same as
> TexImage2D? ReadPixels doesn't support all formats the TexImage2D does... I
feel
> like ReadPixels format should always be the same and not affected by the
texture
> format we're testing.

You're right, it's not supposed to be the same as the one passed to TexImage2D.
I'm going to read GL_RGBA and then assert that the texture format is the same.
In this way this CL doesn't need to contain any code to convert pixels in
different formats.

Let me know if this works for you.

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:214: const gfx::Size size_;  // for the
offscreen surface and the texture
On 2015/01/27 13:33:43, reveman (OOO Jan 28 - Feb 9) wrote:
> nit: s/size_/kSize/

Are you sure? This is not a compile time constant:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Constant_Names

https://codereview.chromium.org/871093002/diff/1/gpu/perftests/texture_upload...
gpu/perftests/texture_upload_perftest.cc:229: std::map<std::string, float>
timings;
On 2015/01/27 13:33:43, reveman (OOO Jan 28 - Feb 9) wrote:
> nit: s/timings/measurements/
> 
> and maybe add a "Measurement" struct instead of using std::pair in a number of
> places. Wdyt?

Done.

[reveman, 2015-01-27 22:58:31]

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:157: float wall_time;
Please use base::TimeDelta.

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:184: Measurement{"teximage2d",
tex_timer.Elapsed().InMillisecondsF()});
Please check what part of c++11 is allowed in chromium code.

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:235: std::map<std::string, Measurement>
measurements;  // indexed by name
Vector maybe?

[Daniele Castagna, 2015-01-28 17:28:38]

dcastagna@chromium.org changed reviewers:
+ piman@chromium.org

[Daniele Castagna, 2015-01-28 17:28:39]

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:157: float wall_time;
On 2015/01/27 22:58:31, reveman (OOO Jan 28 - Feb 9) wrote:
> Please use base::TimeDelta.

Done.

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:184: Measurement{"teximage2d",
tex_timer.Elapsed().InMillisecondsF()});
On 2015/01/27 22:58:31, reveman (OOO Jan 28 - Feb 9) wrote:
> Please check what part of c++11 is allowed in chromium code.

Removed everything that is blacklisted. Non-Static Class Member Initializers are
allowed.

https://codereview.chromium.org/871093002/diff/20001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:235: std::map<std::string, Measurement>
measurements;  // indexed by name
On 2015/01/27 22:58:31, reveman (OOO Jan 28 - Feb 9) wrote:
> Vector maybe?

Done.

[piman, 2015-01-28 18:35:16]

LGTM overall, some nits, some suggestions for follow-up.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:21: namespace gfx {
nit: why gfx? gpu instead maybe?

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:174: base::ElapsedTimer tex_timer;
Maybe as a follow-up: it would be useful also to bracket those within GL timer
queries so that we can measure GPU time, if any. Worth making a similar helper
class.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:219: const gfx::Size size_;  // for the
offscreen surface and the texture
Maybe as a follow-up: I think it would be useful to vary the size to separate
fixed costs from per-pixel costs.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:242: [&m](const Measurement& m2) {
return m2.name == m.name; });
nit: Maybe a hash map would help?

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:260: }  // namespace cc
nit: Please use the same namespace as the opening bracket.

[Daniele Castagna, 2015-01-28 19:23:04]

Thank you Antoine for looking into this so quickly.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
File gpu/perftests/texture_upload_perftest.cc (right):

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:21: namespace gfx {
On 2015/01/28 18:35:16, piman (Very slow to review) wrote:
> nit: why gfx? gpu instead maybe?

I originally wrote this under ui/gl. Then I moved it here and forgot to use the
appropriate namespace. Fixed.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:174: base::ElapsedTimer tex_timer;
On 2015/01/28 18:35:16, piman (Very slow to review) wrote:
> Maybe as a follow-up: it would be useful also to bracket those within GL timer
> queries so that we can measure GPU time, if any. Worth making a similar helper
> class.

Acknowledged.

Measuerement could be the helper class and take care of measuring wall time, gl
time and cpu time.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:219: const gfx::Size size_;  // for the
offscreen surface and the texture
On 2015/01/28 18:35:16, piman (Very slow to review) wrote:
> Maybe as a follow-up: I think it would be useful to vary the size to separate
> fixed costs from per-pixel costs.

Acknowledged.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:242: [&m](const Measurement& m2) {
return m2.name == m.name; });
On 2015/01/28 18:35:16, piman (Very slow to review) wrote:
> nit: Maybe a hash map would help?

The previous patch I uploaded was using a map, reveman@ suggested to use a
vector.
I'm going to use a SmallMap so it has the convenient syntax of a map while still
being backed by a vector when there are just a few elements.

https://codereview.chromium.org/871093002/diff/40001/gpu/perftests/texture_up...
gpu/perftests/texture_upload_perftest.cc:260: }  // namespace cc
On 2015/01/28 18:35:16, piman (Very slow to review) wrote:
> nit: Please use the same namespace as the opening bracket.

Done.

[Daniele Castagna, 2015-01-29 16:20:45]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-01-29 16:21:45]

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

[commit-bot: I haz the power, 2015-01-29 16:31:55]

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

[commit-bot: I haz the power, 2015-01-29 16:31:56]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Daniele Castagna, 2015-01-29 19:09:10]

The CQ bit was checked by dcastagna@chromium.org

[commit-bot: I haz the power, 2015-01-29 19:10:18]

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

[commit-bot: I haz the power, 2015-01-29 19:24:15]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-01-29 19:25:11]

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