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

Issue 696596 link

Starred by 1 user
Status: Fixed
Owner:
kylec...@chromium.org
Closed: Mar 2017
Cc:
danakj@chromium.org
samans@chromium.org
█ fsam...@chromium.org
█ jbau...@chromium.org
mustash-bugs@google.com
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature



Sign in to add a comment

Add UMA stat when SurfaceAggregator can't find a surface

Project Member Reported by kylec...@chromium.org, Feb 27 2017 
In SurfaceAggregator::HandleSurfaceQuad() it would be useful to have an UMA stat for how often SurfaceAggregator tries to get a cc::Surface from SurfaceManager that doesn't exist. Proper surface lifetime management should ensure this is zero, so the stat would provide a good indication of any regressions in surface lifetime management.

This shouldn't include being able to find a surface but the surface not having a CompositorFrame due to frame eviction.

This is also complicated a bit by surface synchronization. A CompositorFrame can include a yet to be created surface. I think there are two cases here:

(1) If the primary surface doesn't exist and there is no fallback quad then it's an error.
(2) If the primary surface doesn't exist and there is a fallback quad but the fallback surface doesn't exist then it's an error.

Comment 1 by kylec...@chromium.org, Mar 14 2017 

This could be implemented by adding three(?) UMA stats and logging them for each SurfaceAggregator::Aggregate() call.

1. The number of SurfaceDrawQuads where the Surface was found and it had an active CompositorFrame.
2. The number of SurfaceDrawQuads where the Surface wasn't found. There is no fallback Surface.
3. The number of SurfaceDrawQuads where the Surface was found but there was no active CompositorFrame. There is no fallback Surface.

Case (2) would hopefully never happen. This indicates there is a bug in surface lifetime management. This could be used to see if any behaviour is changed with surface sequences vs surface references.

Case (3) should only happen if the surface is evicted I think.

Case (1) would be normal operation but it would give some context for (2) and (3). When something like --top-document-isolation is turned on there will be more SurfaceDrawQuads so the other errors would occur more often.
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 21 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0db8650476b17b19f443de6b58f20465ac6604f4

commit 0db8650476b17b19f443de6b58f20465ac6604f4
Author: kylechar <kylechar@chromium.org>
Date: Tue Mar 21 18:26:29 2017

Add UMA stats for SurfaceDrawQuads in SurfaceAggregator.

This CL adds UMA stats to keep track of SurfaceDrawQuads during surface
aggregation. For each call to SurfaceAggregator::Aggregate(), keep
track of surface referenced from SurfaceDrawQuads.

1. ValidSurface: The surface exists and has an active CompositorFrame.
2. MissingSurface: The surface doesn't exist.
3. NoActiveFrame: The surface exists but doesn't have an active frame.

Ideally 2 and 3 should never happen but the UMA stats will provide a
signal to if something has changed with regards to surface lifetime
management.

BUG= 696596 
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2752053003
Cr-Commit-Position: refs/heads/master@{#458480}

[modify] https://crrev.com/0db8650476b17b19f443de6b58f20465ac6604f4/cc/surfaces/surface_aggregator.cc
[modify] https://crrev.com/0db8650476b17b19f443de6b58f20465ac6604f4/cc/surfaces/surface_aggregator.h
[modify] https://crrev.com/0db8650476b17b19f443de6b58f20465ac6604f4/tools/metrics/histograms/histograms.xml

Comment 3 by kylec...@chromium.org, Mar 27 2017

Status: Fixed (was: Assigned)

Comment 4 by kylec...@chromium.org, Mar 27 2017

Status: Started (was: Fixed)
Project Member

Comment 5 by bugdroid1@chromium.org, Mar 28 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/032f46acd2bf679f9253370237dfabdbc139c3fb

commit 032f46acd2bf679f9253370237dfabdbc139c3fb
Author: kylechar <kylechar@chromium.org>
Date: Tue Mar 28 15:26:38 2017

Fix typo in histogram name.

Change "SurfaceMissing" to "MissingSurface".

R=jwd@chromium.org
BUG= 696596 

Review-Url: https://codereview.chromium.org/2773393003
Cr-Commit-Position: refs/heads/master@{#460110}

[modify] https://crrev.com/032f46acd2bf679f9253370237dfabdbc139c3fb/tools/metrics/histograms/histograms.xml

Comment 6 by kylec...@chromium.org, Mar 29 2017

Status: Fixed (was: Started)

Comment 7 by fsam...@chromium.org, Jun 13 2017

Blocking:
►

Sign in to add a comment