Optimize when we increment cc property tree sequence numbers in PaintArtifactCompositor. |
|||||||||
Issue descriptionSpinoff from http://crrev.com/2702483002 for http://crbug.com/692310 . The tests execute and pass with a release build. Running locally with debug content shell however reports a fatal exception, sample below. It reads as if somehow the main/worker thread property trees are in an incompatible/unexpected state. This is likely the case for many or potentially all of the tests whose timeout failure is fixed in the above change. % ./out/Default/content_shell --enable-slimming-paint-v2 --run-layout-test --expose-internals-for-testing --enable-threaded-compositing third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html #READY Content-Type: text/plain This is a testharness.js-based test. PASS Using KeyframeEffect or KeyframeEffectReadOnly should not change whether an animation is composited Harness: the test ran to completion. #EOF #EOF [1:5:0215/163108.596958:444071241420:FATAL:property_tree.h(68)] Check failed: i < static_cast<int>(nodes_.size()). #0 0x7f5bcfee829b base::debug::StackTrace::StackTrace() #1 0x7f5bcfee68dc base::debug::StackTrace::StackTrace() #2 0x7f5bcff51baf logging::LogMessage::~LogMessage() #3 0x7f5bcbd1102b cc::PropertyTree<>::Node() #4 0x7f5bcbfe66d7 cc::PropertyTrees::PushChangeTrackingTo() #5 0x7f5bcbf8ee1b cc::LayerTreeHostImpl::ActivateSyncTree() #6 0x7f5bcc0287a8 cc::ProxyImpl::ScheduledActionActivateSyncTree() #7 0x7f5bcbeb8110 cc::Scheduler::ProcessScheduledActions() #8 0x7f5bcbeb87c8 cc::Scheduler::NotifyReadyToActivate() #9 0x7f5bcc025251 cc::ProxyImpl::NotifyReadyToActivate() #10 0x7f5bcbf8a31a cc::LayerTreeHostImpl::NotifyReadyToActivate() #11 0x7f5bcbf29fa8 cc::TileManager::CheckAndIssueSignals() #12 0x7f5bcbd78d05 _ZN4base8internal13FunctorTraitsIMN2cc19CompositorFrameSinkEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_ #13 0x7f5bcbf3b3f1 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN2cc11TileManagerEFvvEJPS5_EEEvOT_DpOT0_ #14 0x7f5bcbf3b397 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc11TileManagerEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #15 0x7f5bcbf3b2dc _ZN4base8internal7InvokerINS0_9BindStateIMN2cc11TileManagerEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #16 0x7f5bcbcf33cb base::internal::RunMixin<>::Run() #17 0x7f5bcc06b583 cc::UniqueNotifier::Notify() #18 0x7f5bcbcf3057 _ZN4base8internal13FunctorTraitsIMN2cc28ScrollbarAnimationControllerEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_ #19 0x7f5bcc06b7ca _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN2cc14UniqueNotifierEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_ #20 0x7f5bcc06b752 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc14UniqueNotifierEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #21 0x7f5bcc06b69c _ZN4base8internal7InvokerINS0_9BindStateIMN2cc14UniqueNotifierEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #22 0x7f5bcfeee261 _ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_10RepeatModeE0EEEE3RunEv #23 0x7f5bcfeedade base::debug::TaskAnnotator::RunTask() #24 0x7f5bcff7ad1d base::MessageLoop::RunTask() #25 0x7f5bcff7afb4 base::MessageLoop::DeferOrRunPendingTask() #26 0x7f5bcff7b2a4 base::MessageLoop::DoWork() #27 0x7f5bcff92988 base::MessagePumpDefault::Run() #28 0x7f5bcff7a8b7 base::MessageLoop::RunHandler() #29 0x7f5bd0029b9a base::RunLoop::Run() #30 0x7f5bd00d1664 base::Thread::Run() #31 0x7f5bd00d1eca base::Thread::ThreadMain() #32 0x7f5bd00b879a base::(anonymous namespace)::ThreadFunc() #33 0x7f5bd5580184 start_thread #34 0x7f5bc1e3e37d clone Received signal 6 #0 0x7f5bcfee829b base::debug::StackTrace::StackTrace() #1 0x7f5bcfee68dc base::debug::StackTrace::StackTrace() #2 0x7f5bcfee7daf base::debug::(anonymous namespace)::StackDumpSignalHandler() #3 0x7f5bd5588330 <unknown> #4 0x7f5bc1d7ac37 gsignal #5 0x7f5bc1d7e028 abort #6 0x7f5bcfee4656 base::debug::(anonymous namespace)::DebugBreak() #7 0x7f5bcfee4638 base::debug::BreakDebugger() #8 0x7f5bcff51f46 logging::LogMessage::~LogMessage() #9 0x7f5bcbd1102b #EOF cc::PropertyTree<>::Node() #10 0x7f5bcbfe66d7 cc::PropertyTrees::PushChangeTrackingTo() #11 0x7f5bcbf8ee1b cc::LayerTreeHostImpl::ActivateSyncTree() #12 0x7f5bcc0287a8 cc::ProxyImpl::ScheduledActionActivateSyncTree() #13 0x7f5bcbeb8110 cc::Scheduler::ProcessScheduledActions() #14 0x7f5bcbeb87c8 cc::Scheduler::NotifyReadyToActivate() #15 0x7f5bcc025251 cc::ProxyImpl::NotifyReadyToActivate() #16 0x7f5bcbf8a31a cc::LayerTreeHostImpl::NotifyReadyToActivate() #17 0x7f5bcbf29fa8 cc::TileManager::CheckAndIssueSignals() #18 0x7f5bcbd78d05 _ZN4base8internal13FunctorTraitsIMN2cc19CompositorFrameSinkEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_ #19 0x7f5bcbf3b3f1 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN2cc11TileManagerEFvvEJPS5_EEEvOT_DpOT0_ #20 0x7f5bcbf3b397 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc11TileManagerEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #21 0x7f5bcbf3b2dc _ZN4base8internal7InvokerINS0_9BindStateIMN2cc11TileManagerEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #22 0x7f5bcbcf33cb base::internal::RunMixin<>::Run() #23 0x7f5bcc06b583 cc::UniqueNotifier::Notify() #24 0x7f5bcbcf3057 _ZN4base8internal13FunctorTraitsIMN2cc28ScrollbarAnimationControllerEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_ #25 0x7f5bcc06b7ca _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN2cc14UniqueNotifierEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_ #26 0x7f5bcc06b752 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc14UniqueNotifierEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #27 0x7f5bcc06b69c _ZN4base8internal7InvokerINS0_9BindStateIMN2cc14UniqueNotifierEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #28 0x7f5bcfeee261 _ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_10RepeatModeE0EEEE3RunEv
,
Feb 16 2017
Ah, no, it looks like we're blindly setting sequence_number_ to 1 in PaintArtifactCompositor::update where we: - m_rootLayer->set_property_tree_sequence_number(1) - (child_layer)->set_property_tree_sequence_number(1) - layerTreeHost->property_trees()->sequence_number = 1 - layerTreeHost->property_trees()->needs_rebuild = false Am not sure what the right thing to do is. Presumably we have some way to identify whether the property trees have changed since last time, but I don't know what that is yet, and obviously we're not propagating it in any manner. Investigating but of course any further thoughts welcome.
,
Feb 16 2017
Also worth noting that Layer::property_tree_sequence_number_ must match PropertyTrees::sequence_number_ for the Layer's property tree indexes to be considered valid.
,
Feb 17 2017
Note from brief discussion w/ pdr@/chrishtr@: - short term could bump sequence number every time - subsequently we can look at adding a dirty bit in PaintArtifactCompositor similar to PaintLayerCompositor::setNeedsCompositingUpdate
,
Feb 23 2017
A couple more notes: - SPv2 also sets the sequence number to 1 for all dummy layers in PropertyTreeManager (used by PaintArtifactCompositor). - in both SPv1 and SPv2, cc::PropertyTrees::clear() increments the sequence number for the cleared tree. I'm not sure what this means w.r.t. our plan above to just bump the sequence number every time, since naively this will mean that our monotonically increasing sequence number will conflict with the sequence numbers generated by calls to clear(), which do happen even in SPV2 (see PropertyTreeManager::setupRootTransformNode() or PropertyTreeManager::setupRootClipNode() for example).
,
Feb 23 2017
Could PropertyTreeManager stop directly setting the sequence number and instead rely on the incrementing that happens in clear()? (And then read off the current sequence number and use that on the dummy layers?)
,
Feb 23 2017
Yes, something like that sounds possible. Will explore further and follow up.
,
Feb 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0c0f9ef081109ce6fdeee588d356238272d7b9f8 commit 0c0f9ef081109ce6fdeee588d356238272d7b9f8 Author: wkorman <wkorman@chromium.org> Date: Tue Feb 28 23:44:53 2017 Increment property tree sequence number when updating. BUG= 692842 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2725513002 Cr-Commit-Position: refs/heads/master@{#453746} [modify] https://crrev.com/0c0f9ef081109ce6fdeee588d356238272d7b9f8/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/0c0f9ef081109ce6fdeee588d356238272d7b9f8/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp [modify] https://crrev.com/0c0f9ef081109ce6fdeee588d356238272d7b9f8/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.cpp [modify] https://crrev.com/0c0f9ef081109ce6fdeee588d356238272d7b9f8/third_party/WebKit/Source/platform/graphics/compositing/PropertyTreeManager.h
,
Mar 1 2017
Updating bug for remaining work in c#4 re: dirty bit or further optimization.
,
Mar 7 2017
FYI unit tests don't play well with global counters. (Because unit test reuse process.) PaintArtifactCompositorTestWithPropertyTrees.UpdateProducesNewSequenceNumber always fail the first run then succeed on the second run. How about switching to dynamic expectation?
,
Mar 28 2017
,
May 11 2017
Unassigning self from bugs that I don't expect to be able to get to soon in case someone else is able to pick them up.
,
May 14 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 14 2018
Curious if this is still an issue.
,
May 14 2018
,
Yesterday
(31 hours ago)
This has pretty much been fixed as part of BlinkGenPropertyTrees. We're also implementing a property tree dirty-bit in https://crbug.com/920323 to avoid incrementing the dirty bit unless things change. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by ajuma@chromium.org
, Feb 16 2017