[BlinkGenPropertyTrees] Perf analysis |
|||||
Issue descriptionWe should do some preliminary perf analysis of BlinkGenPropertyTrees (--enable-blink-gen-property-trees).
,
Jul 18
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/16bb843ea40000
,
Jul 19
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/1016e485a40000
,
Jul 19
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/1416e485a40000
,
Jul 19
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/12ed3dfea40000
,
Jul 19
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16cc8235a40000
,
Jul 19
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.
,
Jul 19
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/1416e485a40000
,
Jul 20
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/16cc8235a40000
,
Jul 20
😿 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.
,
Jul 20
😿 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.
,
Jul 20
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
,
Jul 23
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/16be8d5da40000
,
Jul 23
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/16be8d5da40000
,
Jul 23
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/169c77f1a40000
,
Jul 23
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/169c77f1a40000
,
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
,
Aug 6
You'll need to give me info on how to deal with UKM and data analysis for this metric.
,
Aug 6
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.
,
Aug 14
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/128146d2640000
,
Aug 14
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/15fb7050640000
,
Aug 14
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/128146d2640000
,
Aug 14
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/15fb7050640000
,
Aug 14
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/13d81a88640000
,
Aug 14
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/15f82504640000
,
Aug 14
📍 Pinpoint job started. https://pinpoint-dot-chromeperf.appspot.com/job/13a003c4640000
,
Aug 14
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/13d81a88640000
,
Aug 15
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/13a003c4640000
,
Aug 15
📍 Job complete. See results below. https://pinpoint-dot-chromeperf.appspot.com/job/15f82504640000
,
Aug 15
,
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
,
Oct 25
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.
,
Dec 6
For now this is fixed. We're going to use Finch to track this rollout. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by 42576172...@developer.gserviceaccount.com
, Jul 18