New issue
Advanced search Search tips

Issue 865168 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 874453

Blocking:
issue 836886



Sign in to add a comment

[BlinkGenPropertyTrees] Perf analysis

Project Member Reported by pdr@chromium.org, Jul 18

Issue description

We should do some preliminary perf analysis of BlinkGenPropertyTrees (--enable-blink-gen-property-trees).
 
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16bb843ea40000
Labels: -Pri-3 Pri-1
Some of our metrics are incorrect with BlinkGenPropertyTrees. We need to rationalize this. The goal is to have a metric that goes down when the cc property tree builder is turned off, and a second metric that goes up when the blink paint artifact compositor code starts being used.

The cc property tree builder is called from LayerTreeHost::UpdateLayers and tracked with the uma metric Compositing.Renderer.LayersUpdateTime (SPV175).

The paint artifact compositor update is called from LocalFrameView::PushPaintArtifactToCompositor with the uma metric Blink.Compositing.UpdateTime (SPV2, BGPT).

There is a second uma metric with Blink.Compositing.UpdateTime that is called from LocalFrameView::UpdateLifecyclePhasesInternal and covers the code in PaintLayerCompositor::UpdateIfNeededRecursive (SPV175, BGPT).

One approach to fixing this is to ensure Blink.Compositing.UpdateTime is only emitted once with BGPT by collecting the time spent in PaintLayerCompositor::UpdateIfNeededRecursive and adding it to the Blink.Compositing.UpdateTime metric called from LocalFrameView::PushPaintArtifactToCompositor. Another approach is to rename the uma metrics so they can disambiguated manually.
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/1416e485a40000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16cc8235a40000
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/12ed3dfea40000

All of the runs failed. The most common error (17/20 runs) was:
TimeoutException: Timed out while waiting 60s for IsJavaScriptExpressionTrue.
😿 Pinpoint job stopped with an error.
https://pinpoint-dot-chromeperf.appspot.com/job/1016e485a40000

All of the runs failed. The most common error (8/20 runs) was:
TimeoutException: Timed out while waiting 90s for IsJavaScriptExpressionTrue.
Update on comment #7, it's not trivial to just add times together (i.e., PaintLayerCompositor::UpdateIfNeededRecursive + LocalFrameView::PushPaintArtifactToCompositor = Blink.Compositing.UpdateTime) for BGPT because some updates only do PaintLayerCompositor::UpdateIfNeededRecursive and it's useful to consider the number of times these functions are called.

Therefore, lets add a new metric. Patch: https://chromium-review.googlesource.com/c/chromium/src/+/1145478
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16be8d5da40000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/169c77f1a40000
Project Member

Comment 17 by bugdroid1@chromium.org, Aug 4

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

commit 10645d7b029470b8cf5744e86a4b8506a969e214
Author: Philip Rogers <pdr@chromium.org>
Date: Sat Aug 04 03:00:36 2018

Add new UMA for compositing: Blink.CompositingCommit.UpdateTime

PaintArtifactCompositor is a new compositing algorithm (slimming paint
v2, SPV2) and we also have a legacy algorithm. Blink gen property trees
is an incremental step towards lunching the new algorithm, and uses
parts of both the new and legacy systems. To disambiguate our metrics,
the PaintArtifactCompositor metrics are being renamed to
Blink.CompositingCommit.UpdateTime.

SPV175 (current):
Only Blink.Compositing.UpdateTime

BlinkGenPropertyTrees:
Blink.CompositingCommit.UpdateTime and Blink.Compositing.UpdateTime

SPV2:
Only Blink.CompositingCommit.UpdateTime

Bug:  865168 

Change-Id: Ia676208a8b10cf14f5add11c09f2d7884b5d71e1
Reviewed-on: https://chromium-review.googlesource.com/1145478
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#580735}
[modify] https://crrev.com/10645d7b029470b8cf5744e86a4b8506a969e214/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/10645d7b029470b8cf5744e86a4b8506a969e214/tools/metrics/histograms/histograms.xml
[modify] https://crrev.com/10645d7b029470b8cf5744e86a4b8506a969e214/tools/metrics/ukm/ukm.xml

You'll need to give me info on how to deal with UKM and data analysis for this metric.
It'll be a little confusing but the tl;dr is:
SPV175 (current): Only Blink.Compositing.UpdateTime
BlinkGenPropertyTrees: Blink.CompositingCommit.UpdateTime and Blink.Compositing.UpdateTime
SPV2: Only Blink.CompositingCommit.UpdateTime

We should see Blink.Compositing.UpdateTime drop with BlinkGenPropertyTrees, and Blink.CompositingCommit.UpdateTime will rise. These are not guaranteed to change by the same amount so we will need to be on the lookout for regressions. We'll be launching via finch and will know more once that's enabled.
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/128146d2640000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/15fb7050640000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/13d81a88640000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/13a003c4640000
📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/15f82504640000
Blockedon: 874453
Project Member

Comment 31 by bugdroid1@chromium.org, Aug 20

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

commit 5ae8f4987e9dadad988e91472f6c0e53acfda67a
Author: Philip Rogers <pdr@chromium.org>
Date: Mon Aug 20 19:37:50 2018

Fix Blink.CompositingCommit.UpdateTime metric

https://crrev.com/580735 updated the wrong Blink.Compositing.UpdateTime
metric due to a rebase mistake. This patch fixes the mistake so that the
following is true:

SPV175 (current):
Only Blink.Compositing.UpdateTime

BlinkGenPropertyTrees:
Blink.CompositingCommit.UpdateTime and Blink.Compositing.UpdateTime

SPV2:
Only Blink.CompositingCommit.UpdateTime

Bug:  865168 
Change-Id: I50b3bc45454755ce82ba72da05c42000eca971b7
Reviewed-on: https://chromium-review.googlesource.com/1181647
Reviewed-by: Stephen Chenney <schenney@chromium.org>
Commit-Queue: Philip Rogers <pdr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584537}
[modify] https://crrev.com/5ae8f4987e9dadad988e91472f6c0e53acfda67a/third_party/blink/renderer/core/frame/local_frame_view.cc

Cc: -trchen@chromium.org chrishtr@chromium.org wangxianzhu@chromium.org
BlinkGenPropertyTrees has been promoted to experimental in https://crrev.com/602150. Here are the perf results so far:
https://chromeperf.appspot.com/group_report?rev=602150

This is surprising because, at the time of writing, there are no perf regressions and only one possible perf progression. BlinkGenPropertyTrees should have some effect, if only because it shifts time around. Here are the differences I can think of that would cause perf differences:
1) LocalFrameView::RunPaintLifecyclePhase should get more expensive because we do a collection step to collect all layers as foreign layers, and we now run PaintArtifactCompositor which builds cc property trees from blink property trees.
2) LayerTreeHost::DoUpdateLayers should get cheaper because we are not building property trees.
3) Some additional cc property tree nodes should be used. Blink's property tree nodes are simpler (e.g., blink::TransformPaintPropertyTreeNode has 1 matrix, whereas cc::TransformNode has 4 matrices), which leads to additional cc::TransformNodes being created.

I verified the perf tests run with experimental web platform features enabled, and BlinkGenPropertyTrees is enabled. I also verified using tracing that BlinkGenPropertyTrees runs PAC and does not run the cc property tree builder.

I have kicked off two cluster telemetry runs to try to get more information about why there have not been more effects from this change.
Status: Fixed (was: Assigned)
For now this is fixed. We're going to use Finch to track this rollout.

Sign in to add a comment