20% regression in draw properties computation time |
||||||||||||
Issue descriptionThe Compositing.Renderer.LayerTreeImpl.CalculateDrawPropertiesUs UMA metric regressed by about 20% on Windows Canary around 56.0.2906.0. Looking at property-tree-related changes around that time, a possible suspect is https://codereview.chromium.org/2460413003/, which changed RenderSurfaceImpl::EffectTreeIndex() to using a hash map lookup. I'm going to try reverting that to see if the metric recovers.
,
Nov 8 2016
The metrics did not recover. https://codereview.chromium.org/2473453002 is the next suspect. So, I will take this.
,
Nov 9 2016
,
Nov 9 2016
Re #2, that patch also got merged to beta, so changing the milestone to M-55.
,
Nov 9 2016
Is this applicable to all OSs or any specific OS?
,
Nov 9 2016
,
Nov 9 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b90ee7fe1cb7843f41100413e907c3e86c620ad7 commit b90ee7fe1cb7843f41100413e907c3e86c620ad7 Author: jaydasika <jaydasika@chromium.org> Date: Wed Nov 09 19:13:16 2016 TransformTree::SetRootTransformsAndScales should set 'needs_update' on transform tree only if we are changing the transforms stored on the root or the contents_root node. Otherwise, we will end up updating transforms every frame. BUG= 663068 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2490773002 Cr-Commit-Position: refs/heads/master@{#431000} [modify] https://crrev.com/b90ee7fe1cb7843f41100413e907c3e86c620ad7/cc/trees/property_tree.cc [modify] https://crrev.com/b90ee7fe1cb7843f41100413e907c3e86c620ad7/cc/trees/property_tree_unittest.cc
,
Nov 10 2016
,
Nov 10 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 10 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2a4fc7ddcd6b0f9368d510f46882831d290fe9b7 commit 2a4fc7ddcd6b0f9368d510f46882831d290fe9b7 Author: Jayadev Dasika <jaydasika@google.com> Date: Thu Nov 10 23:55:03 2016 TransformTree::SetRootTransformsAndScales should set 'needs_update' on transform tree only if we are changing the transforms stored on the root or the contents_root node. Otherwise, we will end up updating transforms every frame. BUG= 663068 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2490773002 Cr-Commit-Position: refs/heads/master@{#431000} (cherry picked from commit b90ee7fe1cb7843f41100413e907c3e86c620ad7) Review URL: https://codereview.chromium.org/2493943003 . Cr-Commit-Position: refs/branch-heads/2883@{#528} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/2a4fc7ddcd6b0f9368d510f46882831d290fe9b7/cc/trees/property_tree.cc [modify] https://crrev.com/2a4fc7ddcd6b0f9368d510f46882831d290fe9b7/cc/trees/property_tree_unittest.cc
,
Nov 11 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9e4e7c21e6c6b311e5ccc02afa358a149b6c7a06 commit 9e4e7c21e6c6b311e5ccc02afa358a149b6c7a06 Author: jaydasika <jaydasika@chromium.org> Date: Fri Nov 11 19:42:34 2016 Revert of TransformTree::SetRootTransformsAndScales should set 'needs_update' on transform tree only if we ar… (patchset #1 id:1 of https://codereview.chromium.org/2493943003/ ) Reason for revert: https://bugs.chromium.org/p/chromium/issues/detail?id=664408 Original issue's description: > TransformTree::SetRootTransformsAndScales should set 'needs_update' on transform tree only if we are changing the transforms stored on the root or the contents_root node. Otherwise, we will end up updating transforms every frame. > > BUG= 663068 > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > Review-Url: https://codereview.chromium.org/2490773002 > Cr-Commit-Position: refs/heads/master@{#431000} > (cherry picked from commit b90ee7fe1cb7843f41100413e907c3e86c620ad7) > > Committed: https://chromium.googlesource.com/chromium/src/+/2a4fc7ddcd6b0f9368d510f46882831d290fe9b7 TBR= # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 663068 Review-Url: https://codereview.chromium.org/2500603002 Cr-Commit-Position: refs/branch-heads/2883@{#545} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/9e4e7c21e6c6b311e5ccc02afa358a149b6c7a06/cc/trees/property_tree.cc [modify] https://crrev.com/9e4e7c21e6c6b311e5ccc02afa358a149b6c7a06/cc/trees/property_tree_unittest.cc
,
Nov 11 2016
The merge got reverted. need to re-merge.
,
Nov 12 2016
Your change meets the bar and is auto-approved for M55 (branch: 2883)
,
Nov 14 2016
**** Bulk edit - please ignore if not applicable **** A friendly reminder that M55 Stable is launch is coming soon! Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and get it merged into the release branch ASAP so it gets enough baking time in Beta (before Stable promotion). Thank you! Also due to Thanksgiving holidays in US, please make sure fix is ready and merged to M55 latest by 5:00 PM PT Friday, 11/18/16 (sooner the better).
,
Nov 14 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/39c2ff384acb89d238d1f61549e718eea1257ed3 commit 39c2ff384acb89d238d1f61549e718eea1257ed3 Author: jaydasika <jaydasika@chromium.org> Date: Mon Nov 14 19:43:10 2016 TransformTree::SetRootTransformsAndScales should set 'needs_update' on transform tree only if we are changing the transforms stored on the root or the contents_root node. Otherwise, we will end up updating transforms every frame. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 663068 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Review-Url: https://codereview.chromium.org/2490773002 Cr-Commit-Position: refs/heads/master@{#431000} (cherry picked from commit b90ee7fe1cb7843f41100413e907c3e86c620ad7) Committed: https://chromium.googlesource.com/chromium/src/+/2a4fc7ddcd6b0f9368d510f46882831d290fe9b7 patch from issue 2493943003 at patchset 1 (http://crrev.com/2493943003#ps1) Review-Url: https://codereview.chromium.org/2499683002 Cr-Commit-Position: refs/branch-heads/2883@{#564} Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768} [modify] https://crrev.com/39c2ff384acb89d238d1f61549e718eea1257ed3/cc/trees/property_tree.cc [modify] https://crrev.com/39c2ff384acb89d238d1f61549e718eea1257ed3/cc/trees/property_tree_unittest.cc
,
Nov 14 2016
,
Nov 24 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fa5b0164962353a8e0e3ad6597ee9723a1c51985 commit fa5b0164962353a8e0e3ad6597ee9723a1c51985 Author: ajuma <ajuma@chromium.org> Date: Thu Nov 24 16:44:46 2016 Reland of cc: Remove more references to owning_layer_ from RenderSurfaceImpl (patchset #1 id:1 of https://codereview.chromium.org/2483033002/ ) Reason for revert: This turned out not to be the cause of the regression in draw properties computation time. Original issue's description: > Revert of cc: Remove more references to owning_layer_ from RenderSurfaceImpl (patchset #2 id:20001 of https://codereview.chromium.org/2460413003/ ) > > Reason for revert: > Possibly caused a regression in draw properties computation time (see http://crbug.com/663068 ). > > Original issue's description: > > cc: Remove more references to owning_layer_ from RenderSurfaceImpl > > > > This removes more dependencies of RenderSurfaceImpl on its owning layer. > > Specifically, this removes the use of owning_layer_ for accessing > > property trees and debug colors, and for gettng the surface's > > effect tree index. This also renames the OwningLayerId() method to id(). > > > > BUG= 611883 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/740a49caf190bab13f8fdfd0979b29e09752da6b > > Cr-Commit-Position: refs/heads/master@{#428991} > > TBR=jaydasika@chromium.org,weiliangc@chromium.org > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG= 611883 , 663068 > > Committed: https://crrev.com/84667306ea7de0c234ad66737a3c519c21511ade > Cr-Commit-Position: refs/heads/master@{#430419} BUG= 611883 , 663068 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2517273002 Cr-Commit-Position: refs/heads/master@{#434349} [modify] https://crrev.com/fa5b0164962353a8e0e3ad6597ee9723a1c51985/cc/layers/render_surface_impl.cc [modify] https://crrev.com/fa5b0164962353a8e0e3ad6597ee9723a1c51985/cc/layers/render_surface_impl.h [modify] https://crrev.com/fa5b0164962353a8e0e3ad6597ee9723a1c51985/cc/trees/damage_tracker.cc [modify] https://crrev.com/fa5b0164962353a8e0e3ad6597ee9723a1c51985/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 by bugdroid1@chromium.org
, Nov 7 2016