Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
508373002:
cc: Single-threaded impl-side painting for unit tests (Closed)
|
Created:
6 years, 3 months ago by enne (OOO) Modified:
6 years, 2 months ago Reviewers:
danakj CC:
cc-bugs_chromium.org, chromium-reviews, reveman, vmpstr Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptioncc: Single-threaded impl-side painting for unit tests
This adds methods to SingleThreadProxy to support impl-side painting
when using the scheduler. This allows most unit tests to test impl-side
painting classes through both the single and threaded paths.
The biggest caveat to this approach is the "commit waits for activation"
logic that mailboxes want to use. Because there is no easy way to block
the only thread there is while activation happens, activation always
happens immediately during commit, even if the tree is not ready. To
cause the tree to wait to draw until it is ready, drawing is prevented
via SetRequiresHighResToDraw. This prepares the way for the future
where the SingleThreadProxy will likely commit directly to the active
tree.
The synchronous CompositeImmediately function does not support impl-side
painting yet. This is the path used by everything but unit tests at
this point.
R=danakj@chromium.org
BUG=329553
Committed: https://crrev.com/98f3a6c5c30f1ecfc934838688ce265455a35ec9
Cr-Commit-Position: refs/heads/master@{#298960}
Patch Set 1 #
Total comments: 27
Patch Set 2 : Rebase #Patch Set 3 : danakj review #
Total comments: 8
Patch Set 4 : more danakj review #Patch Set 5 : Rebase #Patch Set 6 : Rebase #Patch Set 7 : Support failing PrepareToDraw in STP and tests #Patch Set 8 : Do BeginFrame stuff in BeginFrame, not lazily in commit #
Total comments: 1
Patch Set 9 : Fix aborted swap promises in synchronous composite case #Patch Set 10 : Also abort remaining swap promises for synchronous composite #
Total comments: 1
Patch Set 11 : Rebase #Patch Set 12 : Swap promise unit test #
Total comments: 3
Patch Set 13 : Add comments #Patch Set 14 : Rebase #
Messages
Total messages: 58 (18 generated)
https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:532: // This test does not make sense in the single-threaded impl-side painting These two tests are the only ones that called BlockNotifyReadyToActivateForTesting and failed. This is clearly not supported in the SingleThreadProxy world that always calls NotifyReadyToActivate during commit. Unfortunately, there are other tests that call this, but they don't fail. Maybe I should make BlockNotifyReadyToActivateForTesting explode if it's single threaded and impl-side painting and then just make some SINGLE_AND_MULTI_THREAD_BLOCKNOTIFY_TEST_F macro that runs everything but that configuration? https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy.cc File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:241: NotifyReadyToActivate(); Unfortunateness #1: This works, but feels unsafe. SetNextCommitWaitsForActivation really means that you want to activate synchronously inside of commit. I think that is the only way to make that logic work in a single-threaded world. Unfortunately, Because ScheduledActionCommit is already inside of Scheduler::ProcessActions, NotifyReadyToActivate will not cause ScheduledActionActivateSyncTree to happen. So, it's impossible to tell from inside of ScheduledActionCommit that ScheduledActionActivate will happen immediately afterwards. Looking at the code, it will, always. However, I don't know how to check this. I could add some hokey "PleaseExplodeIfActivationHasntHappened" flag into Scheduler and check that when returning from ScheduledActionCommit. That's the best I can come up with. https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:430: MainThreadTaskRunner()->PostTask( Unfortunateness #2: Because activation is controlled via the scheduler, there's no stack frame where I can block all post tasks. So, things like mailbox returning gets posted during commit. That means when DidActivateSyncTree is called, they've been posted but haven't run. So, CommitComplete can't be called directly or it will happen before those tasks (incorrectly). To fix this ordering, I just PostTask CommitComplete here. Given that commit -> activation -> DidActivateSyncTree have all happened synchronously, I think this is legit, but it's weird. I could potentially create a member variable blocked task runner and create it in commit and destroy it here, rather than the scoped version that exists inside of commit today. Maybe?
danakj: ping?
https://codereview.chromium.org/508373002/diff/1/cc/test/layer_tree_test.h File cc/test/layer_tree_test.h (right): https://codereview.chromium.org/508373002/diff/1/cc/test/layer_tree_test.h#ne... cc/test/layer_tree_test.h:221: } Can you put "class SingleThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}" here? https://codereview.chromium.org/508373002/diff/1/cc/test/layer_tree_test.h#ne... cc/test/layer_tree_test.h:234: } same here? https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:532: // This test does not make sense in the single-threaded impl-side painting On 2014/08/27 23:53:38, enne wrote: > These two tests are the only ones that called > BlockNotifyReadyToActivateForTesting and failed. This is clearly not supported > in the SingleThreadProxy world that always calls NotifyReadyToActivate during > commit. Unfortunately, there are other tests that call this, but they don't > fail. > > Maybe I should make BlockNotifyReadyToActivateForTesting explode if it's single > threaded and impl-side painting and then just make some > SINGLE_AND_MULTI_THREAD_BLOCKNOTIFY_TEST_F macro that runs everything but that > configuration? I like the sound of that idea. https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2228: SINGLE_THREAD_NOIMPL_TEST_F(LayerTreeHostTestLCDNotification); Why this? What happened to MULTI? https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2412: scoped_refptr<ContentLayer> root_layer = ContentLayer::Create(&client_); can you make this a PictureLayer and run this with implside instead? https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2425: virtual void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) OVERRIDE { This should be DidActivateOnThread since it's using the active_tree() https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2442: SINGLE_THREAD_NOIMPL_TEST_F( can this be SINGLE_AND_MULTI_IMPL after all that? https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy.cc File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:241: NotifyReadyToActivate(); On 2014/08/27 23:53:38, enne wrote: > Unfortunateness #1: This works, but feels unsafe. > > SetNextCommitWaitsForActivation really means that you want to activate > synchronously inside of commit. I think that is the only way to make that logic > work in a single-threaded world. Unfortunately, Because ScheduledActionCommit > is already inside of Scheduler::ProcessActions, NotifyReadyToActivate will not > cause ScheduledActionActivateSyncTree to happen. So, it's impossible to tell I'm confused, NotifyReadyToActivate won't cause it to happen? Do you mean it will happen when we unwind the stack to ProcessActions? That is what I am expecting. > from inside of ScheduledActionCommit that ScheduledActionActivate will happen > immediately afterwards. Looking at the code, it will, always. However, I don't > know how to check this. > I could add some hokey "PleaseExplodeIfActivationHasntHappened" flag into > Scheduler and check that when returning from ScheduledActionCommit. That's the > best I can come up with. You could DCHECK there's no pending tree in STP::CommitComplete? https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:415: scheduler_on_impl_thread_->SetSmoothnessTakesPriority(false); Is there really any need to do this? Should we just do this once at startup and nothing in here? https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:424: layer_tree_host_impl_->active_tree()->SetRequiresHighResToDraw(); This is going to be bad when we are OOM? We will skip frames cuz of this flag if any tile is a checkerboard. I'm not sure what to suggest tho.. or am I wrong and this is okay? https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:430: MainThreadTaskRunner()->PostTask( On 2014/08/27 23:53:39, enne wrote: > Unfortunateness #2: Because activation is controlled via the scheduler, there's > no stack frame where I can block all post tasks. So, things like mailbox > returning gets posted during commit. That means when DidActivateSyncTree is > called, they've been posted but haven't run. So, CommitComplete can't be called > directly or it will happen before those tasks (incorrectly). To fix this > ordering, I just PostTask CommitComplete here. Given that commit -> activation > -> DidActivateSyncTree have all happened synchronously, I think this is legit, > but it's weird. > > I could potentially create a member variable blocked task runner and create it > in commit and destroy it here, rather than the scoped version that exists inside > of commit today. Maybe? Ya, creating a CaptureTasks and holding it thru the activation would be more performant.
https://codereview.chromium.org/508373002/diff/1/cc/test/layer_tree_test.h File cc/test/layer_tree_test.h (right): https://codereview.chromium.org/508373002/diff/1/cc/test/layer_tree_test.h#ne... cc/test/layer_tree_test.h:221: } On 2014/09/03 17:01:31, danakj wrote: > Can you put "class SingleThreadDirectNoImplNeedsSemicolon##TEST_FIXTURE_NAME {}" > here? Done. https://codereview.chromium.org/508373002/diff/1/cc/test/layer_tree_test.h#ne... cc/test/layer_tree_test.h:234: } On 2014/09/03 17:01:31, danakj wrote: > same here? Done. https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:532: // This test does not make sense in the single-threaded impl-side painting On 2014/09/03 17:01:31, danakj wrote: > On 2014/08/27 23:53:38, enne wrote: > > These two tests are the only ones that called > > BlockNotifyReadyToActivateForTesting and failed. This is clearly not > supported > > in the SingleThreadProxy world that always calls NotifyReadyToActivate during > > commit. Unfortunately, there are other tests that call this, but they don't > > fail. > > > > Maybe I should make BlockNotifyReadyToActivateForTesting explode if it's > single > > threaded and impl-side painting and then just make some > > SINGLE_AND_MULTI_THREAD_BLOCKNOTIFY_TEST_F macro that runs everything but that > > configuration? > > I like the sound of that idea. Done. https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2228: SINGLE_THREAD_NOIMPL_TEST_F(LayerTreeHostTestLCDNotification); On 2014/09/03 17:01:32, danakj wrote: > Why this? What happened to MULTI? Fixed. https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2412: scoped_refptr<ContentLayer> root_layer = ContentLayer::Create(&client_); On 2014/09/03 17:01:31, danakj wrote: > can you make this a PictureLayer and run this with implside instead? Added PictureLayer when impl-side painting. Left existing code since ContentLayer is still shipping. https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2425: virtual void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) OVERRIDE { On 2014/09/03 17:01:32, danakj wrote: > This should be DidActivateOnThread since it's using the active_tree() Done. https://codereview.chromium.org/508373002/diff/1/cc/trees/layer_tree_host_uni... cc/trees/layer_tree_host_unittest.cc:2442: SINGLE_THREAD_NOIMPL_TEST_F( On 2014/09/03 17:01:32, danakj wrote: > can this be SINGLE_AND_MULTI_IMPL after all that? Made it SINGLE_AND_MULTI instead. https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy.cc File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:241: NotifyReadyToActivate(); On 2014/09/03 17:01:32, danakj wrote: > On 2014/08/27 23:53:38, enne wrote: > > Unfortunateness #1: This works, but feels unsafe. > > > > SetNextCommitWaitsForActivation really means that you want to activate > > synchronously inside of commit. I think that is the only way to make that > logic > > work in a single-threaded world. Unfortunately, Because ScheduledActionCommit > > is already inside of Scheduler::ProcessActions, NotifyReadyToActivate will not > > cause ScheduledActionActivateSyncTree to happen. So, it's impossible to tell > > I'm confused, NotifyReadyToActivate won't cause it to happen? Do you mean it > will > happen when we unwind the stack to ProcessActions? That is what I am expecting. Yes, that's exactly it. NotifyReadyToActivate will not cause a synchronous activation; it will instead (hopefully) cause an activation during the stack unwind. > > from inside of ScheduledActionCommit that ScheduledActionActivate will happen > > immediately afterwards. Looking at the code, it will, always. However, I > don't > > know how to check this. > > > I could add some hokey "PleaseExplodeIfActivationHasntHappened" flag into > > Scheduler and check that when returning from ScheduledActionCommit. That's > the > > best I can come up with. > > You could DCHECK there's no pending tree in STP::CommitComplete? Great suggestion! Done. https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:415: scheduler_on_impl_thread_->SetSmoothnessTakesPriority(false); On 2014/09/03 17:01:32, danakj wrote: > Is there really any need to do this? Should we just do this once at startup and > nothing in here? Done. https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:424: layer_tree_host_impl_->active_tree()->SetRequiresHighResToDraw(); On 2014/09/03 17:01:32, danakj wrote: > This is going to be bad when we are OOM? We will skip frames cuz of this flag if > any tile is a checkerboard. I'm not sure what to suggest tho.. or am I wrong and > this is okay? I think the discussion we had today about raster-on-demand (or alternatively calling it like "I can't get a resource for this so consider it high res", but in a more pithy way) covers this case. https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:430: MainThreadTaskRunner()->PostTask( On 2014/09/03 17:01:32, danakj wrote: > On 2014/08/27 23:53:39, enne wrote: > > Unfortunateness #2: Because activation is controlled via the scheduler, > there's > > no stack frame where I can block all post tasks. So, things like mailbox > > returning gets posted during commit. That means when DidActivateSyncTree is > > called, they've been posted but haven't run. So, CommitComplete can't be > called > > directly or it will happen before those tasks (incorrectly). To fix this > > ordering, I just PostTask CommitComplete here. Given that commit -> > activation > > -> DidActivateSyncTree have all happened synchronously, I think this is legit, > > but it's weird. > > > > I could potentially create a member variable blocked task runner and create it > > in commit and destroy it here, rather than the scoped version that exists > inside > > of commit today. Maybe? > > Ya, creating a CaptureTasks and holding it thru the activation would be more > performant. Done.
https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy.cc File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:424: layer_tree_host_impl_->active_tree()->SetRequiresHighResToDraw(); On 2014/09/03 19:41:28, enne wrote: > On 2014/09/03 17:01:32, danakj wrote: > > This is going to be bad when we are OOM? We will skip frames cuz of this flag > if > > any tile is a checkerboard. I'm not sure what to suggest tho.. or am I wrong > and > > this is okay? > > I think the discussion we had today about raster-on-demand (or alternatively > calling it like "I can't get a resource for this so consider it high res", but > in a more pithy way) covers this case. Hm, can you explain how? I can see how that would interact with activation. But when this flag is true, then PrepareToDraw returns DRAW_ABORTED_MISSING_STUFF which will cause frames to not be drawn?
+reveman,vmpstr for a question about raster on demand https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy.cc File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... cc/trees/single_thread_proxy.cc:424: layer_tree_host_impl_->active_tree()->SetRequiresHighResToDraw(); On 2014/09/03 19:49:30, danakj wrote: > On 2014/09/03 19:41:28, enne wrote: > > On 2014/09/03 17:01:32, danakj wrote: > > > This is going to be bad when we are OOM? We will skip frames cuz of this > flag > > if > > > any tile is a checkerboard. I'm not sure what to suggest tho.. or am I wrong > > and > > > this is okay? > > > > I think the discussion we had today about raster-on-demand (or alternatively > > calling it like "I can't get a resource for this so consider it high res", but > > in a more pithy way) covers this case. > > Hm, can you explain how? I can see how that would interact with activation. But > when this flag is true, then PrepareToDraw returns DRAW_ABORTED_MISSING_STUFF > which will cause frames to not be drawn? Looking at TileManager, we only set RasterOnDemand when the tile is required for activation. Should we be doing this for visible tiles as well? Also, this is not a new problem with this patch. This would already be a problem for OOMing pages since we set this flag when a tab goes invisible.
On 2014/09/03 20:02:48, enne wrote: > +reveman,vmpstr for a question about raster on demand > > https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy.cc > File cc/trees/single_thread_proxy.cc (right): > > https://codereview.chromium.org/508373002/diff/1/cc/trees/single_thread_proxy... > cc/trees/single_thread_proxy.cc:424: > layer_tree_host_impl_->active_tree()->SetRequiresHighResToDraw(); > On 2014/09/03 19:49:30, danakj wrote: > > On 2014/09/03 19:41:28, enne wrote: > > > On 2014/09/03 17:01:32, danakj wrote: > > > > This is going to be bad when we are OOM? We will skip frames cuz of this > > flag > > > if > > > > any tile is a checkerboard. I'm not sure what to suggest tho.. or am I > wrong > > > and > > > > this is okay? > > > > > > I think the discussion we had today about raster-on-demand (or alternatively > > > calling it like "I can't get a resource for this so consider it high res", > but > > > in a more pithy way) covers this case. > > > > Hm, can you explain how? I can see how that would interact with activation. > But > > when this flag is true, then PrepareToDraw returns DRAW_ABORTED_MISSING_STUFF > > which will cause frames to not be drawn? > > Looking at TileManager, we only set RasterOnDemand when the tile is required for > activation. Should we be doing this for visible tiles as well? +1 since shared tiles are not marked as required. > Also, this is not a new problem with this patch. This would already be a > problem for OOMing pages since we set this flag when a tab goes invisible. Ok, I can't tell how an OOM page ever draws right now when you switch tabs to it. Clearly this doesn't prevent the draws we think it should.
https://codereview.chromium.org/508373002/diff/40001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/508373002/diff/40001/cc/test/layer_tree_test.... cc/test/layer_tree_test.cc:205: CHECK(settings().impl_side_painting); nit: DCHECKs would be sufficient (and be a good example for others) https://codereview.chromium.org/508373002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/508373002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2225: SINGLE_AND_MULTI_THREAD_NOIMPL_TEST_F(LayerTreeHostTestLCDNotification); Why is this happening? https://codereview.chromium.org/508373002/diff/40001/cc/trees/single_thread_p... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/40001/cc/trees/single_thread_p... cc/trees/single_thread_proxy.cc:251: DCHECK(commit_blocking_task_runner_.get()); nit: don't need .get() to check scoped_ptr for NULL, you're probably thinking of scoped_refptr which you do for now, and will get cleaned up later https://codereview.chromium.org/508373002/diff/40001/cc/trees/single_thread_p... cc/trees/single_thread_proxy.cc:254: DebugScopedSetMainThread main(this); Can you do this before the reset? To indicate main thread is unblocked and running
https://codereview.chromium.org/508373002/diff/40001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/508373002/diff/40001/cc/test/layer_tree_test.... cc/test/layer_tree_test.cc:205: CHECK(settings().impl_side_painting); On 2014/09/03 21:14:24, danakj wrote: > nit: DCHECKs would be sufficient (and be a good example for others) It's a CHECK so that it also fails in release. If this were in gtest code, I would put an EXPECT or an ASSERT which would also fail in release. So, therefore CHECK. https://codereview.chromium.org/508373002/diff/40001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/508373002/diff/40001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:2225: SINGLE_AND_MULTI_THREAD_NOIMPL_TEST_F(LayerTreeHostTestLCDNotification); On 2014/09/03 21:14:24, danakj wrote: > Why is this happening? Fixed. OOPS. https://codereview.chromium.org/508373002/diff/40001/cc/trees/single_thread_p... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/40001/cc/trees/single_thread_p... cc/trees/single_thread_proxy.cc:251: DCHECK(commit_blocking_task_runner_.get()); On 2014/09/03 21:14:24, danakj wrote: > nit: don't need .get() to check scoped_ptr for NULL, you're probably thinking of > scoped_refptr which you do for now, and will get cleaned up later Done. https://codereview.chromium.org/508373002/diff/40001/cc/trees/single_thread_p... cc/trees/single_thread_proxy.cc:254: DebugScopedSetMainThread main(this); On 2014/09/03 21:14:24, danakj wrote: > Can you do this before the reset? To indicate main thread is unblocked and > running Done.
LGTM
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/508373002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/508373002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/508373002/60001
I ran these all locally and can't repro any of these failures or timeouts or crashes. I'm going to commit again and see if this was just a fluke.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/508373002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/508373002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by danakj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/508373002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as 7eee8e03582772716b820001af75343f6341e6e8
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/550073002/ by henrika@chromium.org. The reason for reverting is: LayerTreeHostContextTestDontUseLostResources.RunSingleThread_DirectRenderer_MainThreadPaint Fails on Win7 bot after this change. Speculative revert by sheriff. http://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%28....
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/41dbfb1a2668ebfc913cea247011f6e596c374f2 Cr-Commit-Position: refs/heads/master@{#293629}
danakj: PTAL This fixes two bugs (separated out into two patches if you want to diff them): (1) LayerTreeHostContextTestDontUseLostResources.RunSingleThread_DirectRenderer_MainThreadPaint (yes, MainThreadPaint) would flakily crash and this was the reason for the revert. The direct cause from the patch was the addition of timing_history_.DidActivateSyncTree() which did not happen previously. This would cause the scheduler to think it could make the deadline and throw another BeginFrame. The reason it crashed was because UpdateLayers would happen while there was a lost context, but only because I had squashed all of BeginMainFrame and Commit into Commit to avoid passing around a resource queue, thinking that it didn't matter because it was all single-threaded (haha, oops). Putting UpdateLayers inside BeginMainFrame where it should be fixes this problem entirely and is probably cleaner. (2) LayerTreeHostContextTestLostContextSucceedsWithContent.RunSingleThread_*Renderer_ImplSidePaint was now failing because PrepareToDraw wasn't returning DRAW_SUCCESS and resources weren't ready in DrawLayers. This is because in single-threaded impl-side painting mode, it activates immediately with a "high res required" flag, which can cause drawing to fail. By adding support for failing PrepareToDraw (and then not calling DrawLayers) in both SingleThreadProxy and the lost context tests, this now does the right thing.
https://codereview.chromium.org/508373002/diff/140001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/140001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:538: draw_result = layer_tree_host_impl_->PrepareToDraw(frame); Question: Isn't sync composite supposed to wait for activating until it's able to draw? Or, wait to draw until it's able to, so that we will in fact draw?
LGTM we don't need to synchronously composite with implside (yet)
CaptureScreenshot tests were failing because the previous patch had the synchronous path calling BeginMainFrame. This function contains a ScopedAbortRemainingSwapPromises and forces a commit via NotifyReadToCommit. This works to cause a commit for the scheduled case, but for the non-scheduled case it meant that all swap promises were aborted. Oops. Fixed by breaking up BeginMainFrame into BeginMainFrame (only for scheduled) and DoBeginMainFrame (to actually do the work for both). Since it was mildly related, I added a swap promise aborter for the synchronous path, which seemed missing.
LGTM https://codereview.chromium.org/508373002/diff/180001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/180001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:467: ScopedAbortRemainingSwapPromises swap_promise_checker(layer_tree_host_); do you think we should unit test this?
On 2014/10/02 17:15:54, danakj wrote: > do you think we should unit test this? PTAL https://codereview.chromium.org/508373002/diff/220001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/220001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:487: SwapPromise::SWAP_FAILS); Here's the one thing I'm not sure about. I could add a separate swap promise enum failure for DRAW_FAILS, but I don't think this is all that interesting external to cc. It might make the test better, but then ui::Event would have to handle this case, even though it's really about the same as the swap failing, in my mind. Is this good, or do you think it'd be worth making it more explicit?
https://codereview.chromium.org/508373002/diff/220001/cc/trees/single_thread_... File cc/trees/single_thread_proxy.cc (right): https://codereview.chromium.org/508373002/diff/220001/cc/trees/single_thread_... cc/trees/single_thread_proxy.cc:487: SwapPromise::SWAP_FAILS); On 2014/10/08 00:34:49, enne wrote: > Here's the one thing I'm not sure about. I could add a separate swap promise > enum failure for DRAW_FAILS, but I don't think this is all that interesting > external to cc. It might make the test better, but then ui::Event would have to > handle this case, even though it's really about the same as the swap failing, in > my mind. > > Is this good, or do you think it'd be worth making it more explicit? This is fine. The commit didn't abort, but the swap didn't happen. We use this in ~LayerTreeImpl also.
Thanks very LGTM https://codereview.chromium.org/508373002/diff/220001/cc/trees/layer_tree_hos... File cc/trees/layer_tree_host_unittest.cc (right): https://codereview.chromium.org/508373002/diff/220001/cc/trees/layer_tree_hos... cc/trees/layer_tree_host_unittest.cc:5109: // Fail to swap. add "(no damage)"
The CQ bit was checked by danakj@chromium.org
The CQ bit was unchecked by danakj@chromium.org
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/508373002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/508373002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/508373002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/98f3a6c5c30f1ecfc934838688ce265455a35ec9 Cr-Commit-Position: refs/heads/master@{#298960} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
