Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2659123004:
cc: Add scheduler support for invalidating content on impl thread. (Closed)
Descriptioncc: Add scheduler support for invalidating content on impl thread.
The change adds an API to the scheduler for requesting invalidations
to content on the impl thread. The scheduler makes best effort to merge
impl-side invalidations with the incoming main frame, if any, but may
decide to create a pending tree for these invalidations on the impl
thread.
BUG=686267
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2659123004
Cr-Commit-Position: refs/heads/master@{#452720}
Committed: https://chromium.googlesource.com/chromium/src/+/ab73d50c7514cdb1ed443d090c5b9d6a17d4b585
Patch Set 1 #Patch Set 2 : .. #
Total comments: 1
Patch Set 3 : commit_to_active #
Total comments: 7
Patch Set 4 : addressed comments. #
Total comments: 1
Patch Set 5 : tests #
Total comments: 24
Patch Set 6 : addressed comments #Patch Set 7 : moar tests #
Total comments: 8
Patch Set 8 : only inside deadline #
Total comments: 6
Patch Set 9 : spelling #
Total comments: 22
Patch Set 10 : addressed sunny's comments #Patch Set 11 : no DCHECK #
Messages
Total messages: 54 (15 generated)
Description was changed from ========== cc: Add scheduler support for invalidating content on impl thread. The change adds an API to the scheduler for requesting invalidations to content on the impl thread. The scheduler makes best effort to merge impl-side invalidations with the incoming main frame, if any, but may decide to create a pending tree for these invalidations on the impl thread. BUG=686267 ========== to ========== cc: Add scheduler support for invalidating content on impl thread. The change adds an API to the scheduler for requesting invalidations to content on the impl thread. The scheduler makes best effort to merge impl-side invalidations with the incoming main frame, if any, but may decide to create a pending tree for these invalidations on the impl thread. BUG=686267 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + brianderson@chromium.org
I'm going to add tests for it. Thought I'd send it to you to take a look in the meanwhile.
khushalsagar@chromium.org changed reviewers: + sunnyps@chromium.org
+sunnyps as well.
https://codereview.chromium.org/2659123004/diff/20001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/20001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:588: if (impl_side_invalidation_funnel_) May be this should get set after each commit, irrespective of whether there were impl-side invalidations. Otherwise if there is no main frame request, this will create a pending tree immediately after the current one is activated. And we would want to defer doing this until the next BeginFrame?
I accidentally added a WIP test change as well, please ignore that. pingy for a general review.
Not quite done with the review, but posting what I have. There are lots of corner cases to think about! https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:100: void SetNeedsImplSideInvalidation(); Would SetNeedsInvalidatePendingTree be a better name? "Impl side" could refer to pending or active or the frame sink. https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:100: void SetNeedsImplSideInvalidation(); Oh - I see that it invalidates the active tree too. What about SetNeedsNewImageTiles? https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:676: WillRunImplSideInvalidation(); I know it'll seem like extra boiler plate, but this is a little weird since the Will* methods are usually called by cc::Scheduler. Can you add a shared method that you call here and from WillRunImplSideInvalidation? https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.h:254: void WillRunImplSideInvalidation(); To be consistent, group this with the other Will* methods.
https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:100: void SetNeedsImplSideInvalidation(); On 2017/02/03 00:54:53, brianderson wrote: > Oh - I see that it invalidates the active tree too. > > What about SetNeedsNewImageTiles? ImplSideInvalidation seemed better to keep it generic for any feature that might want to invalidate visible content on the impl thread. In case there are potential use cases beyond checker-imaging. https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:676: WillRunImplSideInvalidation(); On 2017/02/03 00:54:53, brianderson wrote: > I know it'll seem like extra boiler plate, but this is a little weird since the > Will* methods are usually called by cc::Scheduler. Can you add a shared method > that you call here and from WillRunImplSideInvalidation? Done. https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/2659123004/diff/40001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.h:254: void WillRunImplSideInvalidation(); On 2017/02/03 00:54:53, brianderson wrote: > To be consistent, group this with the other Will* methods. Done. https://codereview.chromium.org/2659123004/diff/60001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/60001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:588: if (impl_side_invalidation_funnel_) One thought while thinking about the corner cases here. I think we basically want to do one pending tree per frame, coming from main or impl thread. And prefer to give priority to the main thread. So what happens when we get a BeginFrame, no main frame request so we create a pending tree for impl-side invalidation, and then get a main frame request. Should we defer sending BeginMainFrame until the next frame? Similarly if we got a commit and the impl-side invalidation request comes after it, handling it should be deferred until the next BeginFrame. The main frame aborted is one corner case, but may be this can be generalized. This function would always defer creating a pending tree if a main frame response is pending, we need to identify cases where we want to ignore this. Commits being aborted is one, another could be where the main thread is high latency.
I've added a bunch of tests for this in SchedulerStateMachineTest for all the cases I could think of. I've mostly looked at ShouldSendBeginMainFrame for possible cases and added what would be relevant in this case to ShouldPerformImplSideInvalidation. I'll add some more SchedulerTests as well. In the meanwhile it would be great to get some feedback on how this is looking.
naggy ping. :)
Left a few comments. Do you reckon we have cases where we perform an invalidation before BeginMainFrame/Commit in one frame interval? The invalidation will call UpdateDrawProperties and PrepareTiles both of which are expensive. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:630: client_->ScheduledActionPerformImplSideInvalidation(); break? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:48: virtual void ScheduledActionPerformImplSideInvalidation() = 0; nit/bikeshed: ScheduledActionInvalidateActiveTree? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:600: (active_tree_needs_first_draw_ || IsDrawThrottled())) { What does impl-side invalidation mean if commit_to_active_tree is true? Will the compositor create a pending tree in that case? I think invalidations should be disabled in the commit to active tree case. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:645: send_begin_main_frame_funnel_ = true; I'm kinda worried about this. It means there will be a one frame delay from requestAnimationFrame, if there's a pending invalidation. What's the difference between this and main frame before activation? With MFBA (enabled by default for renderer) we allow BeginMainFrame before the previous frame's activation. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:701: impl_side_invalidations_take_priority_ = true; nit: you can use last_commit_had_no_updates_ instead of this flag https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2139: // No impl-side invalidations should be performed while we are not visible. You should start a BeginFrame before testing invalidations, otherwise the funnel isn't set and invalidations will happen. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2153: state.SetBeginFrameSourcePaused(true); Same here. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2164: // Impl-side invalidations should not be triggered till the frame sink is Same here. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2337: state.BeginMainFrameAborted(CommitEarlyOutReason::FINISHED_NO_UPDATES); After aborting the commit send BeginMainFrame again so that we know invalidation takes priority over the commit. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2360: // impl-side invalidations. Can you explain what tree priority has to do with invalidations?
khushalsagar@chromium.org changed reviewers: + enne@chromium.org
+enne for the commit_to_active_tree mode question. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:630: client_->ScheduledActionPerformImplSideInvalidation(); On 2017/02/14 21:40:18, sunnyps wrote: > break? Woops. Let me add add the Scheduler tests also. :) https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:48: virtual void ScheduledActionPerformImplSideInvalidation() = 0; On 2017/02/14 21:40:18, sunnyps wrote: > nit/bikeshed: ScheduledActionInvalidateActiveTree? Hmmm, given that in the default case we actually create a pending tree to invalidate on the active tree, I don't know if this makes it any clearer... https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:600: (active_tree_needs_first_draw_ || IsDrawThrottled())) { On 2017/02/14 21:40:18, sunnyps wrote: > What does impl-side invalidation mean if commit_to_active_tree is true? Will the > compositor create a pending tree in that case? I think invalidations should be > disabled in the commit to active tree case. It would mean we invalidate the tiles on the active tree. It can work in that mode too since we checker-image on the active tree instead of checker-boarding. And once the images are decoded, we still need the scheduling here to pipeline the invalidations for them on the active tree. Given that the commit_to_active_tree mode is only used in the UI, I don't know whether we would actually use impl-side invalidations in that mode, but I think enne's suggestions on my initial patch for this hinted that we should set up the code such that it could be done. +enne, wdyt? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:645: send_begin_main_frame_funnel_ = true; On 2017/02/14 21:40:18, sunnyps wrote: > I'm kinda worried about this. It means there will be a one frame delay from > requestAnimationFrame, if there's a pending invalidation. What's the difference > between this and main frame before activation? With MFBA (enabled by default for > renderer) we allow BeginMainFrame before the previous frame's activation. If there is rAF alongside a pending invalidation, then in most cases the BeginMainFrame will get priority over the invalidation request. This will only come into play in the case where at the beginning of BeginFrame we only had an invalidation request, and got a BeginMainFrame *after* we went ahead with that invalidation. In which case sending it will be delayed until the next frame. MFBA is a good point. May be its better to avoid adding this then, since if we do end up blocked with the draw, BeginMainFrame will be throttled later anyway (ShouldSendBeginMainFrame takes in a lot of factors to handle this case)? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:701: impl_side_invalidations_take_priority_ = true; On 2017/02/14 21:40:18, sunnyps wrote: > nit: you can use last_commit_had_no_updates_ instead of this flag That flag gets reset on every BeginImplFrame, but I want this one to stick till the impl-side invalidations are run. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2139: // No impl-side invalidations should be performed while we are not visible. On 2017/02/14 21:40:18, sunnyps wrote: > You should start a BeginFrame before testing invalidations, otherwise the funnel > isn't set and invalidations will happen. I'm not sure I followed this. The |impl_side_invalidation_funnel_| is initialized as false, and I want to test that in that state even if we could invalidate, the action is not performed when we are invisible. Why is it necessary to do a BeginFrame for that? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2337: state.BeginMainFrameAborted(CommitEarlyOutReason::FINISHED_NO_UPDATES); On 2017/02/14 21:40:18, sunnyps wrote: > After aborting the commit send BeginMainFrame again so that we know invalidation > takes priority over the commit. Done. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2360: // impl-side invalidations. On 2017/02/14 21:40:19, sunnyps wrote: > Can you explain what tree priority has to do with invalidations? This was just to get a case for: if (CouldSendBeginMainFrame() && begin_main_frame_state_ == BeginMainFrameState::BEGIN_MAIN_FRAME_STATE_IDLE) return false; If we are throttling main frames for any reason while a request is still pending, the impl-side invalidations should continue waiting on the main frame.
https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:600: (active_tree_needs_first_draw_ || IsDrawThrottled())) { On 2017/02/15 at 00:56:33, Khushal wrote: > On 2017/02/14 21:40:18, sunnyps wrote: > > What does impl-side invalidation mean if commit_to_active_tree is true? Will the > > compositor create a pending tree in that case? I think invalidations should be > > disabled in the commit to active tree case. > > It would mean we invalidate the tiles on the active tree. It can work in that mode too since we checker-image on the active tree instead of checker-boarding. And once the images are decoded, we still need the scheduling here to pipeline the invalidations for them on the active tree. > > Given that the commit_to_active_tree mode is only used in the UI, I don't know whether we would actually use impl-side invalidations in that mode, but I think enne's suggestions on my initial patch for this hinted that we should set up the code such that it could be done. > +enne, wdyt? Yeah, that was my thought as well. It seems like invalidations could just be applied to the active tree directly. We definitely shouldn't create a pending tree for that case. It's maybe a tiny bit of extra code, but I think my general preference is to not have mode-specific features if it's easy to avoid, just because it's more special knowledge to remember.
And SchedulerTests added. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:645: send_begin_main_frame_funnel_ = true; On 2017/02/15 00:56:33, Khushal wrote: > On 2017/02/14 21:40:18, sunnyps wrote: > > I'm kinda worried about this. It means there will be a one frame delay from > > requestAnimationFrame, if there's a pending invalidation. What's the > difference > > between this and main frame before activation? With MFBA (enabled by default > for > > renderer) we allow BeginMainFrame before the previous frame's activation. > > If there is rAF alongside a pending invalidation, then in most cases the > BeginMainFrame will get priority over the invalidation request. This will only > come into play in the case where at the beginning of BeginFrame we only had an > invalidation request, and got a BeginMainFrame *after* we went ahead with that > invalidation. In which case sending it will be delayed until the next frame. > > MFBA is a good point. May be its better to avoid adding this then, since if we > do end up blocked with the draw, BeginMainFrame will be throttled later anyway > (ShouldSendBeginMainFrame takes in a lot of factors to handle this case)? Okay. After offline discussion, removed this. We want to not delay sending the main frame in this case. If there is back pressure from receiving a commit sooner, then the code later will take care of throttling frames if need be. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2337: state.BeginMainFrameAborted(CommitEarlyOutReason::FINISHED_NO_UPDATES); On 2017/02/15 00:56:33, Khushal wrote: > On 2017/02/14 21:40:18, sunnyps wrote: > > After aborting the commit send BeginMainFrame again so that we know > invalidation > > takes priority over the commit. > > Done. Actually, while adding the SchedulerTest for this, I realized that right after BeginMainFrameAborted, the next action will be to perform impl-side invalidations because the invalidation funnel will be empty for this frame. So I we won't actually send the main frame until the pending tree is activated (non MFBA setting). The fact that the next action is to perform invalidations while a main frame request is pending verifies that it is being prioritized over the main frame.
The CQ bit was checked by khushalsagar@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: This issue passed the CQ dry run.
more naggy ping.
I think we should scope the invalidation to only happen after the frame is over, in the deadline. We do the same thing for PrepareTiles because we don't want to issue PrepareTiles calls if there are continuous commits. I think the same argument applies to invalidations as well. If you have continuous commits coming in, then you shouldn't perform invalidations. However, once the commits stop, because we don't need begin main frames or aborted commit, then we can do the invalidation. We also want to ensure that we don't do both invalidation and prepare tiles in the same frame. https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:600: (active_tree_needs_first_draw_ || IsDrawThrottled())) { On 2017/02/15 01:09:28, enne wrote: > On 2017/02/15 at 00:56:33, Khushal wrote: > > On 2017/02/14 21:40:18, sunnyps wrote: > > > What does impl-side invalidation mean if commit_to_active_tree is true? Will > the > > > compositor create a pending tree in that case? I think invalidations should > be > > > disabled in the commit to active tree case. > > > > It would mean we invalidate the tiles on the active tree. It can work in that > mode too since we checker-image on the active tree instead of checker-boarding. > And once the images are decoded, we still need the scheduling here to pipeline > the invalidations for them on the active tree. > > > > Given that the commit_to_active_tree mode is only used in the UI, I don't know > whether we would actually use impl-side invalidations in that mode, but I think > enne's suggestions on my initial patch for this hinted that we should set up the > code such that it could be done. > > +enne, wdyt? > > Yeah, that was my thought as well. It seems like invalidations could just be > applied to the active tree directly. We definitely shouldn't create a pending > tree for that case. > > It's maybe a tiny bit of extra code, but I think my general preference is to not > have mode-specific features if it's easy to avoid, just because it's more > special knowledge to remember. I'm still confused about how active tree tiles will be replaced once the image decodes complete. The mechanism for doing that is by creating a pending tree and activating that. How do we replace active tree tiles except in a commit? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine_unittest.cc:2139: // No impl-side invalidations should be performed while we are not visible. On 2017/02/15 00:56:33, Khushal wrote: > On 2017/02/14 21:40:18, sunnyps wrote: > > You should start a BeginFrame before testing invalidations, otherwise the > funnel > > isn't set and invalidations will happen. > > I'm not sure I followed this. The |impl_side_invalidation_funnel_| is > initialized as false, and I want to test that in that state even if we could > invalidate, the action is not performed when we are invisible. Why is it > necessary to do a BeginFrame for that? Ah, I assumed that the funnel is initialized to true. I do think we want to scope the invalidation though and not let it happen at any time. https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:639: has_pending_tree_ = true; For UI, has_pending_tree_ = true should never be true except between SetNeedsCommit and ScheduledActionCommit. https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:3189: TEST_F(SchedulerTest, ImplSideInvalidationsMergedWithCommit) { I don't understand what this is testing. If SetNeedsImplSideInvalidation was a nop, this test would still pass.
I'm not sure I'm following your train of thought. So laying down a few things just to make sure we're on the same page. There are 2 ways the impl-side invalidation can happen: 1) Commit: We have a commit which creates the pending tree and may already have an invalidation from the main thread, so the impl thread merges its invalidation with it. This is the case we'll hit if there are continuous commits. Are you suggesting we shouldn't perform impl-side invalidations in this case? Why? That could perpetually stall swapping the image in the frame. 2) Impl-side pending tree: This is the case where instead of creating a pending tree for commit, we create one for these invalidations. We hit this case if there is no main frame request on BeginFrame, or we know commits are getting aborted. Again, that can happen a lot. Continuous scrolling that doesn't change the interest rect will keep aborting commits. Same goes for rAF. Aside from the latency in swapping the image in the frame, note that we also have to keep the image locked till it is swapped. And for not doing PrepareTiles if there are continuous commits, I don't have much knowledge about it but I would imagine that's because a PrepareTiles is done each time there is a commit anyway (https://cs.chromium.org/chromium/src/cc/trees/layer_tree_host_impl.cc?l=393). I'm not understanding the argument and parallel being made here. Do you mind elaborating? https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/80001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:600: (active_tree_needs_first_draw_ || IsDrawThrottled())) { On 2017/02/20 23:19:22, sunnyps wrote: > On 2017/02/15 01:09:28, enne wrote: > > On 2017/02/15 at 00:56:33, Khushal wrote: > > > On 2017/02/14 21:40:18, sunnyps wrote: > > > > What does impl-side invalidation mean if commit_to_active_tree is true? > Will > > the > > > > compositor create a pending tree in that case? I think invalidations > should > > be > > > > disabled in the commit to active tree case. > > > > > > It would mean we invalidate the tiles on the active tree. It can work in > that > > mode too since we checker-image on the active tree instead of > checker-boarding. > > And once the images are decoded, we still need the scheduling here to pipeline > > the invalidations for them on the active tree. > > > > > > Given that the commit_to_active_tree mode is only used in the UI, I don't > know > > whether we would actually use impl-side invalidations in that mode, but I > think > > enne's suggestions on my initial patch for this hinted that we should set up > the > > code such that it could be done. > > > +enne, wdyt? > > > > Yeah, that was my thought as well. It seems like invalidations could just be > > applied to the active tree directly. We definitely shouldn't create a pending > > tree for that case. > > > > It's maybe a tiny bit of extra code, but I think my general preference is to > not > > have mode-specific features if it's easy to avoid, just because it's more > > special knowledge to remember. > > I'm still confused about how active tree tiles will be replaced once the image > decodes complete. The mechanism for doing that is by creating a pending tree and > activating that. How do we replace active tree tiles except in a commit? The impl-side invalidation works exactly like a commit for single-threaded case. In a commit, the invalidations from the main thread invalidate tiles on the active tree, and we wait for them to be prepared (ready to draw signal) before drawing the active tree. This would work the same way, except the invalidation will be for images for which the decode has been completed. We don't need to create a pending tree, the same way we don't need to create it when the main thread invalidates content. https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:639: has_pending_tree_ = true; On 2017/02/20 23:19:22, sunnyps wrote: > For UI, has_pending_tree_ = true should never be true except between > SetNeedsCommit and ScheduledActionCommit. Good point. The impl-side invalidation should be done similar to a commit in SingleThreadProxy. In ScheduledActionPerformImplSideInvalidation, right after we invalidate the sync tree, it should immediately call NotifyReadyToActivate to update the scheduler state correctly. https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:3189: TEST_F(SchedulerTest, ImplSideInvalidationsMergedWithCommit) { On 2017/02/20 23:19:22, sunnyps wrote: > I don't understand what this is testing. If SetNeedsImplSideInvalidation was a > nop, this test would still pass. Its testing that if there is a main frame request, then an impl-side invalidation request *should* be a no-op, since they should get merged with the commit and we shouldn't see any other actions. The only action should be the commit. Do you have a suggestion on re-structuring the test for this, or do you think its unnecessary?
https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:623: BeginMainFrameState::BEGIN_MAIN_FRAME_STATE_IDLE) Given the preceding logic, I think the STATE_IDLE check can become a DCHECK. Probably put the DCHECK within the if to maintain the relationship even though it'll be true before the if. https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2134: TEST(SchedulerStateMachineTest, NoImplSideInvalidationsWhileInvsible) { Invsible -> Invisible
Following up on the discussion we had earlier about only running impl-side invalidations when we are inside the deadline. The rationale being that we want to give the main thread a chance to respond before creating an impl-side pending tree. If the commit is aborted the deadline will be triggered immediately, otherwise if the main thread fails to respond within the deadline, then also we take care of handling these invalidations in the deadline. While this should handle all cases. One thing to note is that if we invalidate inside the deadline and any required for activation tiles are invalidated, this will defer drawing it until the next frame. Because draw happens only inside the deadline, unless activation of the pending tree happens synchronously after the invalidation, drawing this pending tree will always have to wait until the next frame. I don't think its a problem right now, but something to revisit once we start using impl-side invalidations for animations. This also means on the next BeginFrame we will have |active_tree_needs_first_draw_| always set which will set |main_thread_missed_last_deadline_|. Does that affect anything?
On 2017/02/22 00:23:13, Khushal wrote: > Following up on the discussion we had earlier about only running impl-side > invalidations when we are inside the deadline. The rationale being that we want > to give the main thread a chance to respond before creating an impl-side pending > tree. If the commit is aborted the deadline will be triggered immediately, > otherwise if the main thread fails to respond within the deadline, then also we > take care of handling these invalidations in the deadline. I think we also want to avoid the invalidation if a commit is pending. Impl-side invalidations should only happen after continuous commits have stopped. > > While this should handle all cases. One thing to note is that if we invalidate > inside the deadline and any required for activation tiles are invalidated, this > will defer drawing it until the next frame. Because draw happens only inside the > deadline, unless activation of the pending tree happens synchronously after the > invalidation, drawing this pending tree will always have to wait until the next > frame. I don't think its a problem right now, but something to revisit once we > start using impl-side invalidations for animations. This sounds OK. enne@ WDYT? Maybe ask vmpstr@ too. > > This also means on the next BeginFrame we will have > |active_tree_needs_first_draw_| always set which will set > |main_thread_missed_last_deadline_|. Does that affect anything? active_tree_needs_first_draw_ will delay draws until the invalidation pending tree activates. main_thread_missed_last_deadline_ seems to affect tracing only.
On 2017/02/22 00:44:19, sunnyps wrote: > On 2017/02/22 00:23:13, Khushal wrote: > > Following up on the discussion we had earlier about only running impl-side > > invalidations when we are inside the deadline. The rationale being that we > want > > to give the main thread a chance to respond before creating an impl-side > pending > > tree. If the commit is aborted the deadline will be triggered immediately, > > otherwise if the main thread fails to respond within the deadline, then also > we > > take care of handling these invalidations in the deadline. > > I think we also want to avoid the invalidation if a commit is pending. Impl-side > invalidations should only happen after continuous commits have stopped. We were already not doing invalidations if commits are happening, because we just get the pending tree from the main frame. I think this was about handling main frames being aborted and stuff like that. Doing it inside the deadline will just take care of all of them. If a commit is aborted, the deadline is triggered immediately. If the main thread does not respond before the deadline is triggered, then we'll just end up doing the impl-side invalidation inside the deadline. > > > > > While this should handle all cases. One thing to note is that if we invalidate > > inside the deadline and any required for activation tiles are invalidated, > this > > will defer drawing it until the next frame. Because draw happens only inside > the > > deadline, unless activation of the pending tree happens synchronously after > the > > invalidation, drawing this pending tree will always have to wait until the > next > > frame. I don't think its a problem right now, but something to revisit once we > > start using impl-side invalidations for animations. > > This sounds OK. enne@ WDYT? Maybe ask vmpstr@ too. > > > > > This also means on the next BeginFrame we will have > > |active_tree_needs_first_draw_| always set which will set > > |main_thread_missed_last_deadline_|. Does that affect anything? > > active_tree_needs_first_draw_ will delay draws until the invalidation pending > tree activates. main_thread_missed_last_deadline_ seems to affect tracing only.
Description was changed from ========== cc: Add scheduler support for invalidating content on impl thread. The change adds an API to the scheduler for requesting invalidations to content on the impl thread. The scheduler makes best effort to merge impl-side invalidations with the incoming main frame, if any, but may decide to create a pending tree for these invalidations on the impl thread. BUG=686267 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Add scheduler support for invalidating content on impl thread. The change adds an API to the scheduler for requesting invalidations to content on the impl thread. The scheduler makes best effort to merge impl-side invalidations with the incoming main frame, if any, but may decide to create a pending tree for these invalidations on the impl thread. BUG=686267 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
khushalsagar@chromium.org changed reviewers: + vmpstr@chromium.org
I have moved the impl side invalidation action to happen only inside the deadline. Its fine for now, we can come back to it if its a problem. PrepareTiles should also now wait for this invalidation, if there is a request pending, so we don't end up doing it twice in the frame. https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.h (right): https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.h:366: bool previous_pending_tree_was_impl_side_; Added this after I ran into a DCHECK when trying this out on a page. The SchedulerTest should also catch this now.
https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:554: !state_machine_.previous_pending_tree_was_impl_side(); Is there any way to avoid setting active_tree_needs_first_draw in the first place, rather than canceling it out after the fact?
https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:554: !state_machine_.previous_pending_tree_was_impl_side(); On 2017/02/22 19:31:20, brianderson wrote: > Is there any way to avoid setting active_tree_needs_first_draw in the first > place, rather than canceling it out after the fact? I think we would want to set |active_tree_needs_first_draw_| to make sure that this pending tree is pipelined similar to main frames, i.e., don't activate a new tree until the previous one is drawn.
https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:623: BeginMainFrameState::BEGIN_MAIN_FRAME_STATE_IDLE) On 2017/02/21 23:34:59, brianderson wrote: > Given the preceding logic, I think the STATE_IDLE check can become a DCHECK. > Probably put the DCHECK within the if to maintain the relationship even though > it'll be true before the if. This code went away in the new patch, now that we just do it inside the deadline, so the main frame state can be ignored. https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/120001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2134: TEST(SchedulerStateMachineTest, NoImplSideInvalidationsWhileInvsible) { On 2017/02/21 23:34:59, brianderson wrote: > Invsible -> Invisible Done.
https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:554: !state_machine_.previous_pending_tree_was_impl_side(); On 2017/02/22 20:19:29, Khushal wrote: > On 2017/02/22 19:31:20, brianderson wrote: > > Is there any way to avoid setting active_tree_needs_first_draw in the first > > place, rather than canceling it out after the fact? > > I think we would want to set |active_tree_needs_first_draw_| to make sure that > this pending tree is pipelined similar to main frames, i.e., don't activate a > new tree until the previous one is drawn. I think overwriting the active tree in this case might actually be desired to reduce latency. When a commit comes in while there's still a pending tree, the two are merged. Would the same thing happen (merging) if the pending tree is activated when previous_pending_tree_was_impl_side is true?
https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:554: !state_machine_.previous_pending_tree_was_impl_side(); On 2017/02/22 22:58:08, brianderson wrote: > On 2017/02/22 20:19:29, Khushal wrote: > > On 2017/02/22 19:31:20, brianderson wrote: > > > Is there any way to avoid setting active_tree_needs_first_draw in the first > > > place, rather than canceling it out after the fact? > > > > I think we would want to set |active_tree_needs_first_draw_| to make sure that > > this pending tree is pipelined similar to main frames, i.e., don't activate a > > new tree until the previous one is drawn. > > I think overwriting the active tree in this case might actually be desired to > reduce latency. When a commit comes in while there's still a pending tree, the "When a commit comes in while there's still a pending tree, the two are merged", do you mean that happens right now? Afaict, if a commit comes in while there's still a pending tree, the commit has to wait till the pending tree is activated. With impl-side invalidations, we defer creating a pending tree if we think a commit is coming, so we can merge invalidations from both sides on the same tree. But once created, it is pipelined the same way that a pending tree from the main thread would have been. I was more thinking about the case where impl-side invalidations are being used for an animated gif, in which case you don't want to keep overwriting the active tree and skipping frames. > two are merged. Would the same thing happen (merging) if the pending tree is > activated when previous_pending_tree_was_impl_side is true?
lgtm https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/2659123004/diff/140001/cc/scheduler/scheduler... cc/scheduler/scheduler.cc:554: !state_machine_.previous_pending_tree_was_impl_side(); On 2017/02/22 23:20:24, Khushal wrote: > On 2017/02/22 22:58:08, brianderson wrote: > > On 2017/02/22 20:19:29, Khushal wrote: > > > On 2017/02/22 19:31:20, brianderson wrote: > > > > Is there any way to avoid setting active_tree_needs_first_draw in the > first > > > > place, rather than canceling it out after the fact? > > > > > > I think we would want to set |active_tree_needs_first_draw_| to make sure > that > > > this pending tree is pipelined similar to main frames, i.e., don't activate > a > > > new tree until the previous one is drawn. > > > > I think overwriting the active tree in this case might actually be desired to > > reduce latency. When a commit comes in while there's still a pending tree, the > > "When a commit comes in while there's still a pending tree, the > two are merged", do you mean that happens right now? Afaict, if a commit comes > in while there's still a pending tree, the commit has to wait till the pending > tree is activated. > > With impl-side invalidations, we defer creating a pending tree if we think a > commit is coming, so we can merge invalidations from both sides on the same > tree. But once created, it is pipelined the same way that a pending tree from > the main thread would have been. > > I was more thinking about the case where impl-side invalidations are being used > for an animated gif, in which case you don't want to keep overwriting the active > tree and skipping frames. > > > two are merged. Would the same thing happen (merging) if the pending tree is > > activated when previous_pending_tree_was_impl_side is true? I see. I completely misunderstood the merge mechanics. That makes sense now, especially in the context of the animated gif example.
Thanks Brian! I'll wait for sunnyps@ to take a look as well before landing.
Left a few comments mostly about adding DCHECKs and tests. Almost there :) https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:534: if (needs_impl_side_invalidation_) Impl side invalidation will happen before prepare tiles. The invalidation will do a prepare tiles and fill the prepare tiles funnel. So this should be a DCHECK. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:618: prepare_tiles_funnel_ == 0) nit: braces https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:635: has_pending_tree_ = true; For invalidating active tree (commit_to_active_tree_ = true), the scheduler must be sent the activation signal (like in commit) or has_pending_tree_ must be false. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2222: // till the current pending tree is activated. Can you trigger the deadline here and activate in the next frame? The invalidation won't happen until deadline so there's no point checking here. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2243: TEST(SchedulerStateMachineTest, ImplSideInvalidationWhileMainFramePending) { nit: WhileReadyToCommit? https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2259: EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE); We should check for invalidation in the deadline. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2331: // Ack the previous frame and begin impl frame, which shoul perrform the nit: should perform https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2355: EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE); Check for invalidation in deadline. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:3189: TEST_F(SchedulerTest, ImplSideInvalidationsMergedWithCommit) { Please add a basic test for just invalidation in the deadline. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:3208: EXPECT_FALSE(scheduler_->needs_impl_side_invalidation()); Can you run the deadline and check that no invalidation gets run?
Done. Sorry, rebase crept in there too. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:534: if (needs_impl_side_invalidation_) On 2017/02/23 00:33:21, sunnyps wrote: > Impl side invalidation will happen before prepare tiles. The invalidation will > do a prepare tiles and fill the prepare tiles funnel. So this should be a > DCHECK. Done. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:618: prepare_tiles_funnel_ == 0) On 2017/02/23 00:33:21, sunnyps wrote: > nit: braces Done. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:635: has_pending_tree_ = true; On 2017/02/23 00:33:21, sunnyps wrote: > For invalidating active tree (commit_to_active_tree_ = true), the scheduler must > be sent the activation signal (like in commit) or has_pending_tree_ must be > false. Yup, sending the activation signal will happen in SingleThreadProxy. That's in my next patch that glues together the scheduler and LTHI changes. It synchronously notifies ready to activate right after invalidating. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2222: // till the current pending tree is activated. On 2017/02/23 00:33:21, sunnyps wrote: > Can you trigger the deadline here and activate in the next frame? The > invalidation won't happen until deadline so there's no point checking here. Hmmm, in fact I wanted to activate within this frame and then trigger the deadline to make sure that the invalidation doesn't happen if the deadline is triggered while there is no pending tree, just because the commit filled the invalidation funnel also. If I trigger the deadline before activating, then we know invalidations won't happen simply because we still have a pending tree. We trigger the deadline and then check that invalidations don't happen in #2233. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2243: TEST(SchedulerStateMachineTest, ImplSideInvalidationWhileMainFramePending) { On 2017/02/23 00:33:21, sunnyps wrote: > nit: WhileReadyToCommit? Done. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2259: EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE); On 2017/02/23 00:33:21, sunnyps wrote: > We should check for invalidation in the deadline. Done. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2331: // Ack the previous frame and begin impl frame, which shoul perrform the On 2017/02/23 00:33:21, sunnyps wrote: > nit: should perform Done. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine_unittest.cc:2355: EXPECT_ACTION_UPDATE_STATE(SchedulerStateMachine::ACTION_NONE); On 2017/02/23 00:33:21, sunnyps wrote: > Check for invalidation in deadline. Actually, I don't know if the test makes sense anymore. The invalidations will be performed once we're in the deadline, irrespective of what the main frame state is. Its a Scheduler detail that the deadline is triggered immediately if the main frame is aborted, and the SchedulerTest should take care of that. I updated this to make sure that the impl-side invalidation runs during the deadline, even if a main frame request is present. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:3189: TEST_F(SchedulerTest, ImplSideInvalidationsMergedWithCommit) { On 2017/02/23 00:33:21, sunnyps wrote: > Please add a basic test for just invalidation in the deadline. Done. https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_unittest.cc:3208: EXPECT_FALSE(scheduler_->needs_impl_side_invalidation()); On 2017/02/23 00:33:21, sunnyps wrote: > Can you run the deadline and check that no invalidation gets run? Done.
lgtm % nit https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:534: if (needs_impl_side_invalidation_) On 2017/02/23 01:25:43, Khushal wrote: > On 2017/02/23 00:33:21, sunnyps wrote: > > Impl side invalidation will happen before prepare tiles. The invalidation will > > do a prepare tiles and fill the prepare tiles funnel. So this should be a > > DCHECK. > > Done. nit: Try moving this DCHECK to WillPrepareTiles. This may not be possible as WillPrepareTiles is called from outside the scheduler too.
https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... cc/scheduler/scheduler_state_machine.cc:534: if (needs_impl_side_invalidation_) On 2017/02/23 01:34:12, sunnyps wrote: > On 2017/02/23 01:25:43, Khushal wrote: > > On 2017/02/23 00:33:21, sunnyps wrote: > > > Impl side invalidation will happen before prepare tiles. The invalidation > will > > > do a prepare tiles and fill the prepare tiles funnel. So this should be a > > > DCHECK. > > > > Done. > > nit: Try moving this DCHECK to WillPrepareTiles. This may not be possible as > WillPrepareTiles is called from outside the scheduler too. Actually on second look, I don't think this logic/DCHECK is correct. We could have a case where commit happens, but it doesn't necessarily run a PrepareTiles (so the |prepare_tiles_funnel_| is empty). It still fills the |impl_side_invalidation_funnel_| so if we get an impl side invalidation request after this, it will stay till the next frame. Now something could still request a PrepareTiles still in the same frame, so we have |needs_impl_side_invalidation_| and |needs_prepare_tiles_|, but PrepareTiles can run this frame but the invalidation can not. There is also the case where calling ShouldPerformImplSideInvalidation requests another invalidation but running it doesn't do PrepareTiles. I think having the test to ensure we call ShouldPerformImplSideInvalidation before ShouldPrepareTiles is enough. We don't need anything else here. Does that sound right?
On 2017/02/23 07:42:37, Khushal wrote: > https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... > File cc/scheduler/scheduler_state_machine.cc (right): > > https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... > cc/scheduler/scheduler_state_machine.cc:534: if (needs_impl_side_invalidation_) > On 2017/02/23 01:34:12, sunnyps wrote: > > On 2017/02/23 01:25:43, Khushal wrote: > > > On 2017/02/23 00:33:21, sunnyps wrote: > > > > Impl side invalidation will happen before prepare tiles. The invalidation > > will > > > > do a prepare tiles and fill the prepare tiles funnel. So this should be a > > > > DCHECK. > > > > > > Done. > > > > nit: Try moving this DCHECK to WillPrepareTiles. This may not be possible as > > WillPrepareTiles is called from outside the scheduler too. > > Actually on second look, I don't think this logic/DCHECK is correct. We could > have a case where commit happens, but it doesn't necessarily run a PrepareTiles > (so the |prepare_tiles_funnel_| is empty). It still fills the > |impl_side_invalidation_funnel_| so if we get an impl side invalidation request > after this, it will stay till the next frame. Now something could still request > a PrepareTiles still in the same frame, so we have > |needs_impl_side_invalidation_| and |needs_prepare_tiles_|, but PrepareTiles can > run this frame but the invalidation can not. I don't think this happens in practice because if a commit merges the impl-side invalidation it will have to do a PrepareTiles, right? Unless both commit and (merged) invalidation don't dirty any tile priorities. In that case commit won't do PrepareTiles, but scheduler might do PrepareTiles if tile priorities get dirtied afterwards. > > There is also the case where calling ShouldPerformImplSideInvalidation requests > another invalidation but running it doesn't do PrepareTiles. > > I think having the test to ensure we call ShouldPerformImplSideInvalidation > before ShouldPrepareTiles is enough. We don't need anything else here. Does that > sound right? It's OK to leave the DCHECK out for now. lgtm
On 2017/02/23 19:53:05, sunnyps wrote: > On 2017/02/23 07:42:37, Khushal wrote: > > > https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... > > File cc/scheduler/scheduler_state_machine.cc (right): > > > > > https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... > > cc/scheduler/scheduler_state_machine.cc:534: if > (needs_impl_side_invalidation_) > > On 2017/02/23 01:34:12, sunnyps wrote: > > > On 2017/02/23 01:25:43, Khushal wrote: > > > > On 2017/02/23 00:33:21, sunnyps wrote: > > > > > Impl side invalidation will happen before prepare tiles. The > invalidation > > > will > > > > > do a prepare tiles and fill the prepare tiles funnel. So this should be > a > > > > > DCHECK. > > > > > > > > Done. > > > > > > nit: Try moving this DCHECK to WillPrepareTiles. This may not be possible as > > > WillPrepareTiles is called from outside the scheduler too. > > > > Actually on second look, I don't think this logic/DCHECK is correct. We could > > have a case where commit happens, but it doesn't necessarily run a > PrepareTiles > > (so the |prepare_tiles_funnel_| is empty). It still fills the > > |impl_side_invalidation_funnel_| so if we get an impl side invalidation > request > > after this, it will stay till the next frame. Now something could still > request > > a PrepareTiles still in the same frame, so we have > > |needs_impl_side_invalidation_| and |needs_prepare_tiles_|, but PrepareTiles > can > > run this frame but the invalidation can not. > > I don't think this happens in practice because if a commit merges the impl-side > invalidation it will have to do a PrepareTiles, right? Unless both commit and > (merged) invalidation don't dirty any tile priorities. In that case commit won't > do PrepareTiles, but scheduler might do PrepareTiles if tile priorities get > dirtied afterwards. We are talking about the case where the commit doesn't merge impl-side invalidation, because there is no request for it when the commit happens, and the commit doesn't do PrepareTiles. But there can be an impl-side invalidation request and prepare tiles request between the commit and inside the deadline. So the DCHECK should be: DCHECK(!needs_prepare_tiles_ || !needs_impl_side_invalidation_ || impl_side_invalidation_funnel_); If we are doing PrepareTiles over invalidation, then the invalidation funnel should be full. > > > > > There is also the case where calling ShouldPerformImplSideInvalidation > requests > > another invalidation but running it doesn't do PrepareTiles. > > > > I think having the test to ensure we call ShouldPerformImplSideInvalidation > > before ShouldPrepareTiles is enough. We don't need anything else here. Does > that > > sound right? > > It's OK to leave the DCHECK out for now. lgtm
On 2017/02/23 20:43:04, Khushal wrote: > On 2017/02/23 19:53:05, sunnyps wrote: > > On 2017/02/23 07:42:37, Khushal wrote: > > > > > > https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... > > > File cc/scheduler/scheduler_state_machine.cc (right): > > > > > > > > > https://codereview.chromium.org/2659123004/diff/160001/cc/scheduler/scheduler... > > > cc/scheduler/scheduler_state_machine.cc:534: if > > (needs_impl_side_invalidation_) > > > On 2017/02/23 01:34:12, sunnyps wrote: > > > > On 2017/02/23 01:25:43, Khushal wrote: > > > > > On 2017/02/23 00:33:21, sunnyps wrote: > > > > > > Impl side invalidation will happen before prepare tiles. The > > invalidation > > > > will > > > > > > do a prepare tiles and fill the prepare tiles funnel. So this should > be > > a > > > > > > DCHECK. > > > > > > > > > > Done. > > > > > > > > nit: Try moving this DCHECK to WillPrepareTiles. This may not be possible > as > > > > WillPrepareTiles is called from outside the scheduler too. > > > > > > Actually on second look, I don't think this logic/DCHECK is correct. We > could > > > have a case where commit happens, but it doesn't necessarily run a > > PrepareTiles > > > (so the |prepare_tiles_funnel_| is empty). It still fills the > > > |impl_side_invalidation_funnel_| so if we get an impl side invalidation > > request > > > after this, it will stay till the next frame. Now something could still > > request > > > a PrepareTiles still in the same frame, so we have > > > |needs_impl_side_invalidation_| and |needs_prepare_tiles_|, but PrepareTiles > > can > > > run this frame but the invalidation can not. > > > > I don't think this happens in practice because if a commit merges the > impl-side > > invalidation it will have to do a PrepareTiles, right? Unless both commit and > > (merged) invalidation don't dirty any tile priorities. In that case commit > won't > > do PrepareTiles, but scheduler might do PrepareTiles if tile priorities get > > dirtied afterwards. > > We are talking about the case where the commit doesn't merge impl-side > invalidation, because there is no request for it when the commit happens, and > the commit doesn't do PrepareTiles. But there can be an impl-side invalidation > request and prepare tiles request between the commit and inside the deadline. So > the DCHECK should be: > > DCHECK(!needs_prepare_tiles_ || !needs_impl_side_invalidation_ || > impl_side_invalidation_funnel_); > > If we are doing PrepareTiles over invalidation, then the invalidation funnel > should be full. > Actually, what about the case where the pending tree doesn't activate for 2 frames. So now you have another invalidation request, the funnel is not full, but you can't perform invalidations. But you can still do PrepareTiles. At this point we are just going through conditions in ShouldPerformImplSideInvalidations. There is no point of this DCHECK. :l > > > > > > > > There is also the case where calling ShouldPerformImplSideInvalidation > > requests > > > another invalidation but running it doesn't do PrepareTiles. > > > > > > I think having the test to ensure we call ShouldPerformImplSideInvalidation > > > before ShouldPrepareTiles is enough. We don't need anything else here. Does > > that > > > sound right? > > > > It's OK to leave the DCHECK out for now. lgtm
The CQ bit was checked by khushalsagar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org, sunnyps@chromium.org Link to the patchset: https://codereview.chromium.org/2659123004/#ps200001 (title: "no DCHECK")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1487896984857520,
"parent_rev": "e7f1cb881be7f52498d58b21ea855a5db487a515", "commit_rev":
"ab73d50c7514cdb1ed443d090c5b9d6a17d4b585"}
Message was sent while issue was closed.
Description was changed from ========== cc: Add scheduler support for invalidating content on impl thread. The change adds an API to the scheduler for requesting invalidations to content on the impl thread. The scheduler makes best effort to merge impl-side invalidations with the incoming main frame, if any, but may decide to create a pending tree for these invalidations on the impl thread. BUG=686267 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== cc: Add scheduler support for invalidating content on impl thread. The change adds an API to the scheduler for requesting invalidations to content on the impl thread. The scheduler makes best effort to merge impl-side invalidations with the incoming main frame, if any, but may decide to create a pending tree for these invalidations on the impl thread. BUG=686267 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2659123004 Cr-Commit-Position: refs/heads/master@{#452720} Committed: https://chromium.googlesource.com/chromium/src/+/ab73d50c7514cdb1ed443d090c5b... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/ab73d50c7514cdb1ed443d090c5b...
Message was sent while issue was closed.
andersonbriank1970@gmail.com changed reviewers: + andersonbriank1970@gmail.com
Message was sent while issue was closed.
Dont understand
Message was sent while issue was closed.
lgtm How do u get into ur gmail account
Message was sent while issue was closed.
lgtm lgtm How do u get into ur gmail account |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
