Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2339633003:
Reland of cc: Remove frame queuing from the scheduler. (Closed)
DescriptionReland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ )
Reason for revert:
Reland after fixing screenshot grabber test and perf issues.
Original issue's description:
> Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ )
>
> Reason for revert:
> Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%20Tests%20%28dbg%29%281%29/builds/18015.
>
> Original issue's description:
> > cc: Remove frame queuing from the scheduler.
> >
> > CC scheduler has a frame queuing mechanism called "retro frames". This
> > has been responsible for a lot of complexity and hard to fix bugs. The
> > original intent for adding retro frames was to allow the scheduler to
> > handle multiple frames in flight but that goal doesn't seem feasible or
> > even desirable any more. This CL removes the retro frames queue and
> > instead makes the Scheduler end the previous frame when it receives a
> > BeginFrame message.
> >
> > One surprising behavior was that SyntheticBFS MISSED frames would be
> > queued as retro frames and this would convert the synchronous begin
> > frame call (inside Scheduler::ProcessScheduledActions) to an
> > asynchronous retro frame PostTask. To work around this the Scheduler
> > keeps track of a single CancelableClosure that's used for MISSED frames.
> >
> > The above behavior was also causing the BeginFramesNotFromClient tests
> > to work even though there was an extra MISSED frame in the queue. This
> > is more elegantly solved in another way by using frame number to advance
> > the task runner instead of just running pending tasks.
> >
> > Lastly SchedulerStateMachine is modified so that it's possible to end
> > the previous frame and still have the same behavior as before in the
> > commit to active tree (browser compositor) mode.
> >
> > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org
> > BUG=602485, 644992
> > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
> >
> > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78
> > Cr-Commit-Position: refs/heads/master@{#417764}
>
> TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@chromium.org
> # Not skipping CQ checks because original CL landed more than 1 days ago.
> BUG=602485, 644992
>
> Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd
> Cr-Commit-Position: refs/heads/master@{#417895}
TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=602485, 644992
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
Committed: https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7
Cr-Commit-Position: refs/heads/master@{#421268}
Patch Set 1 #Patch Set 2 : Do not commit before display draws. #Patch Set 3 : fix use of begin frame args after cancelling missed begin frame task #Patch Set 4 : expire missed frames in renderer only #
Total comments: 6
Patch Set 5 : rebase #Patch Set 6 : address comments #Patch Set 7 : destroy scheduler in STP before output surface #Patch Set 8 : make CFS not send swap ack in DetachFromClient #Patch Set 9 : remove capture post tasks from STP::Stop #Patch Set 10 : remove unnecessary Scheduler::DidLoseCFS call when shutting down compositor #Patch Set 11 : rebase #Patch Set 12 : post swap ack from CFS in fresh call stack #
Total comments: 2
Patch Set 13 : delay begin main frame until swap ack in browser compositor #
Total comments: 4
Patch Set 14 : prevent draw if commit is pending #
Total comments: 1
Messages
Total messages: 59 (34 generated)
Created Reland of cc: Remove frame queuing from the scheduler.
Description was changed from ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 ========== to ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
PTAL The main changes are: 1. Do not commit in browser compositor before swap ack of previous frame. The browser compositor will steal all resources for raster in commit which means some mailboxes will be released early, copy requests might fail, etc. This is why the screenshot grabber test was failing and there were some mailbox tests (e.g. TextureLayerImplWithMailboxThreadedCallback.RunSingleThread_DelegatingRenderer) that were also flaking before this fix. 2. Call DidDrawCallback (swap ack) for browser (root) surface after they are used in display draw. Other surfaces can be notified before draw to reduce latency. 3. Expire missed frames in cc scheduler if they're too late but only in renderer. I'm hoping this solves issue 591725 (increased input latency due to browser not drawing missed frames during omnibar animation that's caused by renderer) but I couldn't reproduce the regression locally on a nexus 5 (i.e. ToT and this CL have approx. the same input latency). All changes are against patchset #1 (which is just a plain revert of the revert).
lgtm Sorry for the belated review. I didn't realize you wanted more reviewing. :) https://codereview.chromium.org/2339633003/diff/200001/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2339633003/diff/200001/cc/surfaces/display.cc... cc/surfaces/display.cc:351: // Run draw callback for root surface after drawing. Can you make this comment more of a 'why' and less of a 'what' so that future engineers can understand why we break up the callbacks?
https://codereview.chromium.org/2339633003/diff/200001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2339633003/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:275: if (!state_machine_.BeginFrameNeeded()) { This block of code may cause us to skip frames in cases where the renderer suddenly decides it wants to draw mid frame (for example because of a new input event or a setInterval driven animation). Do you think there would be any harm in removing it? I think the previous logic would queue the BeginFrame up as a retro frame that could still be used even if it weren't needed immediately. https://codereview.chromium.org/2339633003/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:339: // Discard missed begin frames in renderer scheduler if they are too late. Can you add a comment explaining why there's a difference in renderer vs. browser here.
PTAL The screenshot test was still flaky but due to a DCHECK this time (vs failing to take a screenshot). It's also disabled on ToT because it's flaky (fails to take a screenshot). The DCHECK failure happens because we re-enter the scheduler via the surface draw callback (DidSwapBuffersComplete) in STP::Stop which can cause a commit if there was one pending. To fix this I made STP destroy the scheduler before the output surface (NOTE: LTHI must still be destroyed after the output surface). This makes the test stop flaking. The stack trace for the DCHECK: [4515:4515:0919/162252:955258388925:FATAL:task_runner_provider.h(92)] Check failed: !task_runner_provider_->IsMainThreadBlocked(). #0 0x7f9a0699a00e base::debug::StackTrace::StackTrace() #1 0x7f9a06a003fc logging::LogMessage::~LogMessage() #2 0x7f9a00a28f90 cc::DebugScopedSetMainThreadBlocked::DebugScopedSetMainThreadBlocked() #3 0x7f9a00a3fa21 cc::SingleThreadProxy::DoCommit() #4 0x7f9a00a43d1d cc::SingleThreadProxy::ScheduledActionCommit() #5 0x7f9a008d854c cc::Scheduler::ProcessScheduledActions() #6 0x7f9a008d90b8 cc::Scheduler::DidSwapBuffersComplete() #7 0x7f9a00a42168 cc::SingleThreadProxy::DidSwapBuffersCompleteOnImplThread() #8 0x7f9a009883fa cc::LayerTreeHostImpl::DidSwapBuffersComplete() #9 0x7f99f99f90e2 cc::DirectCompositorFrameSink::DidDrawCallback() #10 0x7f99f99fb03d _ZN4base8internal13FunctorTraitsIMN2cc25DirectCompositorFrameSinkEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_ #11 0x7f99f99faf61 _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN2cc25DirectCompositorFrameSinkEFvvEJPS5_EEEvOT_DpOT0_ #12 0x7f99f99faf07 _ZN4base8internal7InvokerINS0_9BindStateIMN2cc25DirectCompositorFrameSinkEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE #13 0x7f99f99fae1c _ZN4base8internal7InvokerINS0_9BindStateIMN2cc25DirectCompositorFrameSinkEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #14 0x7f99f9a1c0cb base::internal::RunMixin<>::Run() #15 0x7f99f9a1cce5 cc::Surface::~Surface() #16 0x7f99f9a460bb std::default_delete<>::operator()() #17 0x7f99f9a4602c std::unique_ptr<>::reset() #18 0x7f99f9a44ae9 std::unique_ptr<>::~unique_ptr() #19 0x7f99f9a55285 std::_Destroy<>() #20 0x7f99f9a5524f std::_Destroy_aux<>::__destroy<>() #21 0x7f99f9a5520d std::_Destroy<>() #22 0x7f99f9a551a1 std::_Destroy<>() #23 0x7f99f9a55f23 std::__cxx1998::vector<>::_M_erase_at_end() #24 0x7f99f9a55ed8 std::__cxx1998::vector<>::clear() #25 0x7f99f9a4e54f std::__debug::vector<>::clear() #26 0x7f99f9a4a7cb cc::SurfaceManager::GarbageCollectSurfaces() #27 0x7f99f9a4a1c6 cc::SurfaceManager::Destroy() #28 0x7f99f9a43a77 cc::SurfaceFactory::Destroy() #29 0x7f99f99f8e6b cc::DirectCompositorFrameSink::DetachFromClient() #30 0x7f9a0098d485 cc::LayerTreeHostImpl::ReleaseCompositorFrameSink() #31 0x7f9a00a40a64 cc::SingleThreadProxy::Stop() #32 0x7f9a009b203f cc::LayerTreeHostInProcess::~LayerTreeHostInProcess() #33 0x7f9a009b21a9 cc::LayerTreeHostInProcess::~LayerTreeHostInProcess() #34 0x7f99fd2b327f std::default_delete<>::operator()() #35 0x7f99fd2ab2dc std::unique_ptr<>::reset() #36 0x7f99fd2a7eeb ui::Compositor::~Compositor() #37 0x7f99fd2a82f9 ui::Compositor::~Compositor() #38 0x7f99fd420baf std::default_delete<>::operator()() #39 0x7f99fd41ccbc std::unique_ptr<>::reset() #40 0x7f99fd41b6b0 aura::WindowTreeHost::DestroyCompositor() #41 0x7f99fd4238f7 aura::WindowTreeHostX11::~WindowTreeHostX11() #42 0x7f99fba27b9e ash::AshWindowTreeHostX11::~AshWindowTreeHostX11() #43 0x7f99fba27c49 ash::AshWindowTreeHostX11::~AshWindowTreeHostX11() #44 0x7f99fb735b9f std::default_delete<>::operator()() #45 0x7f99fb9f3b5c std::unique_ptr<>::reset() #46 0x7f99fba3456b ash::RootWindowController::~RootWindowController() #47 0x7f99fba34799 ash::RootWindowController::~RootWindowController() #48 0x7f99fba0834a ash::WindowTreeHostManager::Shutdown() #49 0x7f99fba47625 ash::Shell::~Shell() #50 0x7f99fba48029 ash::Shell::~Shell() #51 0x7f99fba44f3d ash::Shell::DeleteInstance() #52 0x00000874a15f ash::test::AshTestHelper::TearDown() #53 0x000008748c36 ash::test::AshTestBase::TearDown() #54 0x000001a3f5a4 ash::test::ChromeScreenshotGrabberTest::TearDown() #55 0x0000019137ea _ZN7testing8internal12InvokeHelperIN16sync_file_system18RemoteServiceStateESt5tupleIJEEE12InvokeMethodINS2_25MockRemoteFileSyncServiceEMS8_KFS3_vEEES3_PT_T0_RKS5_ #56 0x00000467b78e testing::internal::HandleExceptionsInMethodIfSupported<>() #57 0x000004670bca testing::Test::Run() #58 0x0000046712a8 testing::TestInfo::Run() #59 0x00000467184a testing::TestCase::Run() #60 0x000004676b9c testing::internal::UnitTestImpl::RunAllTests() #61 0x000001d6b7aa _ZN7testing8internal12InvokeHelperIbSt5tupleIJEEE12InvokeMethodI25MockServiceProcessControlMS6_FbvEEEbPT_T0_RKS3_ https://codereview.chromium.org/2339633003/diff/200001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2339633003/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:275: if (!state_machine_.BeginFrameNeeded()) { On 2016/09/19 22:56:44, brianderson wrote: > This block of code may cause us to skip frames in cases where the renderer > suddenly decides it wants to draw mid frame (for example because of a new input > event or a setInterval driven animation). > > Do you think there would be any harm in removing it? > > I think the previous logic would queue the BeginFrame up as a retro frame that > could still be used even if it weren't needed immediately. This is handled by missed begin frames which are supported by all BFS implementations now. Also the scheduler can't really get a BeginFrame before it starts observing the BFS, it used to be the case that the scheduler was the only observer in the BFS and would receive begin frames even when not needed, but now the BFS doesn't even know about the scheduler until the scheduler calls AddObserver. https://codereview.chromium.org/2339633003/diff/200001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:339: // Discard missed begin frames in renderer scheduler if they are too late. On 2016/09/19 22:56:44, brianderson wrote: > Can you add a comment explaining why there's a difference in renderer vs. > browser here. Actually I removed the exception for the browser. I couldn't reproduce a difference by testing locally. I'll make this change in a separate CL. https://codereview.chromium.org/2339633003/diff/200001/cc/surfaces/display.cc File cc/surfaces/display.cc (right): https://codereview.chromium.org/2339633003/diff/200001/cc/surfaces/display.cc... cc/surfaces/display.cc:351: // Run draw callback for root surface after drawing. On 2016/09/19 22:11:50, enne wrote: > Can you make this comment more of a 'why' and less of a 'what' so that future > engineers can understand why we break up the callbacks? Added comment where we run the callbacks for other surfaces above.
On Mon, Sep 19, 2016 at 4:59 PM, <sunnyps@chromium.org> wrote: > PTAL > > The screenshot test was still flaky but due to a DCHECK this time (vs > failing to > take a screenshot). It's also disabled on ToT because it's flaky (fails to > take > a screenshot). > > The DCHECK failure happens because we re-enter the scheduler via the > surface > draw callback (DidSwapBuffersComplete) in STP::Stop which can cause a > commit if > there was one pending. To fix this I made STP destroy the scheduler before > the > output surface (NOTE: LTHI must still be destroyed after the output > surface). > This makes the test stop flaking. > I would like it if ordering between Single and Threaded proxies was the same, so changes to one should also be in the other. > > The stack trace for the DCHECK: > [4515:4515:0919/162252:955258388925:FATAL:task_runner_provider.h(92)] > Check > failed: !task_runner_provider_->IsMainThreadBlocked(). > #0 0x7f9a0699a00e base::debug::StackTrace::StackTrace() > #1 0x7f9a06a003fc logging::LogMessage::~LogMessage() > #2 0x7f9a00a28f90 > cc::DebugScopedSetMainThreadBlocked::DebugScopedSetMainThreadBlocked() > #3 0x7f9a00a3fa21 cc::SingleThreadProxy::DoCommit() > #4 0x7f9a00a43d1d cc::SingleThreadProxy::ScheduledActionCommit() > #5 0x7f9a008d854c cc::Scheduler::ProcessScheduledActions() > #6 0x7f9a008d90b8 cc::Scheduler::DidSwapBuffersComplete() > #7 0x7f9a00a42168 cc::SingleThreadProxy::DidSwap > BuffersCompleteOnImplThread() > #8 0x7f9a009883fa cc::LayerTreeHostImpl::DidSwapBuffersComplete() > #9 0x7f99f99f90e2 cc::DirectCompositorFrameSink::DidDrawCallback() > #10 0x7f99f99fb03d > _ZN4base8internal13FunctorTraitsIMN2cc25DirectCompositorFram > eSinkEFvvEvE6InvokeIPS3_JEEEvS5_OT_DpOT0_ > #11 0x7f99f99faf61 > _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN2cc25Dir > ectCompositorFrameSinkEFvvEJPS5_EEEvOT_DpOT0_ > #12 0x7f99f99faf07 > _ZN4base8internal7InvokerINS0_9BindStateIMN2cc25DirectCompos > itorFrameSinkEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE7Run > ImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE > #13 0x7f99f99fae1c > _ZN4base8internal7InvokerINS0_9BindStateIMN2cc25DirectCompos > itorFrameSinkEFvvEJNS0_17UnretainedWrapperIS4_EEEEEFvvEE3Run > EPNS0_13BindStateBaseE > #14 0x7f99f9a1c0cb base::internal::RunMixin<>::Run() > #15 0x7f99f9a1cce5 cc::Surface::~Surface() > #16 0x7f99f9a460bb std::default_delete<>::operator()() > #17 0x7f99f9a4602c std::unique_ptr<>::reset() > #18 0x7f99f9a44ae9 std::unique_ptr<>::~unique_ptr() > #19 0x7f99f9a55285 std::_Destroy<>() > #20 0x7f99f9a5524f std::_Destroy_aux<>::__destroy<>() > #21 0x7f99f9a5520d std::_Destroy<>() > #22 0x7f99f9a551a1 std::_Destroy<>() > #23 0x7f99f9a55f23 std::__cxx1998::vector<>::_M_erase_at_end() > #24 0x7f99f9a55ed8 std::__cxx1998::vector<>::clear() > #25 0x7f99f9a4e54f std::__debug::vector<>::clear() > #26 0x7f99f9a4a7cb cc::SurfaceManager::GarbageCollectSurfaces() > #27 0x7f99f9a4a1c6 cc::SurfaceManager::Destroy() > #28 0x7f99f9a43a77 cc::SurfaceFactory::Destroy() > #29 0x7f99f99f8e6b cc::DirectCompositorFrameSink::DetachFromClient() > #30 0x7f9a0098d485 cc::LayerTreeHostImpl::ReleaseCompositorFrameSink() > #31 0x7f9a00a40a64 cc::SingleThreadProxy::Stop() > #32 0x7f9a009b203f cc::LayerTreeHostInProcess::~LayerTreeHostInProcess() > #33 0x7f9a009b21a9 cc::LayerTreeHostInProcess::~LayerTreeHostInProcess() > #34 0x7f99fd2b327f std::default_delete<>::operator()() > #35 0x7f99fd2ab2dc std::unique_ptr<>::reset() > #36 0x7f99fd2a7eeb ui::Compositor::~Compositor() > #37 0x7f99fd2a82f9 ui::Compositor::~Compositor() > #38 0x7f99fd420baf std::default_delete<>::operator()() > #39 0x7f99fd41ccbc std::unique_ptr<>::reset() > #40 0x7f99fd41b6b0 aura::WindowTreeHost::DestroyCompositor() > #41 0x7f99fd4238f7 aura::WindowTreeHostX11::~WindowTreeHostX11() > #42 0x7f99fba27b9e ash::AshWindowTreeHostX11::~AshWindowTreeHostX11() > #43 0x7f99fba27c49 ash::AshWindowTreeHostX11::~AshWindowTreeHostX11() > #44 0x7f99fb735b9f std::default_delete<>::operator()() > #45 0x7f99fb9f3b5c std::unique_ptr<>::reset() > #46 0x7f99fba3456b ash::RootWindowController::~RootWindowController() > #47 0x7f99fba34799 ash::RootWindowController::~RootWindowController() > #48 0x7f99fba0834a ash::WindowTreeHostManager::Shutdown() > #49 0x7f99fba47625 ash::Shell::~Shell() > #50 0x7f99fba48029 ash::Shell::~Shell() > #51 0x7f99fba44f3d ash::Shell::DeleteInstance() > #52 0x00000874a15f ash::test::AshTestHelper::TearDown() > #53 0x000008748c36 ash::test::AshTestBase::TearDown() > #54 0x000001a3f5a4 ash::test::ChromeScreenshotGrabberTest::TearDown() > #55 0x0000019137ea > _ZN7testing8internal12InvokeHelperIN16sync_file_system18Remo > teServiceStateESt5tupleIJEEE12InvokeMethodINS2_25MockRemoteF > ileSyncServiceEMS8_KFS3_vEEES3_PT_T0_RKS5_ > #56 0x00000467b78e testing::internal::HandleExcep > tionsInMethodIfSupported<>() > #57 0x000004670bca testing::Test::Run() > #58 0x0000046712a8 testing::TestInfo::Run() > #59 0x00000467184a testing::TestCase::Run() > #60 0x000004676b9c testing::internal::UnitTestImpl::RunAllTests() > #61 0x000001d6b7aa > _ZN7testing8internal12InvokeHelperIbSt5tupleIJEEE12InvokeMet > hodI25MockServiceProcessControlMS6_FbvEEEbPT_T0_RKS3_ > > > https://codereview.chromium.org/2339633003/diff/200001/cc/sc > heduler/scheduler.cc > File cc/scheduler/scheduler.cc (right): > > https://codereview.chromium.org/2339633003/diff/200001/cc/sc > heduler/scheduler.cc#newcode275 > cc/scheduler/scheduler.cc:275: if (!state_machine_.BeginFrameNeeded()) { > On 2016/09/19 22:56:44, brianderson wrote: > > This block of code may cause us to skip frames in cases where the > renderer > > suddenly decides it wants to draw mid frame (for example because of a > new input > > event or a setInterval driven animation). > > > > Do you think there would be any harm in removing it? > > > > I think the previous logic would queue the BeginFrame up as a retro > frame that > > could still be used even if it weren't needed immediately. > > This is handled by missed begin frames which are supported by all BFS > implementations now. Also the scheduler can't really get a BeginFrame > before it starts observing the BFS, it used to be the case that the > scheduler was the only observer in the BFS and would receive begin > frames even when not needed, but now the BFS doesn't even know about the > scheduler until the scheduler calls AddObserver. > > https://codereview.chromium.org/2339633003/diff/200001/cc/sc > heduler/scheduler.cc#newcode339 > cc/scheduler/scheduler.cc:339: // Discard missed begin frames in > renderer scheduler if they are too late. > On 2016/09/19 22:56:44, brianderson wrote: > > Can you add a comment explaining why there's a difference in renderer > vs. > > browser here. > > Actually I removed the exception for the browser. I couldn't reproduce a > difference by testing locally. I'll make this change in a separate CL. > > https://codereview.chromium.org/2339633003/diff/200001/cc/su > rfaces/display.cc > File cc/surfaces/display.cc (right): > > https://codereview.chromium.org/2339633003/diff/200001/cc/su > rfaces/display.cc#newcode351 > cc/surfaces/display.cc:351: // Run draw callback for root surface after > drawing. > On 2016/09/19 22:11:50, enne wrote: > > Can you make this comment more of a 'why' and less of a 'what' so that > future > > engineers can understand why we break up the callbacks? > > Added comment where we run the callbacks for other surfaces above. > > https://codereview.chromium.org/2339633003/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
PTAL After some testing I also found that layer tree tests also perform actions in the middle of ~ProxyImpl because of the same issue viz. running the draw callback -> swap ack in the surface dtor. Also running the commit in STP::Stop is not safe because the surface is destroyed by that time and CFS::ForceReclaimResources tries to submit an empty frame to the surface. So instead of adding more state to the scheduler or STP/ProxyImpl, I made sure that the CFS implementations don't run the draw callback in DetachFromClient. This makes the screenshot test pass without flaking and probably fixes other layer tree test flakes too. I also made a couple of minor cleanups - removed the call to Scheduler::DidLoseCFS in STP::Stop/~ProxyImpl and removed the CapturePostTasks in STP::Stop.
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Sigh... running commit inside the same stack frame as Display::DrawAndSwap does not seem to be safe, itcrashes in CalDrawProps > ComputeVisibleRects?!? So I've made all the CFS post a task for DidSwapBuffersComplete. Hopefully the testing gods will be happy with this new sacrifice.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
sammc@chromium.org changed reviewers: - sammc@chromium.org
The number of patches in this code review also make me nervous that this shutdown path has gotten really unsafe. Is there anything that can be done to make it more robust to future changes so that other people don't have to go through this same pain? Are there DCHECKs that could be added to help with shutdown state? Could you at least leave more comments in the Stop() function about why things are done in what order and what you do and don't expect to happen? On 2016/09/21 at 02:17:38, sunnyps wrote: > Sigh... running commit inside the same stack frame as Display::DrawAndSwap does not seem to be safe, itcrashes in CalDrawProps > ComputeVisibleRects?!? So I've made all the CFS post a task for DidSwapBuffersComplete. Hopefully the testing gods will be happy with this new sacrifice. That seems a little unsafe to post that without a weak pointer. Can you explain more how this should work? https://codereview.chromium.org/2339633003/diff/360001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (right): https://codereview.chromium.org/2339633003/diff/360001/cc/trees/proxy_impl.cc... cc/trees/proxy_impl.cc:103: layer_tree_host_impl_->ReleaseCompositorFrameSink(); It's a little odd that single thread proxy now calls did lose compositor frame sink but proxy impl doesn't. Does this cause any issues? Should the single thread proxy one not do that as well?
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/21 17:38:55, enne wrote: > The number of patches in this code review also make me nervous that this > shutdown path has gotten really unsafe. Is there anything that can be done to > make it more robust to future changes so that other people don't have to go > through this same pain? Are there DCHECKs that could be added to help with > shutdown state? Could you at least leave more comments in the Stop() function > about why things are done in what order and what you do and don't expect to > happen? Ok, so here's what happened. Before this CL it's possible for BeginMainFrame (and Commit) to happen before the previous frame's DidSwapBuffersComplete. Removing retro frames just made it more likely. I tried to fix that by first trying to delay commit until swap ack. This presented three problems: 1. Display calls the did draw callbacks before actually drawing and that calls DidSwapBuffersComplete synchronously. So that would start the commit in the middle of the display draw which is very bad. That can be fixed by either a. moving the did draw for the root surface to after display draw or b. making the DidSwapBuffersComplete use a new post task. I chose the second option. 2. In STP::Stop (shutdown) destroying the CFS would also lead to a DidSwapBuffersComplete and a commit (if pending) in the same stack frame. That was also bad because it would trip up the DebugScopedSetMainThreadBlocked DCHECK. Using a separate post task for DidSwapBuffersComplete also fixes that. 3. Between BeginMainFrame and (delayed) Commit in STP it's possible for layers to be reparented which means that the state used in Commit is invalid. This caused the crashes in CalcDrawProps above. This is irrespective of whether DidSwapBuffersComplete is on a post task. This doesn't happen in renderer because there the main thread is blocked until commit completes. To fix the last issue I tried delaying BeginMainFrame to happen after the swap ack. Cleaning up the scheduler begin main frame state tracking was necessary to make this manageable. So to sum up: 1. BeginMainFrame doesn't happen until after the previous swap ack so frames aren't skipped and CopyOutputRequests aren't lost. 2. BeginMainFrame and Commit must happen together synchronously. 3. CFS uses post task for DidSwapBuffersComplete. 4. STP::Stop and ~ProxyImpl don't run any scheduler actions any more. <--- Need a good way to DCHECK this without adding a bunch of code. > > On 2016/09/21 at 02:17:38, sunnyps wrote: > > > Sigh... running commit inside the same stack frame as Display::DrawAndSwap > does not seem to be safe, itcrashes in CalDrawProps > ComputeVisibleRects?!? So > I've made all the CFS post a task for DidSwapBuffersComplete. Hopefully the > testing gods will be happy with this new sacrifice. > > That seems a little unsafe to post that without a weak pointer. Can you explain > more how this should work? CompositorFrameSink::PostDidSwapBuffersComplete uses a weak pointer. All the subclasses call that method. The draw callback (submitted to surface factory) doesn't need a weak pointer because it's guaranteed to be called when the surface is destroyed (this is same as before). > > https://codereview.chromium.org/2339633003/diff/360001/cc/trees/proxy_impl.cc > File cc/trees/proxy_impl.cc (right): > > https://codereview.chromium.org/2339633003/diff/360001/cc/trees/proxy_impl.cc... > cc/trees/proxy_impl.cc:103: layer_tree_host_impl_->ReleaseCompositorFrameSink(); > It's a little odd that single thread proxy now calls did lose compositor frame > sink but proxy impl doesn't. Does this cause any issues? Should the single > thread proxy one not do that as well?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
It is definitely the case that in SingleThreadProxy, BeginMainFrame and Commit should happen in the same stack frame. There's a line in SingleThreadProxy::BeginMainFrame where swap promises are aborted with a scoped tracker, and so if a commit happens out of that stack frame, then we lose swap promises. I'm not sure I understand how this was happening. Is this because of retro begin frames? Or, what could cause this? https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:497: // if (settings_.commit_to_active_tree && SwapThrottled()) Leftover code? https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:499: DCHECK(!settings_.commit_to_active_tree || !SwapThrottled()); It's nice to DCHECK this.
The CQ bit was checked by sunnyps@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/22 19:01:48, enne wrote: > It is definitely the case that in SingleThreadProxy, BeginMainFrame and Commit > should happen in the same stack frame. There's a line in > SingleThreadProxy::BeginMainFrame where swap promises are aborted with a scoped > tracker, and so if a commit happens out of that stack frame, then we lose swap > promises. > > I'm not sure I understand how this was happening. Is this because of retro > begin frames? Or, what could cause this? This was happening because I made commit delayed until swap ack to fix CopyOutputRequests from disappearing. The proper fix of course was to delay BeginMainFrame instead of commit. > > https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... > File cc/scheduler/scheduler_state_machine.cc (right): > > https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... > cc/scheduler/scheduler_state_machine.cc:497: // if > (settings_.commit_to_active_tree && SwapThrottled()) > Leftover code? > > https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... > cc/scheduler/scheduler_state_machine.cc:499: > DCHECK(!settings_.commit_to_active_tree || !SwapThrottled()); > It's nice to DCHECK this.
On 2016/09/22 19:01:48, enne wrote: > It is definitely the case that in SingleThreadProxy, BeginMainFrame and Commit > should happen in the same stack frame. There's a line in > SingleThreadProxy::BeginMainFrame where swap promises are aborted with a scoped > tracker, and so if a commit happens out of that stack frame, then we lose swap > promises. > > I'm not sure I understand how this was happening. Is this because of retro > begin frames? Or, what could cause this? This was happening because I made commit delayed until swap ack to fix CopyOutputRequests from disappearing. The proper fix of course was to delay BeginMainFrame instead of commit. > > https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... > File cc/scheduler/scheduler_state_machine.cc (right): > > https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... > cc/scheduler/scheduler_state_machine.cc:497: // if > (settings_.commit_to_active_tree && SwapThrottled()) > Leftover code? > > https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... > cc/scheduler/scheduler_state_machine.cc:499: > DCHECK(!settings_.commit_to_active_tree || !SwapThrottled()); > It's nice to DCHECK this.
I'll ask for a review once the tests pass. Some tests were failing on the !SwapThrottled() DCHECK during commit because impl-side animations can cause draws (without commit) in browser compositor. These aren't harmful TBH but there are two side-effects: 1. The next BeginMainFrame/Commit might happen before Display draws the frame and reclaim resources submitted in the draw. This will just delay Display draw and possibly waste some raster/compositing work. 2. The impl-side draws can potentially generate new raster work and this is not single buffered in the same way that commit reclaims resources. So some extra memory usage might be seen. So you might think that we should restrict browser compositor to only draw new active trees (active_tree_needs_first_draw_ in SchedulerStateMachine). But that causes some LayerTreeTests (approx. 7) to fail because they rely on draws to advance state and finish. One example is LayerTreeHostAnimationTestAnimationsGetDeleted which waits for an animation to finish on the impl thread and get deleted on the main thread. But animations only update their lifecycle state (started, finished, etc.) in draw. And this test has no commits except at the beginning and the end. So I tried the next best thing, restrict impl-side draws if commit is pending but allow otherwise. This makes the layer tree tests as well as the browser tests which were crashing pass. https://codereview.chromium.org/2339633003/diff/360001/cc/trees/proxy_impl.cc File cc/trees/proxy_impl.cc (right): https://codereview.chromium.org/2339633003/diff/360001/cc/trees/proxy_impl.cc... cc/trees/proxy_impl.cc:103: layer_tree_host_impl_->ReleaseCompositorFrameSink(); On 2016/09/21 17:38:55, enne wrote: > It's a little odd that single thread proxy now calls did lose compositor frame > sink but proxy impl doesn't. Does this cause any issues? Should the single > thread proxy one not do that as well? This is LTHI::ReleaseCFS not Scheduler::DidLoseCFS which was removed. Both STP and ProxyImpl only call ReleaseCFS. There's no need to call DidLoseCFS because the scheduler isn't expected to do anything at this point. https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:497: // if (settings_.commit_to_active_tree && SwapThrottled()) On 2016/09/22 19:01:47, enne wrote: > Leftover code? Removed. https://codereview.chromium.org/2339633003/diff/380001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:499: DCHECK(!settings_.commit_to_active_tree || !SwapThrottled()); On 2016/09/22 19:01:47, enne wrote: > It's nice to DCHECK this. Yes, although this crashes when we have pure impl-side draws.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Woo hoo passed the CQ this time PTAL
Thanks for wrangling this. :) > So I tried the next best thing, restrict impl-side draws if commit is pending but allow otherwise. This makes the layer tree tests as well as the browser tests which were crashing pass. This makes sense to me. There's no reason to draw while the begin main frame message is in flight. This change lgtm.
lgtm https://codereview.chromium.org/2339633003/diff/400001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (left): https://codereview.chromium.org/2339633003/diff/400001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:126: case BEGIN_MAIN_FRAME_STATE_WAITING_FOR_ACTIVATION: Nice way to remove these states.
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by sunnyps@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ==========
Message was sent while issue was closed.
Committed patchset #14 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel ========== to ========== Reland of cc: Remove frame queuing from the scheduler. (patchset #1 id:1 of https://codereview.chromium.org/2336493002/ ) Reason for revert: Reland after fixing screenshot grabber test and perf issues. Original issue's description: > Revert of cc: Remove frame queuing from the scheduler. (patchset #3 id:40001 of https://codereview.chromium.org/2323063004/ ) > > Reason for revert: > Broke ChromeScreenshotGrabberTest.TakeScreenshot on Linux ChromiumOS Tests (dbg)(1): https://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%.... > > Original issue's description: > > cc: Remove frame queuing from the scheduler. > > > > CC scheduler has a frame queuing mechanism called "retro frames". This > > has been responsible for a lot of complexity and hard to fix bugs. The > > original intent for adding retro frames was to allow the scheduler to > > handle multiple frames in flight but that goal doesn't seem feasible or > > even desirable any more. This CL removes the retro frames queue and > > instead makes the Scheduler end the previous frame when it receives a > > BeginFrame message. > > > > One surprising behavior was that SyntheticBFS MISSED frames would be > > queued as retro frames and this would convert the synchronous begin > > frame call (inside Scheduler::ProcessScheduledActions) to an > > asynchronous retro frame PostTask. To work around this the Scheduler > > keeps track of a single CancelableClosure that's used for MISSED frames. > > > > The above behavior was also causing the BeginFramesNotFromClient tests > > to work even though there was an extra MISSED frame in the queue. This > > is more elegantly solved in another way by using frame number to advance > > the task runner instead of just running pending tasks. > > > > Lastly SchedulerStateMachine is modified so that it's possible to end > > the previous frame and still have the same behavior as before in the > > commit to active tree (browser compositor) mode. > > > > R=brianderson@chromium.org,enne@chromium.org,danakj@chromium.org > > BUG=602485, 644992 > > CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel > > > > Committed: https://crrev.com/559280b26cc5672f5f054e8ac35281e804c14d78 > > Cr-Commit-Position: refs/heads/master@{#417764} > > TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sunnyps@ch... > # Not skipping CQ checks because original CL landed more than 1 days ago. > BUG=602485, 644992 > > Committed: https://crrev.com/95beb47e50065959ee2f5b43cf431431e32e14cd > Cr-Commit-Position: refs/heads/master@{#417895} TBR=enne@chromium.org,brianderson@chromium.org,danakj@chromium.org,sammc@chro... # Not skipping CQ checks because original CL landed more than 1 days ago. BUG=602485, 644992 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel Committed: https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7 Cr-Commit-Position: refs/heads/master@{#421268} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/864a70f6f93a87ff374bf2aea2494d4d7d0150d7 Cr-Commit-Position: refs/heads/master@{#421268}
Message was sent while issue was closed.
A revert of this CL (patchset #14 id:400001) has been created in https://codereview.chromium.org/2379343003/ by brianderson@chromium.org. The reason for reverting is: Highly likely that this is causing the flakes we are seeing in http://crbug.com/645736.
Message was sent while issue was closed.
On 2016/09/30 23:44:38, brianderson wrote: > A revert of this CL (patchset #14 id:400001) has been created in > https://codereview.chromium.org/2379343003/ by mailto:brianderson@chromium.org. > > The reason for reverting is: Highly likely that this is causing the flakes we > are seeing in http://crbug.com/645736. New revert here: https://codereview.chromium.org/2386183003
Message was sent while issue was closed.
On 2016/09/30 23:44:38, brianderson wrote: > A revert of this CL (patchset #14 id:400001) has been created in > https://codereview.chromium.org/2379343003/ by mailto:brianderson@chromium.org. > > The reason for reverting is: Highly likely that this is causing the flakes we > are seeing in http://crbug.com/645736. New revert here: https://codereview.chromium.org/2386183003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
