|
|
Created:
4 years, 4 months ago by msarett Modified:
4 years, 4 months ago Reviewers:
bsalomon, djsollen, reed1 CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionBatched implementation of drawLattice() for GPU
Bechmarks (Nexus 6P):
Src=100x100, Dst=250x250, NumRects=9
Android 77.7us
Skia (without patch) 57.2us
Skia (with patch) 30.9us
Src=100x100, Dst=500x500, NumRects=9
Android 77.0us
Skia (without patch) 56.9us
Skia (with patch) 31.8us
Src=100x100, Dst=1000x1000, NumRects=9
Android 180us
Skia (without patch) 96.8us
Skia (with patch) 70.5us
Src=100x100, Dst=250x250, NumRects=15
Android 208us
Skia (without patch) 155us
Skia (with patch) 38.2us
Src=100x100, Dst=500x500, NumRects=15
Android 207us
Skia (without patch) 152us
Skia (with patch) 38.4us
Src=100x100, Dst=1000x1000, NumRects=15
Android 233us
Skia (without patch) 156us
Skia (with patch) 99.9us
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002
Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d
Committed: https://skia.googlesource.com/skia/+/10e3d9bf59bdec92c05367ae0b71e1ce1ee4a690
Patch Set 1 #Patch Set 2 : Think this should work #Patch Set 3 : Clean up #Patch Set 4 : Improve bench #
Total comments: 7
Patch Set 5 : Response to comments #Patch Set 6 : Improve bench, move matrix map outside loop #Patch Set 7 : Rebase #Patch Set 8 : Speculative reland #
Created: 4 years, 4 months ago
Depends on Patchset: Messages
Total messages: 41 (31 generated)
Description was changed from ========== WIP: Batched implementation of drawLattice() for GPU BUG=skia: ========== to ========== WIP: Batched implementation of drawLattice() for GPU BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
Description was changed from ========== WIP: Batched implementation of drawLattice() for GPU BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== WIP: Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Android 212us Skia (without patch) 122us Skia (with patch) 110us HP z620 Bench Times: Android 25.2us Skia (without patch) 28.0us Skia (with patch) 33.1us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== WIP: Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Android 212us Skia (without patch) 122us Skia (with patch) 110us HP z620 Bench Times: Android 25.2us Skia (without patch) 28.0us Skia (with patch) 33.1us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== WIP: Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Small Large Android 212us 12.9ms Skia (without patch) 175us 9.44ms Skia (with patch) 119us 4.47ms HP z620 Bench Times: Small Large Android 27.6us 60.0us Skia (without patch) 22.8us 53.2us Skia (with patch) 38.5us 40.0us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
Patchset #4 (id:80001) has been deleted
Description was changed from ========== WIP: Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Small Large Android 212us 12.9ms Skia (without patch) 175us 9.44ms Skia (with patch) 119us 4.47ms HP z620 Bench Times: Small Large Android 27.6us 60.0us Skia (without patch) 22.8us 53.2us Skia (with patch) 38.5us 40.0us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Small Large Android 212us 12.9ms Skia (without patch) 175us 9.44ms Skia (with patch) 119us 4.47ms HP z620 Bench Times: Small Large Android 27.6us 60.0us Skia (without patch) 22.8us 53.2us Skia (with patch) 38.5us 40.0us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
Description was changed from ========== Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Small Large Android 212us 12.9ms Skia (without patch) 175us 9.44ms Skia (with patch) 119us 4.47ms HP z620 Bench Times: Small Large Android 27.6us 60.0us Skia (without patch) 22.8us 53.2us Skia (with patch) 38.5us 40.0us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Small Large Android 212us 12.9ms Skia (without patch) 175us 9.44ms Skia (with patch) 119us 4.47ms HP z620 Bench Times: Small Large Android 27.6us 60.0us Skia (without patch) 22.8us 53.2us Skia (with patch) 38.5us 40.0us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
msarett@google.com changed reviewers: + bsalomon@google.com, djsollen@google.com, reed@google.com
Improves performance a bit for drawing Android's ninepatches. https://codereview.chromium.org/2255963002/diff/100001/bench/DrawLatticeBench... File bench/DrawLatticeBench.cpp (right): https://codereview.chromium.org/2255963002/diff/100001/bench/DrawLatticeBench... bench/DrawLatticeBench.cpp:45: void onDraw(int loops, SkCanvas* canvas) override { Brian, any thoughts on this bench? I've had some issues with inconsistent results. In particular, the Nexus 6P seems to really hate the DrawLattice_Large version. https://codereview.chromium.org/2255963002/diff/100001/src/gpu/SkGpuDevice.cpp File src/gpu/SkGpuDevice.cpp (right): https://codereview.chromium.org/2255963002/diff/100001/src/gpu/SkGpuDevice.cp... src/gpu/SkGpuDevice.cpp:1450: std::unique_ptr<SkLatticeIter> iter( Another option is to convert the |center| rect to a SkCanvas::Lattice here. This issue I had is that the SkCanvas::Lattice does not own its pointers to fXDivs and fYDivs. So here we're stuck with no one to take ownership...
https://codereview.chromium.org/2255963002/diff/100001/bench/DrawLatticeBench... File bench/DrawLatticeBench.cpp (right): https://codereview.chromium.org/2255963002/diff/100001/bench/DrawLatticeBench... bench/DrawLatticeBench.cpp:45: void onDraw(int loops, SkCanvas* canvas) override { On 2016/08/18 15:16:08, msarett wrote: > Brian, any thoughts on this bench? I've had some issues with inconsistent > results. In particular, the Nexus 6P seems to really hate the DrawLattice_Large > version. Not really. I wonder if it has more to do with the size of the destination than the size of the lattice. https://codereview.chromium.org/2255963002/diff/100001/src/gpu/batches/GrNine... File src/gpu/batches/GrNinePatch.cpp (right): https://codereview.chromium.org/2255963002/diff/100001/src/gpu/batches/GrNine... src/gpu/batches/GrNinePatch.cpp:110: patch.fViewMatrix.mapPointsWithStride(positions, vertexStride, kVertsPerRect); I wonder if we should lift this out of the loop, so we apply the view matrix to all the points in a lattice at once. https://codereview.chromium.org/2255963002/diff/100001/src/gpu/batches/GrNine... src/gpu/batches/GrNinePatch.cpp:151: fPatches.push_back(std::move(that->fPatches[i])); Should we have a bulk mover? move_back_n(T*, int) That way the array would only check for realloc once.
https://codereview.chromium.org/2255963002/diff/100001/src/gpu/batches/GrNine... File src/gpu/batches/GrNinePatch.cpp (right): https://codereview.chromium.org/2255963002/diff/100001/src/gpu/batches/GrNine... src/gpu/batches/GrNinePatch.cpp:110: patch.fViewMatrix.mapPointsWithStride(positions, vertexStride, kVertsPerRect); On 2016/08/18 15:40:26, bsalomon wrote: > I wonder if we should lift this out of the loop, so we apply the view matrix to > all the points in a lattice at once. Good idea - I think this is a nice opt for the scale-translate case. Otherwise, I think we still need to apply to dstR. https://codereview.chromium.org/2255963002/diff/100001/src/gpu/batches/GrNine... src/gpu/batches/GrNinePatch.cpp:151: fPatches.push_back(std::move(that->fPatches[i])); On 2016/08/18 15:40:26, bsalomon wrote: > Should we have a bulk mover? move_back_n(T*, int) That way the array would only > check for realloc once. Yes! I've added one.
Description was changed from ========== Batched implementation of drawLattice() for GPU Nexus 6P Bench Times: Small Large Android 212us 12.9ms Skia (without patch) 175us 9.44ms Skia (with patch) 119us 4.47ms HP z620 Bench Times: Small Large Android 27.6us 60.0us Skia (without patch) 22.8us 53.2us Skia (with patch) 38.5us 40.0us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
Description was changed from ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ==========
I've moved all of the matrix computation out of the loop now. And I've also improved the benchmark. I gave up on measuring on z620 since the results are so noisy that I can't make any conclusions. But the latest results are good on 6P! And they actually seem to make logical sense!
lgtm, numbers look nice!
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-GN-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on master.client.skia (JOB_FAILED, http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...) Build-Ubuntu-Clang-x86_64-Debug-GN-Trybot on master.client.skia.compile (JOB_FAILED, http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-Clang-x...)
Patchset #7 (id:160001) has been deleted
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2255963002/#ps180001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 ========== to ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:180001) has been created in https://codereview.chromium.org/2255683004/ by msarett@google.com. The reason for reverting is: Things drawing weird..
Message was sent while issue was closed.
Description was changed from ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d ========== to ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d ==========
The CQ bit was checked by msarett@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 34.7us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 44.5us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 55.9us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 63.0us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d ========== to ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 30.9us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 31.8us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 38.2us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 38.4us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2255963002/#ps200001 (title: "Speculative reland")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 30.9us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 31.8us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 38.2us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 38.4us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d ========== to ========== Batched implementation of drawLattice() for GPU Bechmarks (Nexus 6P): Src=100x100, Dst=250x250, NumRects=9 Android 77.7us Skia (without patch) 57.2us Skia (with patch) 30.9us Src=100x100, Dst=500x500, NumRects=9 Android 77.0us Skia (without patch) 56.9us Skia (with patch) 31.8us Src=100x100, Dst=1000x1000, NumRects=9 Android 180us Skia (without patch) 96.8us Skia (with patch) 70.5us Src=100x100, Dst=250x250, NumRects=15 Android 208us Skia (without patch) 155us Skia (with patch) 38.2us Src=100x100, Dst=500x500, NumRects=15 Android 207us Skia (without patch) 152us Skia (with patch) 38.4us Src=100x100, Dst=1000x1000, NumRects=15 Android 233us Skia (without patch) 156us Skia (with patch) 99.9us BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2255963002 Committed: https://skia.googlesource.com/skia/+/93242c4ae50dfcc0d922cdb3ba80bbc7b4bbe93d Committed: https://skia.googlesource.com/skia/+/10e3d9bf59bdec92c05367ae0b71e1ce1ee4a690 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://skia.googlesource.com/skia/+/10e3d9bf59bdec92c05367ae0b71e1ce1ee4a690 |