Issue metadata
Sign in to add a comment
|
7% regression in memory.blink_memory_mobile at 413833:413862 |
||||||||||||||||||||
Issue descriptionSee the link to graphs below.
,
Aug 24 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9003403817673969040
,
Aug 24 2016
Started bisect job https://chromeperf.appspot.com/buildbucket_job_status/9003403807361389040
,
Aug 25 2016
=== Auto-CCing suspected CL author danakj@chromium.org === Hi danakj@chromium.org, the bisect results pointed to your CL below as possibly causing a regression. Please have a look at this info and see whether your CL be related. ===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Expose if we are using swiftshader via WebGraphicsContext3DProvider. Author : danakj Commit description: When we are using swiftshader, we are able to make contexts but the compositor will not use GPU compositing, and WebGL/Canvas should not be producing GL outputs for it. This is visible via the GPUInfo's software_rendering flag. Currently WebGL and Canvas get this info via two different paths, and the compositor makes its decision via a third. They all end up being the same thing (mostly not racey) but we can make this more common by just having them all check |software_rendering|. This has the additional benefits that: 1) We don't need to do a round trip thru the compositor thread to determine if we're doing GPU compositing (no more RendererCapability needed). 2) We don't need a separate method on the Platform API. 3) Canvas2D doesn't have a semi-racey thing where it can check |software_rendering| on one GpuChannelHost, lose it, then make a context on a different GpuChannelHost. Now the capability is explicitly tied to the lifetime of the context, which is what the comment in Platform tried to warn about. 4) Additionally, have DrawingBuffer check lost context and not try to produce frames in this scenario since it may end up producing the wrong mode for the compositor which is already using a new context with a different mode. (Canvas already does this.) This helps us eliminate races between cc/webgl/canvas on making this decision. When the GpuChannelHost is lost, all contexts become lost when the next code tries to create a context. Since we now tie the decisions around which mode to use to the context, and the context is lost when any other part of the system would change its opinion about what mode to use, they should all agree. R=junov@chromium.org, kbr@chromium.org, piman@chromium.org BUG= 606056 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2267993002 Cr-Commit-Position: refs/heads/master@{#413837} Commit : 4dd43951b9efedb9473e65e8132ee5d8976bc860 Date : Tue Aug 23 21:20:36 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@413832 6002969 0.0 5 good chromium@413836 6002969 0.0 5 good chromium@413837 6422387 0.0 5 bad <-- chromium@413838 6422387 0.0 5 bad chromium@413840 6422387 0.0 5 bad chromium@413847 6422387 0.0 5 bad chromium@413862 6422387 0.0 5 bad Bisect job ran on: android_nexus5X_perf_bisect Bug ID: 640811 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests memory.blink_memory_mobile Test Metric: memory:chrome:renderer_processes:reported_by_chrome:gpu:effective_size_avg/memory:chrome:renderer_processes:reported_by_chrome:gpu:effective_size_avg Relative Change: 6.99% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5X_perf_bisect/builds/598 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9003403807361389040 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5856722225201152 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Aug 25 2016
===== BISECT JOB RESULTS ===== Status: completed ===== SUSPECTED CL(s) ===== Subject : Expose if we are using swiftshader via WebGraphicsContext3DProvider. Author : danakj Commit description: When we are using swiftshader, we are able to make contexts but the compositor will not use GPU compositing, and WebGL/Canvas should not be producing GL outputs for it. This is visible via the GPUInfo's software_rendering flag. Currently WebGL and Canvas get this info via two different paths, and the compositor makes its decision via a third. They all end up being the same thing (mostly not racey) but we can make this more common by just having them all check |software_rendering|. This has the additional benefits that: 1) We don't need to do a round trip thru the compositor thread to determine if we're doing GPU compositing (no more RendererCapability needed). 2) We don't need a separate method on the Platform API. 3) Canvas2D doesn't have a semi-racey thing where it can check |software_rendering| on one GpuChannelHost, lose it, then make a context on a different GpuChannelHost. Now the capability is explicitly tied to the lifetime of the context, which is what the comment in Platform tried to warn about. 4) Additionally, have DrawingBuffer check lost context and not try to produce frames in this scenario since it may end up producing the wrong mode for the compositor which is already using a new context with a different mode. (Canvas already does this.) This helps us eliminate races between cc/webgl/canvas on making this decision. When the GpuChannelHost is lost, all contexts become lost when the next code tries to create a context. Since we now tie the decisions around which mode to use to the context, and the context is lost when any other part of the system would change its opinion about what mode to use, they should all agree. R=junov@chromium.org, kbr@chromium.org, piman@chromium.org BUG= 606056 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2267993002 Cr-Commit-Position: refs/heads/master@{#413837} Commit : 4dd43951b9efedb9473e65e8132ee5d8976bc860 Date : Tue Aug 23 21:20:36 2016 ===== TESTED REVISIONS ===== Revision Mean Std Dev N Good? chromium@413832 6002969 0.0 5 good chromium@413836 6002969 0.0 5 good chromium@413837 6422387 0.0 5 bad <-- chromium@413838 6422387 0.0 5 bad chromium@413840 6422387 0.0 5 bad chromium@413847 6422387 0.0 5 bad chromium@413862 6422387 0.0 5 bad Bisect job ran on: android_nexus5X_perf_bisect Bug ID: 640811 Test Command: src/tools/perf/run_benchmark -v --browser=android-chromium --output-format=chartjson --upload-results --also-run-disabled-tests memory.blink_memory_mobile Test Metric: memory:chrome:renderer_processes:reported_by_chrome:gpu:effective_size_avg/memory:chrome:renderer_processes:reported_by_chrome:gpu:effective_size_avg Relative Change: 6.99% Score: 99.9 Buildbot stdio: http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus5X_perf_bisect/builds/597 Job details: https://chromeperf.appspot.com/buildbucket_job_status/9003403817673969040 Not what you expected? We'll investigate and get back to you! https://chromeperf.appspot.com/bad_bisect?try_job_id=5836012429770752 | O O | Visit http://www.chromium.org/developers/speed-infra/perf-bug-faq | X | for more information addressing perf regression bugs. For feedback, | / \ | file a bug with component Tests>AutoBisect. Thank you!
,
Aug 25 2016
,
Aug 25 2016
:(
,
Aug 25 2016
Did the exposure of this flag change the code paths which are taken in some unexpected way?
,
Aug 25 2016
My guess is that software_rendering is true in these tests (aka swiftshader.. on android..?) and we're making a context then deciding not to use it whereas before we just established the channel and didnt make the context. But I'll have to confirm if thats actually whats going on.
,
Aug 25 2016
I thought Chromium currently only used SwiftShader on Windows. I don't even know that the SwiftShader library is accessible to Chromium on Android right now. How could it have been using it? Note that we did update the SwiftShader component this week (not sure it affect these tests) with a more recent version of SwiftShader, but even then, this should only affect Windows and Linux, not Android, AFAIK.
,
Aug 25 2016
Hm ok so I ran the test suite and its never the case that we check and see softwareRendering.
,
Aug 25 2016
Oh ok so, I think it's that we're making a shared context for canvas to decide to accelerate, even if we decide to use display list canvas etc. We should only create it when we are not going to not use it (ignoring swiftshader cases I think?)
,
Aug 25 2016
https://codereview.chromium.org/2280723003/ brings the memory usage back down
,
Aug 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03fe53dc4730038b5d13613fcbad10fc9a204c54 commit 03fe53dc4730038b5d13613fcbad10fc9a204c54 Author: danakj <danakj@chromium.org> Date: Fri Aug 26 21:18:02 2016 Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. R=junov@chromium.org, kbr@chromium.org BUG= 640811 , 606056 Review-Url: https://codereview.chromium.org/2280723003 Cr-Commit-Position: refs/heads/master@{#414806} [modify] https://crrev.com/03fe53dc4730038b5d13613fcbad10fc9a204c54/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp [modify] https://crrev.com/03fe53dc4730038b5d13613fcbad10fc9a204c54/third_party/WebKit/Source/core/html/HTMLCanvasElement.h
,
Aug 27 2016
Graph recovered.
,
Sep 1 2016
,
Sep 1 2016
Issue 640527 has been merged into this issue.
,
Sep 1 2016
Your change meets the bar and is auto-approved for M54 (branch: 2840)
,
Sep 1 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29e2540ac9bf025bb174d55543b7596e550920cd commit 29e2540ac9bf025bb174d55543b7596e550920cd Author: danakj <danakj@chromium.org> Date: Thu Sep 01 21:13:13 2016 Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. TBR=junov@chromium.org, kbr@chromium.org BUG= 640811 , 606056 Review-Url: https://codereview.chromium.org/2280723003 Cr-Commit-Position: refs/heads/master@{#414806} (cherry picked from commit 03fe53dc4730038b5d13613fcbad10fc9a204c54) Review URL: https://codereview.chromium.org/2303023002 . Cr-Commit-Position: refs/branch-heads/2840@{#106} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/29e2540ac9bf025bb174d55543b7596e550920cd/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp [modify] https://crrev.com/29e2540ac9bf025bb174d55543b7596e550920cd/third_party/WebKit/Source/core/html/HTMLCanvasElement.h
,
Oct 27 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29e2540ac9bf025bb174d55543b7596e550920cd commit 29e2540ac9bf025bb174d55543b7596e550920cd Author: danakj <danakj@chromium.org> Date: Thu Sep 01 21:13:13 2016 Avoid making the shared main thread context if we won't use it. When deciding if HTMLCanvasElement should be accelerated we changed the code to get the shared main thread context before making the decision because it's part of the decision. However, for most of the things we decide on (such as are-we-using-display-list-canvas) we don't care about the context. We only care once we decide we do want to use it, then verify it's not using swiftshader. So, defer getting/creating the shared main thread context until we know we want to accelerate, then just fail to accelerate if swiftshader. To make this logic more clear, I split the createImageBufferSurface() method into createAcceleratedImageBufferSurface() and createSoftwareImageBufferSurface() which is used if the former fails/returns null. There is less nesting now so hopefully that's nice. With this the memory.blink_memory_mobile's TheVerge case goes from 6422Kb of gpu memory back down to 4324Kb. TBR=junov@chromium.org, kbr@chromium.org BUG= 640811 , 606056 Review-Url: https://codereview.chromium.org/2280723003 Cr-Commit-Position: refs/heads/master@{#414806} (cherry picked from commit 03fe53dc4730038b5d13613fcbad10fc9a204c54) Review URL: https://codereview.chromium.org/2303023002 . Cr-Commit-Position: refs/branch-heads/2840@{#106} Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607} [modify] https://crrev.com/29e2540ac9bf025bb174d55543b7596e550920cd/third_party/WebKit/Source/core/html/HTMLCanvasElement.cpp [modify] https://crrev.com/29e2540ac9bf025bb174d55543b7596e550920cd/third_party/WebKit/Source/core/html/HTMLCanvasElement.h |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by bashi@chromium.org
, Aug 24 2016