Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1497843003:
add bench for sprite-like drawing w/ imagefilters (Closed)
Descriptionadd bench for sprite-like drawing w/ imagefilters
visualbench run on Mac Pro
curr/maxrss loops min median mean max stddev samples config bench
96/96 MB 16 412µs 413µs 413µs 414µs 0% ▄▁█▅▅▅▅▆▅▃▅ gpu warmupbench
98/98 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▁▂▇█▆▅▇▄▃▃▃ gpu image-filter-sprite-draw-image
100/100 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▆▇▅▄▁▆▅█▅▄▅ gpu image-filter-sprite-draw-bitmap
100/100 MB 32 547µs 548µs 590µs 1.01ms 24% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-sprite
BUG=skia:
Committed: https://skia.googlesource.com/skia/+/6f2753deee7b8cd03224e3df91dfea21bbeb9f7f
Patch Set 1 #
Total comments: 8
Patch Set 2 : fix no-gpu build #Patch Set 3 : switch to much faster filter (to emphasize overhead) #
Total comments: 2
Patch Set 4 : fix comment #Patch Set 5 : increase image size to make timing less sensitive to cache sizes etc. #Messages
Total messages: 33 (12 generated)
reed@google.com changed reviewers: + robertphillips@google.com, senorblanco@google.com
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1497843003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497843003/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 2015-12-04 01:02 UTC
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
See also https://codereview.chromium.org/1401173006/, if it gives you any ideas.
On 2015/12/03 19:21:30, Stephen White wrote: > See also https://codereview.chromium.org/1401173006/, if it gives you any ideas. In particular, using an SkOffsetImageFilter is much lighter weight than using a blur, so it can more easily show the delta caused by the extra blit.
The CQ bit was checked by reed@google.com
The CQ bit was unchecked by reed@google.com
On 2015/12/03 19:21:30, Stephen White wrote: > See also https://codereview.chromium.org/1401173006/, if it gives you any ideas. Wow, had totally missed that CL. Would you like to land it instead, or have me work on this one?
On 2015/12/03 19:27:23, reed1 wrote: > On 2015/12/03 19:21:30, Stephen White wrote: > > See also https://codereview.chromium.org/1401173006/, if it gives you any > ideas. > > Wow, had totally missed that CL. Would you like to land it instead, or have me > work on this one? Well, I do know that it does show the diffs. I could land it, then you could add the drawImage() case maybe?
On 2015/12/03 19:27:23, reed1 wrote: > On 2015/12/03 19:21:30, Stephen White wrote: > > See also https://codereview.chromium.org/1401173006/, if it gives you any > ideas. > > Wow, had totally missed that CL. Would you like to land it instead, or have me > work on this one? ... maybe now that we also have images, we could go with this one?
hehe, I see our comments are leap-frogging each other.
On 2015/12/03 19:29:13, reed1 wrote: > On 2015/12/03 19:27:23, reed1 wrote: > > On 2015/12/03 19:21:30, Stephen White wrote: > > > See also https://codereview.chromium.org/1401173006/, if it gives you any > > ideas. > > > > Wow, had totally missed that CL. Would you like to land it instead, or have me > > work on this one? > > ... maybe now that we also have images, we could go with this one? Sure.. but maybe we could switch this one to use an Offset instead, to accentuate the delta? (What is the delta currently, BTW?)
https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp File bench/ImageBench.cpp (right): https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcode84 bench/ImageBench.cpp:84: class ImageFilterSpriteBench : public Benchmark { Why do we keep the SkSurface around - can't it be transient ? https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcode87 bench/ImageBench.cpp:87: SkBitmap fBitmap; fFilter doesn't seem to be really used ? https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcode95 bench/ImageBench.cpp:95: kSigma ? https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcod... bench/ImageBench.cpp:147: kSigma ?
I agree that using a cheaper filter is a good idea. Even w/ blur, mine shows a 25% delta between the drawBitmap and drawSprite cases, so easy to measure even w/ blur.
https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp File bench/ImageBench.cpp (right): https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcode84 bench/ImageBench.cpp:84: class ImageFilterSpriteBench : public Benchmark { On 2015/12/03 19:30:56, robertphillips wrote: > Why do we keep the SkSurface around - can't it be transient ? Done. https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcode87 bench/ImageBench.cpp:87: SkBitmap fBitmap; On 2015/12/03 19:30:56, robertphillips wrote: > fFilter doesn't seem to be really used ? Done. https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcode95 bench/ImageBench.cpp:95: On 2015/12/03 19:30:56, robertphillips wrote: > kSigma ? Done. https://codereview.chromium.org/1497843003/diff/1/bench/ImageBench.cpp#newcod... bench/ImageBench.cpp:147: On 2015/12/03 19:30:56, robertphillips wrote: > kSigma ? Done.
The CQ bit was checked by reed@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1497843003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497843003/40001
The CQ bit was unchecked by reed@google.com
senorblanco@chromium.org changed reviewers: + senorblanco@chromium.org
LGTM w/nit https://codereview.chromium.org/1497843003/diff/40001/bench/ImageBench.cpp File bench/ImageBench.cpp (right): https://codereview.chromium.org/1497843003/diff/40001/bench/ImageBench.cpp#ne... bench/ImageBench.cpp:152: // since the point of this bench is to time the actual blurring/drawing process. nit: s/blurring/filtering/
One final thing: I'd bump up the size of the image a bit. I'm a little mistrustful of benches in the microsecond range, since they can be sensitive to CPU cache sizes, etc.
On 2015/12/03 19:47:31, Stephen White wrote: > One final thing: I'd bump up the size of the image a bit. I'm a little > mistrustful of benches in the microsecond range, since they can be sensitive to > CPU cache sizes, etc. Done
Description was changed from ========== add bench for sprite-like drawing w/ imagefilters BUG=skia: ========== to ========== add bench for sprite-like drawing w/ imagefilters visualbench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 96/96 MB 16 412µs 413µs 413µs 414µs 0% ▄▁█▅▅▅▅▆▅▃▅ gpu warmupbench 98/98 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▁▂▇█▆▅▇▄▃▃▃ gpu image-filter-sprite-draw-image 100/100 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▆▇▅▄▁▆▅█▅▄▅ gpu image-filter-sprite-draw-bitmap 100/100 MB 32 547µs 548µs 590µs 1.01ms 24% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-sprite BUG=skia: ==========
https://codereview.chromium.org/1497843003/diff/40001/bench/ImageBench.cpp File bench/ImageBench.cpp (right): https://codereview.chromium.org/1497843003/diff/40001/bench/ImageBench.cpp#ne... bench/ImageBench.cpp:152: // since the point of this bench is to time the actual blurring/drawing process. On 2015/12/03 19:41:40, Stephen White wrote: > nit: s/blurring/filtering/ Done.
The CQ bit was checked by reed@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/1497843003/#ps40002 (title: "increase image size to make timing less sensitive to cache sizes etc.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1497843003/40002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1497843003/40002
Message was sent while issue was closed.
Description was changed from ========== add bench for sprite-like drawing w/ imagefilters visualbench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 96/96 MB 16 412µs 413µs 413µs 414µs 0% ▄▁█▅▅▅▅▆▅▃▅ gpu warmupbench 98/98 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▁▂▇█▆▅▇▄▃▃▃ gpu image-filter-sprite-draw-image 100/100 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▆▇▅▄▁▆▅█▅▄▅ gpu image-filter-sprite-draw-bitmap 100/100 MB 32 547µs 548µs 590µs 1.01ms 24% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-sprite BUG=skia: ========== to ========== add bench for sprite-like drawing w/ imagefilters visualbench run on Mac Pro curr/maxrss loops min median mean max stddev samples config bench 96/96 MB 16 412µs 413µs 413µs 414µs 0% ▄▁█▅▅▅▅▆▅▃▅ gpu warmupbench 98/98 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▁▂▇█▆▅▇▄▃▃▃ gpu image-filter-sprite-draw-image 100/100 MB 4 1.86ms 1.86ms 1.86ms 1.86ms 0% ▆▇▅▄▁▆▅█▅▄▅ gpu image-filter-sprite-draw-bitmap 100/100 MB 32 547µs 548µs 590µs 1.01ms 24% █▁▁▁▁▁▁▁▁▁▁ gpu image-filter-sprite-draw-sprite BUG=skia: Committed: https://skia.googlesource.com/skia/+/6f2753deee7b8cd03224e3df91dfea21bbeb9f7f ==========
Message was sent while issue was closed.
Committed patchset #5 (id:40002) as https://skia.googlesource.com/skia/+/6f2753deee7b8cd03224e3df91dfea21bbeb9f7f |
