Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1675833002:
Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect (Closed)
|
Created:
4 years, 10 months ago by Brian Osman Modified:
4 years, 10 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionSwitch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect
[Notes:]
- Performance delta on desktop looked like noise. I'm curious what the results will be on low-end mobile GPUs. I'm still trying to figure out how I determine that with perf, etc...
- There appeared to be a single one-pixel image diff while running dm, if I'm understanding the results of skdiff correctly, so that's probably good news?
- Should I include anyone else on the review?
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1675833002
Committed: https://skia.googlesource.com/skia/+/bb2ff94e223014403f775c3532a25cc25b00c949
Patch Set 1 #Patch Set 2 : Remove unused variable to fix compile errors. #Messages
Total messages: 18 (9 generated)
Description was changed from ========== Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect BUG=skia: ========== to ========== Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect [Notes:] - Performance delta on desktop looked like noise. I'm curious what the results will be on low-end mobile GPUs. I'm still trying to figure out how I determine that with perf, etc... - There appeared to be a single one-pixel image diff while running dm, if I'm understanding the results of skdiff correctly, so that's probably good news? - Should I include anyone else on the review? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
brianosman@google.com changed reviewers: + bsalomon@google.com, joshualitt@chromium.org
This is the 'write the code' half of this task (and some correctness testing). Still need to figure out if this is a non-negative performance change on all platforms?
On 2016/02/05 22:23:10, Brian Osman wrote: > This is the 'write the code' half of this task (and some correctness testing). > Still need to figure out if this is a non-negative performance change on all > platforms? LGTM, as for testing you can always just land the change and then monitor on perf. We can discuss on Monday. I suspect it would only show up in microbenchmarks
lgtm, let's see if we can get Chris to land his shapes benchmark.
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by brianosman@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675833002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675833002/1
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...) Build-Ubuntu-Clang-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
The CQ bit was checked by brianosman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from joshualitt@google.com, bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1675833002/#ps40001 (title: "Remove unused variable to fix compile errors.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1675833002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1675833002/40001
Message was sent while issue was closed.
Description was changed from ========== Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect [Notes:] - Performance delta on desktop looked like noise. I'm curious what the results will be on low-end mobile GPUs. I'm still trying to figure out how I determine that with perf, etc... - There appeared to be a single one-pixel image diff while running dm, if I'm understanding the results of skdiff correctly, so that's probably good news? - Should I include anyone else on the review? BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Switch to vertex colors rather than uniforms for color in Ellipse/Circle/RRect/DRRect [Notes:] - Performance delta on desktop looked like noise. I'm curious what the results will be on low-end mobile GPUs. I'm still trying to figure out how I determine that with perf, etc... - There appeared to be a single one-pixel image diff while running dm, if I'm understanding the results of skdiff correctly, so that's probably good news? - Should I include anyone else on the review? 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/+/bb2ff94e223014403f775c3532a25cc25b00c949 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/bb2ff94e223014403f775c3532a25cc25b00c949
Message was sent while issue was closed.
On 2016/02/11 at 22:15:21, commit-bot wrote: > Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/bb2ff94e223014403f775c3532a25cc25b00c949 This looks like it caused a significant regression on the TegraX1 and a decent regression on other Android platforms: https://perf.skia.org/#4886 https://perf.skia.org/#4888 https://screenshot.googleplex.com/RfTdqEjJEGA https://screenshot.googleplex.com/aRaKoiVAnFV I'm not sure why Perf didn't alert on this, which I will check separately.
Message was sent while issue was closed.
On 2016/02/17 at 19:54:07, jcgregorio wrote: > On 2016/02/11 at 22:15:21, commit-bot wrote: > > Committed patchset #2 (id:40001) as https://skia.googlesource.com/skia/+/bb2ff94e223014403f775c3532a25cc25b00c949 > > This looks like it caused a significant regression on the TegraX1 and a decent regression on other Android platforms: > > https://perf.skia.org/#4886 > https://perf.skia.org/#4888 > > https://screenshot.googleplex.com/RfTdqEjJEGA > https://screenshot.googleplex.com/aRaKoiVAnFV > > I'm not sure why Perf didn't alert on this, which I will check separately. As Joshua pointed out to me, we only do clustering on SKPs, not on bench results. When looking at SKPs there aren't any obvious regressions, so feel free to ignore the above. |
