Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1886523002:
distinguish distinct matrixconvolution benchmarks (Closed)
Descriptiondistinguish distinct matrixconvolution benchmarks
Today they all show as "matrixconvolution", and we probably only log one.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1886523002
Committed: https://skia.googlesource.com/skia/+/20efb41dbbbe1f9fa9cd8f3fd8bd84840b79dbbb
Patch Set 1 #
Total comments: 1
Patch Set 2 : pretty #Patch Set 3 : invert #Messages
Total messages: 16 (6 generated)
Description was changed from ========== distinguish distinct matrixconvolution benchmarks Today they all show as "matrixconvolution", and we probably only log one. BUG=skia: ========== to ========== distinguish distinct matrixconvolution benchmarks Today they all show as "matrixconvolution", and we probably only log one. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
mtklein@chromium.org changed reviewers: + senorblanco@chromium.org
The CQ bit was checked by mtklein@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886523002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886523002/1
Note for Reviewers: The CQ is waiting for an approval. If you believe that the CL is not ready yet, or if you would like to L-G-T-M with comments then please uncheck the CQ checkbox. Waiting for LGTM from valid reviewer(s) till 2016-04-13 02:10 UTC
Doh! Good find. https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... File bench/MatrixConvolutionBench.cpp (right): https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... bench/MatrixConvolutionBench.cpp:17: : fName(SkStringPrintf("matrixconvolution_%d_%d", (int)tileMode, (int)convolveAlpha)) { I hate to be a pain, but could you turn the enum into a string, and convolveAlpha into an optional suffix?
On 2016/04/12 at 21:12:57, senorblanco wrote: > Doh! Good find. > > https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... > File bench/MatrixConvolutionBench.cpp (right): > > https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... > bench/MatrixConvolutionBench.cpp:17: : fName(SkStringPrintf("matrixconvolution_%d_%d", (int)tileMode, (int)convolveAlpha)) { > I hate to be a pain, but could you turn the enum into a string, and convolveAlpha into an optional suffix? Sure thing. Is convolveAlpha really the unusual case?
On 2016/04/12 21:19:32, mtklein wrote: > On 2016/04/12 at 21:12:57, senorblanco wrote: > > Doh! Good find. > > > > > https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... > > File bench/MatrixConvolutionBench.cpp (right): > > > > > https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... > > bench/MatrixConvolutionBench.cpp:17: : > fName(SkStringPrintf("matrixconvolution_%d_%d", (int)tileMode, > (int)convolveAlpha)) { > > I hate to be a pain, but could you turn the enum into a string, and > convolveAlpha into an optional suffix? > > Sure thing. Is convolveAlpha really the unusual case? Good point. FEConvolveMatrix.preserveAlpha (where this comes from) defaults to false, so we should probably assume convolveAlpha defaults to true. so maybe the suffix should be "no_convolve_alpha" or "convolve_alpha_false" or something like that.
LGTM like this, or with convolveAlpha inverted.
On 2016/04/12 at 22:28:38, senorblanco wrote: > On 2016/04/12 21:19:32, mtklein wrote: > > On 2016/04/12 at 21:12:57, senorblanco wrote: > > > Doh! Good find. > > > > > > > > https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... > > > File bench/MatrixConvolutionBench.cpp (right): > > > > > > > > https://codereview.chromium.org/1886523002/diff/1/bench/MatrixConvolutionBenc... > > > bench/MatrixConvolutionBench.cpp:17: : > > fName(SkStringPrintf("matrixconvolution_%d_%d", (int)tileMode, > > (int)convolveAlpha)) { > > > I hate to be a pain, but could you turn the enum into a string, and > > convolveAlpha into an optional suffix? > > > > Sure thing. Is convolveAlpha really the unusual case? > > Good point. FEConvolveMatrix.preserveAlpha (where this comes from) defaults to false, so we should probably assume convolveAlpha defaults to true. so maybe the suffix should be "no_convolve_alpha" or "convolve_alpha_false" or something like that. (Done.)
The CQ bit was checked by mtklein@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from senorblanco@chromium.org Link to the patchset: https://codereview.chromium.org/1886523002/#ps40001 (title: "invert")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1886523002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1886523002/40001
Message was sent while issue was closed.
Description was changed from ========== distinguish distinct matrixconvolution benchmarks Today they all show as "matrixconvolution", and we probably only log one. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== distinguish distinct matrixconvolution benchmarks Today they all show as "matrixconvolution", and we probably only log one. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/20efb41dbbbe1f9fa9cd8f3fd8bd84840b79dbbb ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://skia.googlesource.com/skia/+/20efb41dbbbe1f9fa9cd8f3fd8bd84840b79dbbb |
