Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
23495022:
CC: Add a scheduled action for ManageTiles (Closed)
|
Created:
7 years, 3 months ago by epenner Modified:
7 years, 3 months ago CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionCC: Add a scheduled action for ManageTiles
Other than commits, ManageTiles should be coalesced to
happen on a semi-regular basis. This used to happen during
draws, but this is not reliable as draws might not occur,
nor we shouldn't force them to.
This adds a scheduled action for ManagingTiles, and the
ability to request this action, similar to requesting draws
etc.
BUG=264566
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223182
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Address feedback. #Patch Set 3 : Call ManageTiles. #
Total comments: 6
Patch Set 4 : Scheduled Action. #Patch Set 5 : Add to AsValue #
Total comments: 10
Patch Set 6 : Reduce scope to scheduler. #
Total comments: 5
Patch Set 7 : Fix build/tests. #Patch Set 8 : Add tests. #Patch Set 9 : Rebase. #Patch Set 10 : Use frame count instead of begin-frame throttling #
Total comments: 7
Patch Set 11 : Add failing test. #Patch Set 12 : Remove once-per-frame check. #Patch Set 13 : Fix grammar. #
Total comments: 4
Patch Set 14 : Remove unused. #
Messages
Total messages: 57 (0 generated)
Ptal.
If we're not going to be activating the pending tree in smoothness mode, is there additional logic to be put in place to avoid doing any sorts of management on it (tile management, updating transforms, etc)?
On 2013/09/03 18:40:28, vangelis wrote: > If we're not going to be activating the pending tree in smoothness mode, is > there additional logic to be put in place to avoid doing any sorts of management > on it (tile management, updating transforms, etc)? +enne to make sure what I say below is actually correct! This patch still activates actually, it just doesn't activate if it would require on-demand-raster to do so. Unless I'm mistaken, we still need activation for the basic scrolling-with-no-invalidations case. The reason, IIUC, is that we don't have 100% SkPicture coverage. If there is no invalidations, we still need to get new SkPicture tiles from webkit during the scroll to avoid scrolling into endless checkerboard eventually. I could be wrong, but that's my understanding. This begs the question of what we do if we _both_ lack SkPicture coverage _and_ have large invalidations pending.
https://codereview.chromium.org/23495022/diff/4001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/23495022/diff/4001/cc/resources/tile_manager.... cc/resources/tile_manager.cc:249: bool can_activate = true; can you drop this and just early out below when you find a tile that need raster-on-demand but "global_state_.tree_priority == SMOOTHNESS_TAKES_PRIORITY"? https://codereview.chromium.org/23495022/diff/4001/cc/trees/layer_tree_host_i... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/23495022/diff/4001/cc/trees/layer_tree_host_i... cc/trees/layer_tree_host_impl.cc:2476: if (priority != SMOOTHNESS_TAKES_PRIORITY) We need this to trigger a call to ManageTiles() even without this change. It's a bug if that is not already happening. Not sure client_->SetNeedsRedrawOnImplThread() is the best way to enforce this. Maybe the best is to just call ManageTiles() here directly. Brian, Enne, any suggestions?
On 2013/09/03 18:40:28, vangelis wrote: > If we're not going to be activating the pending tree in smoothness mode, is > there additional logic to be put in place to avoid doing any sorts of management > on it (tile management, updating transforms, etc)? I'm not sure what you're getting at here. This patch doesn't prevent all activations, just activations that would require a raster-on-demand tile. And, you'd still need ManageTiles if you were entirely ignoring the pending tree. https://codereview.chromium.org/23495022/diff/4001/cc/trees/layer_tree_host_i... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/23495022/diff/4001/cc/trees/layer_tree_host_i... cc/trees/layer_tree_host_impl.cc:2477: client_->SetNeedsRedrawOnImplThread(); Sorry, but I don't follow this. If we're missing a call to ManageTiles without a redraw then that seems like a larger issue that should be solved outside this patch.
https://codereview.chromium.org/23495022/diff/4001/cc/trees/layer_tree_host_i... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/23495022/diff/4001/cc/trees/layer_tree_host_i... cc/trees/layer_tree_host_impl.cc:2477: client_->SetNeedsRedrawOnImplThread(); On 2013/09/03 19:11:13, enne wrote: > Sorry, but I don't follow this. If we're missing a call to ManageTiles without > a redraw then that seems like a larger issue that should be solved outside this > patch. The current problem case is a double-tap zoom animation. In that case we don't activate after the zoom is finished, unless we do another accelerated scroll. If this is a deeper bug, I'll remove this code from this patch and work on a better fix for that in a separate patch.
Addressed feedback. Ptal for just the activation blocking part. I won't land this patch until the double-tap zoom issue is handled elsewhere. I'm looking into how we trigger manage-tiles in other places. https://codereview.chromium.org/23495022/diff/4001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/23495022/diff/4001/cc/resources/tile_manager.... cc/resources/tile_manager.cc:249: bool can_activate = true; On 2013/09/03 19:03:21, David Reveman wrote: > can you drop this and just early out below when you find a tile that need > raster-on-demand but "global_state_.tree_priority == SMOOTHNESS_TAKES_PRIORITY"? Good one! Done.
It looks like ManageTiles is for the most part coalesced into draws, except for some cases where we do it explicitly. We can do it explicitly in this case (directly in SetTreePriority) and it does kick a new frame out eventually. However, it takes _much_ longer for any new content to replace checkerboard after zooming out... Looking deeper into the code, I believe these are the relevant events: 1.) The zoom-out animation ends 2.) SetTreePriority occurs 0.25 seconds later 3.) We are now able to paint pending activation tiles 4.) Priorities are equal so we paint both pending and active tiles (low res first) 5.) We are able to activate. And I believe calling ManageTiles directly means we don't get a new frame until 5.), vs 1.) if we trigger a draw. It looks like the reason for this is that we only check for new visible tiles during PrepareToDraw?
Ping. Ptal for this one. This one will have to land first: https://codereview.chromium.org/23686011/
> https://codereview.chromium.org/23686011/ The above patch has landed. So this patch should be sufficient to solve the raster-on-demand we are seeing. Any other issues with this approach?
On 2013/09/09 19:26:32, epenner wrote: > > https://codereview.chromium.org/23686011/ > > The above patch has landed. So this patch should be sufficient > to solve the raster-on-demand we are seeing. > > Any other issues with this approach? My only concern with this approach is that slow small scrolls (+/- 1px constantly) can cause the page to not activate. Is that right? If that's the case then I want to make sure that we all agree that that's acceptable behaviour. Also, can you please file some sort of follow-up if a bug doesn't exist yet that would track the "correct" solution, for some definition of correct?
On 2013/09/09 21:44:57, vmpstr wrote: > On 2013/09/09 19:26:32, epenner wrote: > > > https://codereview.chromium.org/23686011/ > > > > The above patch has landed. So this patch should be sufficient > > to solve the raster-on-demand we are seeing. > > > > Any other issues with this approach? > > My only concern with this approach is that slow small scrolls (+/- 1px > constantly) can cause the page to not activate. Is that right? If that's the > case then I want to make sure that we all agree that that's acceptable > behaviour. Also, can you please file some sort of follow-up if a bug doesn't > exist yet that would track the "correct" solution, for some definition of > correct? That's right. The two required conditions are: 1.) Be in an accelerated gesture. 2.) Visible pending tiles don't make the priority cut. Okay I'll bring this up again on the bug.
Ptal. Reveman/Enne, can you comment on the bug if you think we need further work here? Otherwise is this approach good to go? Note also that I've added the ManageTiles call instead of SetNeedsRedraw. Before the other patch (https://codereview.chromium.org/23686011/) we wouldn't even show new active tree tiles, but that patch still doesn't address the activation live-lock. The live-lock issue exists when we finish raster and haven't yet activated. The next time we would call ManageTiles is in the next CC frame (which is too late for webkit). I don't think there is a way around this unless we promote ManageTiles into the scheduler.
https://codereview.chromium.org/23495022/diff/22001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/23495022/diff/22001/cc/resources/tile_manager... cc/resources/tile_manager.cc:269: if (!allow_rasterize_on_demand) Instead of calling ManageTiles again, would it work to set a flag here indicating that you should call NotifyReadyToActivate when you exit SMOOTHNESS_TAKES_PRIORITY?
https://codereview.chromium.org/23495022/diff/22001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/23495022/diff/22001/cc/resources/tile_manager... cc/resources/tile_manager.cc:269: if (!allow_rasterize_on_demand) On 2013/09/10 02:35:19, Brian Anderson wrote: > Instead of calling ManageTiles again, would it work to set a flag here > indicating that you should call NotifyReadyToActivate when you exit > SMOOTHNESS_TAKES_PRIORITY? The trouble is we are not ready to activate. Potentially we need to do all of the following: - ManageTiles() to prioritize visible pending tiles - Paint those visible pending tiles. - Finally, Activate. Hmm, we could however store a flag indicating we blocked raster-on-demand, and only trigger ManageTiles in that case. It's very common on Android, but that's partially due to the URL bar causing invalidations.
https://codereview.chromium.org/23495022/diff/22001/cc/resources/tile_manager.cc File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/23495022/diff/22001/cc/resources/tile_manager... cc/resources/tile_manager.cc:269: if (!allow_rasterize_on_demand) On 2013/09/10 18:13:39, epenner wrote: > On 2013/09/10 02:35:19, Brian Anderson wrote: > > Instead of calling ManageTiles again, would it work to set a flag here > > indicating that you should call NotifyReadyToActivate when you exit > > SMOOTHNESS_TAKES_PRIORITY? > > The trouble is we are not ready to activate. Potentially we need to do all of > the following: > - ManageTiles() to prioritize visible pending tiles > - Paint those visible pending tiles. > - Finally, Activate. > > Hmm, we could however store a flag indicating we blocked raster-on-demand, and > only trigger ManageTiles in that case. It's very common on Android, but that's > partially due to the URL bar causing invalidations. We should be calling ManageTiles as a result of changing tree priority mode independent of raster-on-demand. Avoiding that seems like a bad idea. https://codereview.chromium.org/23495022/diff/22001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/23495022/diff/22001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2521: if (priority != SMOOTHNESS_TAKES_PRIORITY) why not when entering SMOOTHNESS_TAKES_PRIORITY or NEW_CONTENTS_TAKES_PRIORITY? are we more likely to call ManageTiles() at a later time in that case?
https://codereview.chromium.org/23495022/diff/22001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/23495022/diff/22001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2521: if (priority != SMOOTHNESS_TAKES_PRIORITY) On 2013/09/10 19:21:20, David Reveman wrote: > why not when entering SMOOTHNESS_TAKES_PRIORITY or NEW_CONTENTS_TAKES_PRIORITY? > are we more likely to call ManageTiles() at a later time in that case? The reason we are calling it here is to not starve activation, which is only possible when exiting smoothness mode. It's primarily called on every frame, so I think that is sufficient for the rest of the cases. I don't mind calling it more often if that's desired, but I don't see the necessity for doing so.
https://codereview.chromium.org/23495022/diff/22001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/23495022/diff/22001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:2521: if (priority != SMOOTHNESS_TAKES_PRIORITY) On 2013/09/10 19:36:55, epennerAtGoogle wrote: > On 2013/09/10 19:21:20, David Reveman wrote: > > why not when entering SMOOTHNESS_TAKES_PRIORITY or > NEW_CONTENTS_TAKES_PRIORITY? > > are we more likely to call ManageTiles() at a later time in that case? > > The reason we are calling it here is to not starve activation, which is only > possible when exiting smoothness mode. It's primarily called on every frame, so > I think that is sufficient for the rest of the cases. > > I don't mind calling it more often if that's desired, but I don't see the > necessity for doing so. How are we guaranteeing that a new frame is produced immediately after enter smoothness mode? If we're not always calling ManageTiles here can we add a unit test to ensure that a new frame is produced and ManageTiles is called as expected?
> How are we guaranteeing that a new frame is produced immediately after enter > smoothness mode? If we're not always calling ManageTiles here can we add a unit > test to ensure that a new frame is produced and ManageTiles is called as > expected? I didn't say that it is guaranteed, but rather that calling ManageTiles during draws is sufficient in other cases. What bad behavior or bug would be prevented by guaranteeing ManageTiles was called. It looks like the point of coalescing it into draws in the first places was that it gets updated on a 'semi-regular' basis: https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/thread_pr... In some cases that's not enough, and stalling activation during smoothness mode creates one such instance.
> In some cases that's not enough, and stalling activation during smoothness mode > creates one such instance. I'd much rather call ManageTiles under the precision condition when we need it. I think the precise condition is 'stalled_activation_in_smoothness_mode_'. Any instance where we call ManageTiles explicitly is bad, as it means we are not doing it efficiently (once per frame). If there is too many cases where we call ManageTiles twice in a frame, it should probably be moved to the scheduler to understand and prevent that.
On 2013/09/10 21:15:52, epenner wrote: > If there is too many cases where we call > ManageTiles twice in a frame, it should probably be moved to the scheduler to > understand and prevent that. Moving it to the scheduler sounds like a good plan to me.
> Moving it to the scheduler sounds like a good plan to me. That feels like the right place to me. Then it can be triggered via various state, and it won't even need a draw to trigger. However, since that's purely a refactor, and this patch doesn't make inefficient ManageTiles calls, can we still land this patch? We also need to confirm this activation behavior is acceptable to ship in low-memory devices (raster-on-demand is a big problem). If so I'll do the scheduler refactor in a follow-up P1.
> However, since that's purely a refactor, and this patch doesn't make inefficient > ManageTiles calls, can we still land this patch? We also need to confirm this > activation behavior is acceptable to ship in low-memory devices > (raster-on-demand is a big problem). If so I'll do the scheduler refactor in a > follow-up P1. This isn't minimal anymore, but here's the scheduler change. This trigger ManageTiles via the scheduler even if there is no draw. I rename the old needs_manage_tiles_ to tile_priorities_dirty_ to avoid confusing this with the new state in the scheduler which triggers the action. PTAL.
I like the direction of this. I prefer to see the scheduler changes land first if you want to split this into 2 patches. https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:512: bool tile_priorities_dirty_; why do we still need this state here? isn't it enough with the scheduler state? https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1193: if (layer_tree_host_impl_->settings().impl_side_painting) nit: maybe move this conditional to LTHI where we already have a lot of impl-side painting conditional code. Also, can this be a DCHECK instead?
https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (left): https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:261: ManageTiles(); Will this ManageTiles() be a redundant call now that the Scheduler initiates ManageTiles? It was very confusing and error prone when both the Scheduler and LTHI were activating the pending tree. If the Scheduler is going to be calling ManageTiles, I think it would be a good idea to make it the only class that initiates them. https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1193: if (layer_tree_host_impl_->settings().impl_side_painting) On 2013/09/11 15:27:45, David Reveman wrote: > nit: maybe move this conditional to LTHI where we already have a lot of > impl-side painting conditional code. Also, can this be a DCHECK instead? Should we also have a DCHECK to make sure SetNeedsManageTilesOnImplThread only occurs when impl-side-painting?
https://codereview.chromium.org/23495022/diff/43001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/43001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:448: bool SchedulerStateMachine::ShouldManageTiles() const { Should this function be throttling the calls to ManageTiles at all? Or is SetNeedsManageTiles properly throttled already. Currently, this will always call ManageTiles() towards the end of the series of actions within Scheduler::ProcessScheduledActions. One way to throttle this to once per frame would be to return false here if we are not inside a BeginFrame and modify ProactiveBeginFrameWantedByImplThread to make sure we keep ticking while needs_manage_tiles_ is true.
I realized I also need to change WillModifyTilePriority to DidModifyTilePriority if this function will trigger the scheduled action. The reason is that the action can occur synchronously (even though this rarely happens). If it is triggered synchronously, ManageTiles will occur before the priorities are actually changed. https://codereview.chromium.org/23495022/diff/43001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/43001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:448: bool SchedulerStateMachine::ShouldManageTiles() const { Yes I was concerned about throttling but I didn't know if BeginFrame was the place to do this. Sounds good. https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.cc (left): https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.cc:261: ManageTiles(); On 2013/09/11 17:10:30, Brian Anderson wrote: > Will this ManageTiles() be a redundant call now that the Scheduler initiates > ManageTiles? It was very confusing and error prone when both the Scheduler and > LTHI were activating the pending tree. If the Scheduler is going to be calling > ManageTiles, I think it would be a good idea to make it the only class that > initiates them. This isn't redundant unless we replicate this behavior in the Scheduler. Currently I don't do that. I do think it should move to the Scheduler but I don't know if that should be in this first patch. In terms of replicating it, I _think_ we could get away doing only this ManageTiles (somewhere in the scheduler) and skipping any other ManageTiles within the same frame... But I'm a bit hesitant to conclude that's always okay. https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/23495022/diff/43001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:512: bool tile_priorities_dirty_; On 2013/09/11 15:27:45, David Reveman wrote: > why do we still need this state here? isn't it enough with the scheduler state? We still call ManageTiles explicitly in a few places (see OnTimerTick() and CommitComplete()). The current scheduler action only replaces the ManageTiles() 'semi-regular' ManageTiles that used to happen during draws. To remove the dirty flag we would need to remove the other cases as well. If it's clear I can try that, but otherwise I'd prefer that happens in another patch. https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc File cc/trees/thread_proxy.cc (right): https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1193: if (layer_tree_host_impl_->settings().impl_side_painting) On 2013/09/11 15:27:45, David Reveman wrote: > nit: maybe move this conditional to LTHI where we already have a lot of > impl-side painting conditional code. Also, can this be a DCHECK instead? Done. https://codereview.chromium.org/23495022/diff/43001/cc/trees/thread_proxy.cc#... cc/trees/thread_proxy.cc:1193: if (layer_tree_host_impl_->settings().impl_side_painting) On 2013/09/11 17:10:30, Brian Anderson wrote: > On 2013/09/11 15:27:45, David Reveman wrote: > > nit: maybe move this conditional to LTHI where we already have a lot of > > impl-side painting conditional code. Also, can this be a DCHECK instead? > > Should we also have a DCHECK to make sure SetNeedsManageTilesOnImplThread only > occurs when impl-side-painting? Done.
On 2013/09/11 18:42:59, epennerAtGoogle wrote: > I realized I also need to change WillModifyTilePriority to DidModifyTilePriority > if this function will trigger the scheduled action. The reason is that the > action can occur synchronously (even though this rarely happens). If it is > triggered synchronously, ManageTiles will occur before the priorities are > actually changed. Forget this bit. I think I will change the name/behavior to DidModifyTilePriorities just to be more clear, but I'll also avoid this from ever triggering the scheduled action synchronously.
Okay I've reduced this to only the scheduled-action change, and addressed most feedback. I haven't removed the explicit ManageTiles in CommitComplete. After looking into it, we need to call it always in that case, so to have the Scheduler do it would be analogous to just changing 'ScheduledActionCommit' to 'ScheduledActionCommitAndManageTiles'. I do see the potential to optimize ManageTiles during commits, but I don't think it belongs in the scheduler. For example, we could defer the synchronous ManageTiles by calling SetNeedsManageTiles, but only when we are in an accelerated gesture (or something like that). PTAL.
https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:765: return true; Brianderson@, is there anything I need to worry about here regarding the synchronous compositor. The comment references this being an issue, but I don't know understand how that case should be handled (or whether it's handled in general somewhere else).
Changes the subject and description for reduced scope. Ptal.
https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:765: return true; On 2013/09/11 22:55:47, epenner wrote: > Brianderson@, is there anything I need to worry about here regarding the > synchronous compositor. The comment references this being an issue, but I don't > know understand how that case should be handled (or whether it's handled in > general somewhere else). I'm working on a patch to fix issues like this, when we need to poll for actions/state that might produce a new frame with the synchronous compositor: https://codereview.chromium.org/23463014/ For that to work with this patch, you will need to throttle ShouldManageTiles by the current_frame_number_.
https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:448: bool SchedulerStateMachine::ShouldManageTiles() const { Yeah, I agree strongly with brianderson's suggestion about throttling this to happen once per frame. Without that, this seems like this patch could accidentally regress current behavior. It'd also be nice if the ManageTiles from commit could be wrapped into the scheduler too so that the manual ManageTiles there wouldn't happen in the same frame as a scheduler-initiated one. I think you could do this if you added a needs_post_commit_manage_tiles_ variable. You could update its value to needs_manage_tiles_ in OnUpdateState, and then check it here before checking inside_begin_frame. What do you think?
https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:448: bool SchedulerStateMachine::ShouldManageTiles() const { On 2013/09/11 23:45:11, enne wrote: > Yeah, I agree strongly with brianderson's suggestion about throttling this to > happen once per frame. Without that, this seems like this patch could > accidentally regress current behavior. > > It'd also be nice if the ManageTiles from commit could be wrapped into the > scheduler too so that the manual ManageTiles there wouldn't happen in the same > frame as a scheduler-initiated one. I think you could do this if you added a > needs_post_commit_manage_tiles_ variable. You could update its value to > needs_manage_tiles_ in OnUpdateState, and then check it here before checking > inside_begin_frame. What do you think? Just for clarity: The extra throttling is only required for the synchronous compositor right? IIUC throttling by inside_begin_frame_ is enough in other cases? Regarding the commit ManageTiles: I initially thought we could merge the synchronous commit ManageTiles... However, I think there is a problem: The commit changes tile priorities, so the last ManageTiles isn't sufficient. Therefore, waiting for the next ManageTiles effectively delays commits due to delaying painting the new tiles etc. So unless my understanding above is off, we can't skip the commit ManageTiles unless we are in an accelerate gesture (don't care about commit latency)... Alternatively maybe we could skip the next post-draw ManageTiles, but I think that has the same problem (CC may have changed scroll offsets etc. and we would miss those for one frame).
https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/52001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:448: bool SchedulerStateMachine::ShouldManageTiles() const { On 2013/09/12 00:09:59, epennerAtGoogle wrote: > On 2013/09/11 23:45:11, enne wrote: > > Yeah, I agree strongly with brianderson's suggestion about throttling this to > > happen once per frame. Without that, this seems like this patch could > > accidentally regress current behavior. > > > > It'd also be nice if the ManageTiles from commit could be wrapped into the > > scheduler too so that the manual ManageTiles there wouldn't happen in the same > > frame as a scheduler-initiated one. I think you could do this if you added a > > needs_post_commit_manage_tiles_ variable. You could update its value to > > needs_manage_tiles_ in OnUpdateState, and then check it here before checking > > inside_begin_frame. What do you think? > > Just for clarity: The extra throttling is only required for the synchronous > compositor right? IIUC throttling by inside_begin_frame_ is enough in other > cases? > > Regarding the commit ManageTiles: I initially thought we could merge the > synchronous commit ManageTiles... However, I think there is a problem: The > commit changes tile priorities, so the last ManageTiles isn't sufficient. > Therefore, waiting for the next ManageTiles effectively delays commits due to > delaying painting the new tiles etc. > > So unless my understanding above is off, we can't skip the commit ManageTiles > unless we are in an accelerate gesture (don't care about commit latency)... > Alternatively maybe we could skip the next post-draw ManageTiles, but I think > that has the same problem (CC may have changed scroll offsets etc. and we would > miss those for one frame). Yes, the extra throttling is only required for the synchronous compositor, but should also work without the synchronous compositor and make the check for inside_begin_frame unnecessary. I agree that we should not skip the ManageTiles after the commit as it would delay activation. It would be nice if the ManageTiles after the commit cleared the Scheduler's needs_manage_tiles and also blocked future ManageTiles from the Scheduler until the next frame though.
> Yes, the extra throttling is only required for the synchronous compositor, but > should also work without the synchronous compositor and make the check for > inside_begin_frame unnecessary. > > I agree that we should not skip the ManageTiles after the commit as it would > delay activation. It would be nice if the ManageTiles after the commit cleared > the Scheduler's needs_manage_tiles and also blocked future ManageTiles from the > Scheduler until the next frame though. I think there is a slight issue with that too, as we miss the changes in priority from the compositor for one frame. If the post-draw ManageTiles just needs to be 'semi-regular' it might be fine to skip the next one. Another problem is that ManageTiles occurs _after_ draw/swap, so if we do the commit ManageTiles we have already done two in one frame... Skipping the next one would reduce the CPU cost on the compositor thread, but it would mean we did 2 in one frame followed by 0 in the next.
Hmm.. One possibility would be to mark if there is a commit, and the move the ManageTiles to before the draw instead of after it. But that creates a whole different can of worms. Given the complexity of this optimization, I'm more inclined to not optimize ManageTiles calls in this patch, but rather just keep them the same. Is that acceptable? The goal of this patch is primarily to guarantee ManageTiles executes at all, when SetNeedsManageTiles is called.
Tests?
On 2013/09/12 17:17:50, enne wrote: > Tests? Coming right up :)
Added some tests and DCHECKs. Ptal.
On 2013/09/12 23:24:54, epenner wrote: > Added some tests and DCHECKs. Ptal. Just switched the throttling to use the current_frame_number_. Brian can you double check that this is what you meant?
The way you are throttling ManageTiles will limit it to a single scheduled call per frame. Assuming that doesn't throttle ManageTiles that are needed immediately (such as after a commit), you should be good. https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc... cc/scheduler/scheduler.cc:69: DCHECK(!IsInsideAction(SchedulerStateMachine::ACTION_MANAGE_TILES)); How does ManagingTiles result in SetNeedsManageTiles from being called and why is it okay to ignore it here?
Yes it doesn't throttle the other synchronous ManageTiles calls... I called it 'ScheduledManageTiles' to try to clarify that a bit. https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc... cc/scheduler/scheduler.cc:69: DCHECK(!IsInsideAction(SchedulerStateMachine::ACTION_MANAGE_TILES)); On 2013/09/13 00:29:00, Brian Anderson wrote: > How does ManagingTiles result in SetNeedsManageTiles from being called and why > is it okay to ignore it here? We don't do it. This is just meant to insure we never do that by mistake.
Scheduler changes lgtm. https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc... cc/scheduler/scheduler.cc:69: DCHECK(!IsInsideAction(SchedulerStateMachine::ACTION_MANAGE_TILES)); On 2013/09/13 00:37:56, epennerAtGoogle wrote: > On 2013/09/13 00:29:00, Brian Anderson wrote: > > How does ManagingTiles result in SetNeedsManageTiles from being called and why > > is it okay to ignore it here? > > We don't do it. This is just meant to insure we never do that by mistake. Ah, I completely miss read the code. I really like the concept of the IsInsideAction method. We can probably use it to DCHECK in a few other places.
https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler.cc... cc/scheduler/scheduler.cc:234: mark_inside_action(&inside_action_, action); Nifty! https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:455: bool SchedulerStateMachine::ShouldManageTiles() const { I liked the previous version of this patch where it wouldn't manage tiles unless it was inside begin frame (to prevent doing it before we draw). Is that taken care of elsewhere that I'm missing?
https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:455: bool SchedulerStateMachine::ShouldManageTiles() const { On 2013/09/13 00:47:01, enne wrote: > I liked the previous version of this patch where it wouldn't manage tiles unless > it was inside begin frame (to prevent doing it before we draw). Is that taken > care of elsewhere that I'm missing? Good catch! I was only thinking of the two top level events (begin frame and the new Polling method Brian added)... Hmm, Brian is there a good way to limit to just those two calls? Taking a look.
https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... cc/scheduler/scheduler_state_machine.cc:455: bool SchedulerStateMachine::ShouldManageTiles() const { On 2013/09/13 00:47:01, enne wrote: > I liked the previous version of this patch where it wouldn't manage tiles unless > it was inside begin frame (to prevent doing it before we draw). Is that taken > care of elsewhere that I'm missing? The issue with restricting it to occur only inside the BeginFrame is that WebView might not send us another BeginFrame to ManageTiles with. But I see how we'd still want the draw to occur before a ManageTiles. We could restrict the inside BeginFrame check only to cases where NeedsBeginFrameToDraw is true. That way, if there isn't an immediate draw pending for either the regular or synchronous compositor, we could fill in that time with a ManageTiles.
On 2013/09/13 00:59:51, Brian Anderson wrote: > https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... > File cc/scheduler/scheduler_state_machine.cc (right): > > https://codereview.chromium.org/23495022/diff/34001/cc/scheduler/scheduler_st... > cc/scheduler/scheduler_state_machine.cc:455: bool > SchedulerStateMachine::ShouldManageTiles() const { > On 2013/09/13 00:47:01, enne wrote: > > I liked the previous version of this patch where it wouldn't manage tiles > unless > > it was inside begin frame (to prevent doing it before we draw). Is that taken > > care of elsewhere that I'm missing? > > The issue with restricting it to occur only inside the BeginFrame is that > WebView might not send us another BeginFrame to ManageTiles with. But I see how > we'd still want the draw to occur before a ManageTiles. > > We could restrict the inside BeginFrame check only to cases where > NeedsBeginFrameToDraw is true. That way, if there isn't an immediate draw > pending for either the regular or synchronous compositor, we could fill in that > time with a ManageTiles. I added a failing test for this case. Above sounds good, but I'm having trouble convincing myself it's iron clad. Hmm, how about just adding inside_being_frame || inside_polling_for_anticipated_draw_triggers_? In general it feels doing things 'once-per-frame' can result in a lot of reordering, based on unrelated events. We should probably be robust to that, but it still makes me feel slightly uneasy.
Hmm, is_inside_throttled_event_ ?
On 2013/09/13 01:16:36, epennerAtGoogle wrote: > Hmm, is_inside_throttled_event_ ? Adding inside_polling_for_anticipated_draw_triggers_ sounds good. If you mean that is_inside_throttled_event_ should be equivalent to: (inside_begin_frame || inside_polling_for_anticipated_draw_triggers_), I would still like to keep inside_polling_for_anticipated_draw_triggers_ as well, since it's a different concept. The other events that execute once-per-frame have their ordering enforced by back pressure in the pipeline. ManageTiles is a special case because it doesn't just get inserted into the pipeline, so batching it together with when we might draw seems reasonable since we only seem to care about the ordering with respect to drawing.
On 2013/09/13 01:33:57, Brian Anderson wrote: > On 2013/09/13 01:16:36, epennerAtGoogle wrote: > > Hmm, is_inside_throttled_event_ ? > > Adding inside_polling_for_anticipated_draw_triggers_ sounds good. > > If you mean that is_inside_throttled_event_ should be equivalent to: > (inside_begin_frame || inside_polling_for_anticipated_draw_triggers_), I would > still like to keep inside_polling_for_anticipated_draw_triggers_ as well, since > it's a different concept. > > The other events that execute once-per-frame have their ordering enforced by > back pressure in the pipeline. ManageTiles is a special case because it doesn't > just get inserted into the pipeline, so batching it together with when we might > draw seems reasonable since we only seem to care about the ordering with respect > to drawing. Okay I added inside_polling_ flag. Those cases both increment the frame count, so that makes the once-per-frame check redundant so I removed that. Ptal.
Scheduler changes lgtm.
https://codereview.chromium.org/23495022/diff/99001/cc/scheduler/scheduler_un... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/23495022/diff/99001/cc/scheduler/scheduler_un... cc/scheduler/scheduler_unittest.cc:747: // Draw. The draw will trigger SetNeedsManageTiles, and Where does the draw trigger SetNeedsManageTiles? https://codereview.chromium.org/23495022/diff/99001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/23495022/diff/99001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:432: bool tile_priorities_dirty() const { return tile_priorities_dirty_; } Unused?
https://codereview.chromium.org/23495022/diff/99001/cc/scheduler/scheduler_un... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/23495022/diff/99001/cc/scheduler/scheduler_un... cc/scheduler/scheduler_unittest.cc:747: // Draw. The draw will trigger SetNeedsManageTiles, and On 2013/09/13 18:15:01, enne wrote: > Where does the draw trigger SetNeedsManageTiles? The test does this via SchedulerClientNeedsManageTilesInDraw. In the real code any call to DidModifyTilePriorities will trigger it, primarily PictureLayerImpl::UpdateTilePriorities https://codereview.chromium.org/23495022/diff/99001/cc/trees/layer_tree_host_... File cc/trees/layer_tree_host_impl.h (right): https://codereview.chromium.org/23495022/diff/99001/cc/trees/layer_tree_host_... cc/trees/layer_tree_host_impl.h:432: bool tile_priorities_dirty() const { return tile_priorities_dirty_; } On 2013/09/13 18:15:01, enne wrote: > Unused? I've removed it. Only the tests require manage_tiles_needed() now, to check visibility cases.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/epenner@chromium.org/23495022/65017
Message was sent while issue was closed.
Change committed as 223182 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
