New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 640811 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug-Regression



Sign in to add a comment

7% regression in memory.blink_memory_mobile at 413833:413862

Project Member Reported by bashi@chromium.org, Aug 24 2016

Issue description

See the link to graphs below.
 

Comment 1 by bashi@chromium.org, Aug 24 2016

All graphs for this bug:
  https://chromeperf.appspot.com/group_report?bug_id=640811

Original alerts at time of bug-filing:
  https://chromeperf.appspot.com/group_report?keys=agxzfmNocm9tZXBlcmZyFAsSB0Fub21hbHkYgICgvuT6swoM


Bot(s) for this bug's original alert(s):

android-nexus5X
Project Member

Comment 4 by 42576172...@developer.gserviceaccount.com, Aug 25 2016

Cc: danakj@chromium.org
Owner: danakj@chromium.org

=== 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!
Project Member

Comment 5 by 42576172...@developer.gserviceaccount.com, 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!

Comment 6 by danakj@chromium.org, Aug 25 2016

Cc: kbr@chromium.org enne@chromium.org junov@chromium.org piman@chromium.org

Comment 7 by danakj@chromium.org, Aug 25 2016

:(

Comment 8 by kbr@chromium.org, Aug 25 2016

Cc: sugoi@chromium.org capn@chromium.org
Did the exposure of this flag change the code paths which are taken in some unexpected way?

Comment 9 by danakj@chromium.org, 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.

Comment 10 by sugoi@chromium.org, 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.
Hm ok so I ran the test suite and its never the case that we check and see softwareRendering.
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?)
https://codereview.chromium.org/2280723003/ brings the memory usage back down
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Graph recovered.
Labels: Merge-Request-54
Issue 640527 has been merged into this issue.

Comment 18 by dimu@chromium.org, Sep 1 2016

Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member

Comment 19 by bugdroid1@chromium.org, Sep 1 2016

Labels: -merge-approved-54 merge-merged-2840
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

Project Member

Comment 20 by bugdroid1@chromium.org, 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