Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1225383002:
Added a GLBench for testing performance of vec4 vs scalar for coverage in generated shaders. (Closed)
DescriptionAdded a glBench for testing performance of vec4 vs scalar coverage in generated shaders.
Added bench for timing vec4 vs scalar type for coverage in shaders
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/6104ced165f17eb2f765ace354d5895c0bc890c5
Committed: https://skia.googlesource.com/skia/+/c734e69e8cf94bacaf68d3d8ee3310d1ad1fe8b8
Patch Set 1 #Patch Set 2 : #
Total comments: 39
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #Messages
Total messages: 18 (3 generated)
wangyix@google.com changed reviewers: + joshualitt@google.com, tomhudson@google.com
Looking good, just a huge flock of nits. (Many of which you're inheriting, since I'm not happy with the code you cut-and-pasted from to start this work - not a reflection on what you've done!) https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... File bench/GLVec4ScalarBench.cpp (right): https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:10: #include "SkImageEncoder.h" You don't seem to need any of these 3 includes any more? You might want to explicitly include SkMatrix, SkPoint, SkString - it's open to debate. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:22: /* Please make this /** so doxygen can pick it up. Also, bring the comment up to date to describe what you're actually doing here, and why. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:45: * kDrawMultipier. Outdated comment; cut. If the class comment is good enough and your variable names are good enough you probably don't need to say anything here. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:51: , fVAO(0) { Why explicitly clear fVAO but not fVBO? https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:70: static SkString CoverageSetupToStr(CoverageSetup vboSetup, uint32_t numStages) { Consider tweaking the function name, since you're doing more than just writing "_scalar_" or "_vec4_". https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:92: GrGLuint fVBO; Consider writing out name instead of using abbreviation. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:94: GrGLuint fVAO; ... particularly when two abbreviations are very close together. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:95: GrGLuint fTexture; It makes the name a bit long, but you might want to be explicit that this is a ID or handle for a texture, and add a word to tell us the *semantics* - is this texture the output? Ugh, fFrameBufferTextureID? (Particularly since the object is never obviously used in this code - it's set up, and then later on it's deleted.) https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:100: GrGLuint GLVec4ScalarBench::setupShader(const GrGLContext* ctx, bool useVec4ForCoverage) { Instead of passing the bool here, pass the enum! The type name is good enough your parameter name can be more concise, and the callsite simpler. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:103: // setup vertex shader I'd like to see a "// This shader draws 4 overlapping circles of increasing opacity and decreasing size"-type description somewhere in this function. "// setup vertex shader" is a low-value comment. Slightly better might be "// setup trivial vertex shader to pass through inputs" here, and then the description I suggested above tacked onto the fragment shader. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:170: for (uint32_t i = 0; i < fNumStages; i++) { Why pass in fConverageSetup (or a bool derived from it), but read this out of a member variable? It looks as though passing in both values would let you move this out of the class into an anonymous namespace or static function? https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:205: //printf("\n%s\n", fshaderTxt.c_str()); Remove commented-out code. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:236: const SkMatrix* viewMatrices) { Nit: fix indentation https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:237: // Constants for our various shader programs This comment is not very high-value. Omit? https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:253: // set colors Bad evil comment. Expurgate! https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:263: // setup VBO Low-value comment. What is it? Or why are we doing these things? https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:281: // setup matrices Useless comment. What matrices? Why? Or eliminate! (Likewise the other fragmentary comments in this function.) https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:308: //const char* filename = "/data/local/tmp/out.png"; Delete commented-out code. In theory you could probably leave the #if'd out code here, however, since it's useful and clear - but don't VisualBench and nanobench have command line arguments that say "write out the images you generate"?!
PTAL, I fixed all the nits except the VBO, VAO abbreviations, those were left in.
The questions about shader rewriting (154, 181) are really questions of taste / opinion - I'm probing to measure yours. I think you missed half of my comment at (308). https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... File bench/GLVec4ScalarBench.cpp (right): https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:60: const GrGLContext* onGetGLContext(const GrGLContext*) override; Once we've started reviewing, it's good for the reviewer to understand all your changes. Why are you no longer overriding this? OK, reading the text of the function definition it looks like it was only relevant to the previous test, and the default behavior is sufficient here. But then your PTAL message could have *also* mentioned this change. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:308: //const char* filename = "/data/local/tmp/out.png"; On 2015/07/10 19:23:55, tomhudson wrote: > Delete commented-out code. > In theory you could probably leave the #if'd out code here, however, since it's > useful and clear - but don't VisualBench and nanobench have command line > arguments that say "write out the images you generate"?! Again: How is your #0 different than the command-line args args to the bench programs to force them to write out images? Isn't this redundant? https://codereview.chromium.org/1225383002/diff/40001/bench/GLVec4ScalarBench... File bench/GLVec4ScalarBench.cpp (right): https://codereview.chromium.org/1225383002/diff/40001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:154: switch (fCoverageSetup) { These two cases are identical *except for* two points: float vs vec4, and 1.0 vs vec4(1.0). How's the maintainability / legibility tradeoff of setting a couple of strings in the switch and then printing once, with %s substitutions. https://codereview.chromium.org/1225383002/diff/40001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:181: switch (fCoverageSetup) { As above, can we merge most of the text of these two cases, or is that too hard to read?
https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... File bench/GLVec4ScalarBench.cpp (right): https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:10: #include "SkImageEncoder.h" On 2015/07/10 19:23:54, tomhudson wrote: > You don't seem to need any of these 3 includes any more? > You might want to explicitly include SkMatrix, SkPoint, SkString - it's open to > debate. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:22: /* On 2015/07/10 19:23:54, tomhudson wrote: > Please make this /** so doxygen can pick it up. > Also, bring the comment up to date to describe what you're actually doing here, > and why. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:45: * kDrawMultipier. On 2015/07/10 19:23:54, tomhudson wrote: > Outdated comment; cut. > If the class comment is good enough and your variable names are good enough you > probably don't need to say anything here. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:51: , fVAO(0) { On 2015/07/10 19:23:55, tomhudson wrote: > Why explicitly clear fVAO but not fVBO? Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:60: const GrGLContext* onGetGLContext(const GrGLContext*) override; On 2015/07/13 16:00:13, tomhudson wrote: > Once we've started reviewing, it's good for the reviewer to understand all your > changes. Why are you no longer overriding this? > > OK, reading the text of the function definition it looks like it was only > relevant to the previous test, and the default behavior is sufficient here. But > then your PTAL message could have *also* mentioned this change. Yep that was the reason. The override was specific to the previous test. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:70: static SkString CoverageSetupToStr(CoverageSetup vboSetup, uint32_t numStages) { On 2015/07/10 19:23:55, tomhudson wrote: > Consider tweaking the function name, since you're doing more than just writing > "_scalar_" or "_vec4_". Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:92: GrGLuint fVBO; On 2015/07/10 19:23:54, tomhudson wrote: > Consider writing out name instead of using abbreviation. Vao and Vbo are extremely common occurrences in OpenGL code and should n't cause any confusion. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:94: GrGLuint fVAO; On 2015/07/10 19:23:55, tomhudson wrote: > ... particularly when two abbreviations are very close together. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:95: GrGLuint fTexture; On 2015/07/10 19:23:55, tomhudson wrote: > It makes the name a bit long, but you might want to be explicit that this is a > ID or handle for a texture, and add a word to tell us the *semantics* - is this > texture the output? Ugh, fFrameBufferTextureID? > > (Particularly since the object is never obviously used in this code - it's set > up, and then later on it's deleted.) Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:100: GrGLuint GLVec4ScalarBench::setupShader(const GrGLContext* ctx, bool useVec4ForCoverage) { On 2015/07/10 19:23:55, tomhudson wrote: > Instead of passing the bool here, pass the enum! The type name is good enough > your parameter name can be more concise, and the callsite simpler. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:103: // setup vertex shader On 2015/07/10 19:23:54, tomhudson wrote: > I'd like to see a "// This shader draws 4 overlapping circles of increasing > opacity and decreasing size"-type description somewhere in this function. > > "// setup vertex shader" is a low-value comment. > Slightly better might be "// setup trivial vertex shader to pass through inputs" > here, and then the description I suggested above tacked onto the fragment > shader. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:170: for (uint32_t i = 0; i < fNumStages; i++) { On 2015/07/10 19:23:54, tomhudson wrote: > Why pass in fConverageSetup (or a bool derived from it), but read this out of a > member variable? > > It looks as though passing in both values would let you move this out of the > class into an anonymous namespace or static function? Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:205: //printf("\n%s\n", fshaderTxt.c_str()); On 2015/07/10 19:23:55, tomhudson wrote: > Remove commented-out code. Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:236: const SkMatrix* viewMatrices) { On 2015/07/10 19:23:54, tomhudson wrote: > Nit: fix indentation Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:237: // Constants for our various shader programs On 2015/07/10 19:23:55, tomhudson wrote: > This comment is not very high-value. Omit? Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:253: // set colors On 2015/07/10 19:23:55, tomhudson wrote: > Bad evil comment. Expurgate! Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:263: // setup VBO On 2015/07/10 19:23:54, tomhudson wrote: > Low-value comment. What is it? Or why are we doing these things? Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:281: // setup matrices On 2015/07/10 19:23:55, tomhudson wrote: > Useless comment. > What matrices? Why? Or eliminate! > > (Likewise the other fragmentary comments in this function.) Done. https://codereview.chromium.org/1225383002/diff/20001/bench/GLVec4ScalarBench... bench/GLVec4ScalarBench.cpp:308: //const char* filename = "/data/local/tmp/out.png"; On 2015/07/13 16:00:13, tomhudson wrote: > On 2015/07/10 19:23:55, tomhudson wrote: > > Delete commented-out code. > > In theory you could probably leave the #if'd out code here, however, since > it's > > useful and clear - but don't VisualBench and nanobench have command line > > arguments that say "write out the images you generate"?! > > Again: How is your #0 different than the command-line args args to the bench > programs to force them to write out images? Isn't this redundant? Using -w when running nanobench does not write out the correct images for some reason (I get blank white images), but changing the code here to #if 1 will write out the correct images. Josh was aware of this and used this code for debug purposes.
The CQ bit was checked by tomhudson@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225383002/40002
Message was sent while issue was closed.
Committed patchset #4 (id:40002) as https://skia.googlesource.com/skia/+/6104ced165f17eb2f765ace354d5895c0bc890c5
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:40002) has been created in https://codereview.chromium.org/1235533004/ by scroggo@google.com. The reason for reverting is: This is breaking a buildbot. See https://chromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Shuttl... We get the following error: ERROR::SHADER::COMPLIATION_FAILED: ERROR: 0:8: '0.f' : Floating-point suffix unsupported prior to GLSL ES 3.00 ERROR: 0:8: '0.f' : syntax error .
On 2015/07/13 19:03:15, scroggo wrote: > A revert of this CL (patchset #4 id:40002) has been created in > https://codereview.chromium.org/1235533004/ by mailto:scroggo@google.com. > > The reason for reverting is: This is breaking a buildbot. See > https://chromegw.corp.google.com/i/client.skia/builders/Test-Win8-MSVC-Shuttl... > > We get the following error: ERROR::SHADER::COMPLIATION_FAILED: ERROR: 0:8: '0.f' > : Floating-point suffix unsupported prior to GLSL ES 3.00 > ERROR: 0:8: '0.f' : syntax error > . Issue should be fixed now, please take a look.
On 2015/07/13 19:52:12, wangyix wrote: > Issue should be fixed now, please take a look. If we wanted to be really thorough, we could have trybotted the other architectures/compilers too...
The CQ bit was checked by tomhudson@google.com
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1225383002/70001
Message was sent while issue was closed.
Committed patchset #5 (id:70001) as https://skia.googlesource.com/skia/+/c734e69e8cf94bacaf68d3d8ee3310d1ad1fe8b8
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:90001) has been created in https://codereview.chromium.org/1239503003/ by scroggo@google.com. The reason for reverting is: Still failing on Windows. e.g. https://uberchromegw.corp.google.com/i/client.skia/builders/Perf-Win8-MSVC-Sh... skbug.com/4053. |
