Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12194015:
cc: Rasterize cheap tiles immediately (Closed)
|
Created:
7 years, 10 months ago by Sami Modified:
7 years, 10 months ago CC:
chromium-reviews, cc-bugs_chromium.org, brianderson Visibility:
Public. |
Descriptioncc: Rasterize cheap tiles immediately
Rasterize cheap tiles immediately on the impl thread instead of
delegating them to the raster workers. This improves tile update
latency, because it avoids the communication and synchronization
overhead with the worker threads.
BUG=173426
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=183608
Patch Set 1 #
Total comments: 12
Patch Set 2 : Refactoring. Limit number of pending uploads. #Patch Set 3 : Cleanup. #Patch Set 4 : Limit number of rasters and time spent rasterizing. #Patch Set 5 : Respect smoothness mode. #Patch Set 6 : Trace event for cheap rasters that weren't. #
Total comments: 24
Patch Set 7 : Consider tiles with finished decodes for cheap raster. Deadline for cheap rasters. LTHI in charge o… #Patch Set 8 : Rebased. #
Total comments: 1
Patch Set 9 : Nat's suggestion for posterity. #Patch Set 10 : Schedule cheap tasks in worker pool. #
Total comments: 4
Patch Set 11 : David's feedback. Fixed double destruction of picture_pile_ for cheap tasks. #Patch Set 12 : Post a task to run cheap tasks. #
Total comments: 14
Patch Set 13 : Add ScheduleRunCheapTasks. #Patch Set 14 : Clone pictures for cheap tasks too. #Patch Set 15 : Call RunCheapTasks from TileManager. #Patch Set 16 : Fix picture pile reference management. Post a task for running cheap tasks. #
Total comments: 10
Patch Set 17 : David's comments. #
Total comments: 3
Patch Set 18 : Refactoring PostTask. #
Messages
Total messages: 40 (0 generated)
https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode655 cc/tile_manager.cc:655: RunRasterTask(resource_pool_->resource_provider()->mapPixelBuffer( Just checking: do I need to clone the picture for the impl thread here?
[+brianderson@]
+reveman
https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode655 cc/tile_manager.cc:655: RunRasterTask(resource_pool_->resource_provider()->mapPixelBuffer( On 2013/02/04 13:37:48, Sami wrote: > Just checking: do I need to clone the picture for the impl thread here? You're already on the impl thread here. It doesn't seem like cloning will be needed. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode731 cc/tile_manager.cc:731: OnRasterTaskCompleted(tile, resource.Pass(), In the "cheap tile" case, don't you still need to dispatch more raster tasks after you're done with one of them? It looks like DispatchMoreTasks() only gets called at completion of a threaded raster.
think some simple re-factoring can help make this look more as part of the tile manager design rather than something we tacked on. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode655 cc/tile_manager.cc:655: RunRasterTask(resource_pool_->resource_provider()->mapPixelBuffer( On 2013/02/04 13:37:48, Sami wrote: > Just checking: do I need to clone the picture for the impl thread here? You shouldn't need that. All other threads possibly using the picture will have a clone and those clones are created on this thread. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode661 cc/tile_manager.cc:661: TileManager::OnRasterTaskCompleted(tile, resource.Pass(), nit: TileManager:: is unnecessary. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode664 cc/tile_manager.cc:664: } I don't think all this belongs in DispatchOneRasterTask. DispatchMoreTasks() might be better. In this case you're not really dispatching a task so this running immediately when DispatchOneRasterTask is called seems a bit misleading unless we change the function name. Also the CanDispatchRasterTask() check shouldn't prevent us from running a cheap raster job, right? https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.h#newcode157 cc/tile_manager.h:157: int manage_tiles_call_count_when_dispatched); How about OnRasterCompleted() and OnRasterTaskCompleted() instead and only refer to work posted to the worker pool as tasks? https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.h#newcode165 cc/tile_manager.h:165: static void RunRasterTask(uint8* buffer, Maybe rename to PerformRaster if you like my suggestion above.
On 2013/02/04 22:33:50, vangelis wrote: > You're already on the impl thread here. It doesn't seem like cloning will be > needed. Ok, thanks for confirming. I thought the picture belonged to the main thread since we do the recording there. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode731 > cc/tile_manager.cc:731: OnRasterTaskCompleted(tile, resource.Pass(), > In the "cheap tile" case, don't you still need to dispatch more raster tasks > after you're done with one of them? It looks like DispatchMoreTasks() only gets > called at completion of a threaded raster. The problem is that if OnRasterTaskCompleted calls DispatchMoreTasks, we end up in an infinite loop because DispatchOneRasterTask is called before removing the pending task from the queue. We're not missing any tasks since DispatchMoreTasks runs through all all queues. I'll try the refactoring suggested by David to make this more obvious.
This version addresses all comments. I've also added a check against the maximum number of pending SetPixels. PTAL. I'll do a follow-up patch to implement a per-frame time and cardinality limit to cheap rasters since that needs a little more plumbing. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode661 cc/tile_manager.cc:661: TileManager::OnRasterTaskCompleted(tile, resource.Pass(), On 2013/02/04 22:53:24, David Reveman wrote: > nit: TileManager:: is unnecessary. Done. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.cc#newcode664 cc/tile_manager.cc:664: } On 2013/02/04 22:53:24, David Reveman wrote: > I don't think all this belongs in DispatchOneRasterTask. DispatchMoreTasks() > might be better. In this case you're not really dispatching a task so this > running immediately when DispatchOneRasterTask is called seems a bit misleading > unless we change the function name. Also the CanDispatchRasterTask() check > shouldn't prevent us from running a cheap raster job, right? Agreed, it is a little misleading. I've now split this into DispatchOneRasterTask() and PerformOneRasterTask() -- hopefully that's clearer. You're right that CanDispatchRasterTask() shouldn't prevent cheap rasters, but only once we use sync uploads. With this patch we still need to avoid scheduling too many async uploads. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.h#newcode157 cc/tile_manager.h:157: int manage_tiles_call_count_when_dispatched); On 2013/02/04 22:53:24, David Reveman wrote: > How about OnRasterCompleted() and OnRasterTaskCompleted() instead and only refer > to work posted to the worker pool as tasks? Good idea, I like that naming. https://codereview.chromium.org/12194015/diff/1/cc/tile_manager.h#newcode165 cc/tile_manager.h:165: static void RunRasterTask(uint8* buffer, On 2013/02/04 22:53:24, David Reveman wrote: > Maybe rename to PerformRaster if you like my suggestion above. Done.
I decided to implement the max count/time for cheap rasters in this patch since it was pretty straightforward.
My greatest concern here is what the effect of looping for 6ms in DispatchMoreTasks without returning to the message loop has. This is 6ms where we wont be notified about non-cheap raster and image decode tasks completing which could effectively result in worse raster efficiency than before. Can we implement Picture::IsCheapInRect before landing this so we have an idea what the result is? I'd also feel more comfortable if we only allowed one pending cheap raster and posted completion tasks to the message loop. That would make sure we didn't block the message loop for more than the time of one cheap raster job but maybe the overhead from doing this defeats the purpose of all this? https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:555: base::TimeDelta maxCheapRasterTime = nit: max_cheap_raster_time please https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:557: if (cheap_raster_time_ >= maxCheapRasterTime) { would we get better accuracy if we recorded a "cheap raster end time" in ResetCheapRasterBudget() and checked that we haven't gone past it here? I'm thinking of cases where the impl thread is unscheduled for 11ms and 6ms of cheap raster will make use miss the vsync. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:581: DispatchOneRasterTask(*it); how about these tiles? are tiles with images never considered cheap? maybe add a DCHECK(!CanPerformCheapRaster(tile)) in that case. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:599: base::TimeTicks begin_time = base::TimeTicks::Now(); we recently made sure not to call base::TimeTicks::Now() during raster unless benchmarking is enabled. should we be worried about the calls to base::TimeTicks::Now() here? https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:603: base::TimeDelta expectedCheapRasterTime = nit: expected_cheap_raster_time https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.h#newcode175 cc/tile_manager.h:175: static void RunImageDecodeTask(skia::LazyPixelRef* pixel_ref, PerformImageDecode to be consistent?
I think your concerns are valid, but do you think that given that this is behind-a-flag, we can land this as an incremental step toward the end state? [and anything is not behind a flag, it should be].
Talked offline with @reveman. Lets get this landed behind a flag with the comments before by David resolved. Then we can tinker.
https://codereview.chromium.org/12194015/diff/16001/cc/layer_tree_host_impl.cc File cc/layer_tree_host_impl.cc (right): https://codereview.chromium.org/12194015/diff/16001/cc/layer_tree_host_impl.c... cc/layer_tree_host_impl.cc:850: m_tileManager->ResetCheapRasterBudget(); Please invert the direciton of this; tile manager should say m_client->canDoAnotherCheapRasterTask() then we do one ->didPerformCheapRasterTask(amount of time consumed) have lthi do the bookkeeping https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcode44 cc/tile_manager.cc:44: const int kExpectedCheapRasterMilliseconds = 2; I've been working off of 1. Why 2? https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:540: if (global_state_.tree_priority == SMOOTHNESS_TAKES_PRIORITY) { I'm not convinced of this part. Please remove and we can add it back in afterward. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:545: // TODO(skyostil): Use synchronous uploads for inline rasters. dont see why this todo is relevant here... sync uploads are in another part of the code https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:564: TRACE_EVENT_INSTANT0("cc", "TileManager::CanPerformCheapRaster not cheap"); please remove and integrate with what vlad did in https://codereview.chromium.org/12192025/ https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:604: base::TimeDelta::FromMilliseconds(kExpectedCheapRasterMilliseconds); is this duplicative? you've got the same boookeeping inside the performOneRaster call already, no?
Thanks for the in-depth review guys. This should address all comments. Setting the frame period during which cheap rasters are allowed is an open item. Right now it is conservative and we can tune it once we have more data. https://codereview.chromium.org/12194015/diff/16001/cc/layer_tree_host_impl.cc File cc/layer_tree_host_impl.cc (right): https://codereview.chromium.org/12194015/diff/16001/cc/layer_tree_host_impl.c... cc/layer_tree_host_impl.cc:850: m_tileManager->ResetCheapRasterBudget(); On 2013/02/06 08:34:57, nduca wrote: > Please invert the direciton of this; tile manager should say > m_client->canDoAnotherCheapRasterTask() > > then we do one > ->didPerformCheapRasterTask(amount of time consumed) > > have lthi do the bookkeeping Done. I took the "Task" out of the names to keep things consistent -- only rasters done by the workers are tasks. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcode44 cc/tile_manager.cc:44: const int kExpectedCheapRasterMilliseconds = 2; On 2013/02/06 08:34:57, nduca wrote: > I've been working off of 1. Why 2? Tom was telling me his heuristic gave 1 ms rasters on average, so I thought it'd be better to flag the outliers. Tracking the raster time seems redundant now that Vlad landed his histogram patch, so I'll just remove this. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:540: if (global_state_.tree_priority == SMOOTHNESS_TAKES_PRIORITY) { On 2013/02/06 08:34:57, nduca wrote: > I'm not convinced of this part. Please remove and we can add it back in > afterward. Okay, let's keep things simple. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:545: // TODO(skyostil): Use synchronous uploads for inline rasters. On 2013/02/06 08:34:57, nduca wrote: > dont see why this todo is relevant here... sync uploads are in another part of > the code It's referring to the check against the total allowed async uploads below. Once cheap tiles are sync uploaded, that check should go away. I'll remove the TODO since it's already done in https://codereview.chromium.org/12185024/. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:555: base::TimeDelta maxCheapRasterTime = On 2013/02/05 20:59:13, David Reveman wrote: > nit: max_cheap_raster_time please Done (moved to lthi). https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:557: if (cheap_raster_time_ >= maxCheapRasterTime) { On 2013/02/05 20:59:13, David Reveman wrote: > would we get better accuracy if we recorded a "cheap raster end time" in > ResetCheapRasterBudget() and checked that we haven't gone past it here? I'm > thinking of cases where the impl thread is unscheduled for 11ms and 6ms of cheap > raster will make use miss the vsync. Good point, we could easily use more real time if we're descheduled. I'll change this to allow up to 6 ms of cheap rasters from when we last swapped. However, that's not ideal in the case of jsgamebench since it's intentionally running at 30 fps. I'm not sure what to do about that -- maybe allow cheap rasters during the first halves of frame intervals? https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:564: TRACE_EVENT_INSTANT0("cc", "TileManager::CanPerformCheapRaster not cheap"); On 2013/02/06 08:34:57, nduca wrote: > please remove and integrate with what vlad did in > https://codereview.chromium.org/12192025/ Done. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:581: DispatchOneRasterTask(*it); On 2013/02/05 20:59:13, David Reveman wrote: > how about these tiles? are tiles with images never considered cheap? maybe add a > DCHECK(!CanPerformCheapRaster(tile)) in that case. Oops, I think I misread this code earlier as only applying to tiles with pending image decodes, but actually it is the other way around. I'll change my patch to also consider these. Good catch, thanks. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:599: base::TimeTicks begin_time = base::TimeTicks::Now(); On 2013/02/05 20:59:13, David Reveman wrote: > we recently made sure not to call base::TimeTicks::Now() during raster unless > benchmarking is enabled. should we be worried about the calls to > base::TimeTicks::Now() here? I'm not sure how else to keep track of time spent rasterizing. A cursory look though base didn't reveal a cheaper low res time source. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:603: base::TimeDelta expectedCheapRasterTime = On 2013/02/05 20:59:13, David Reveman wrote: > nit: expected_cheap_raster_time Done (moved to lthi). https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:604: base::TimeDelta::FromMilliseconds(kExpectedCheapRasterMilliseconds); On 2013/02/06 08:34:57, nduca wrote: > is this duplicative? you've got the same boookeeping inside the performOneRaster > call already, no? Yeah, no need for this now that Vlad's patch landed. Removed. https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12194015/diff/16001/cc/tile_manager.h#newcode175 cc/tile_manager.h:175: static void RunImageDecodeTask(skia::LazyPixelRef* pixel_ref, On 2013/02/05 20:59:13, David Reveman wrote: > PerformImageDecode to be consistent? Yeah, good idea.
FYI, I'm seeing some crashes on Android with this patch combined with sync uploads -- looks like tiles_that_need_to_be_rasterized_ is somehow getting corrupted. I'll dig in more.
The crash was happening because DispatchMoreTasks() isn't re-entrant and as a side effect of DidUploadVisibleHighResolutionTile() it may get called again. Deferring that callback fixes this problem nicely. The next problem is that on Nexus 4 doing interleaved async and sync uploads seems to result in some uploads failing silently and the texture not getting updated. The same seems to work fine on other devices. I'll try to narrow this down further.
Since the Nexus 4 problems only affect async uploads (i.e., https://codereview.chromium.org/12185024/), I don't think there's anything blocking this particular patch. Anyone care to review? Thanks.
Do you have a clean blacklisting story for n4?
Can you give this another spin based on the feedback below? https://codereview.chromium.org/12194015/diff/16011/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/16011/cc/tile_manager.cc#newcod... cc/tile_manager.cc:607: PerformOneCheapRaster(tile); So how about doign this... you can save all the interaction with lth/etc: cheap_tiles = [] for tile in tiles_that_need_to_be_rasterized: ... usual crap to see if the tile is ready... if not this.didDoCheapTilesThisFrame && tile.ischeap: cheap_tiles.push_back() if cheap_tiles.size() == 6: break else: dispatch remove tile from tiles_that_need_be_rasterized deadilne = time.now() while len(cheap_tiles): tile =cheap_tiles.pop() tile.raster() if now > deadline: break tiles_that_need_to_be_rasterized.enqueue(cheap_tiles) do uploads for cheap tiles this.didDoCheapTilesThisFrame = true If you do it this way, then the only thing you have to do outside TileManager is tell the manager that there was a swap. Et voila. DOne.
In the light of https://codereview.chromium.org/12217105/ I think we should consider doing this by adding a "bool is_cheap" argument to RasterWorkerPool::PostTask. The worker pool will be better equipped to determine how many main thread tasks to allow and it's critical that cheap task completion processing is lined up with other tasks for good performance (we want all uploads to be issued at the same time) and this would make that straight forward. It should also make the tile manager code much cleaner. The raster worker pool could just queue up cheap tasks rather than posting them to raster threads and run them after dispatching non-cheap task up until it's time to perform CheckForCompletedTasks.
Is this at all along the lines you meant, David? I think it otherwise fits fine in WorkerPool except for the minor ugliness in migrating the picture piles to the worker thread if it turns out we can't complete all cheap tasks before the deadline. Also, WorkerPool::RunCheapTasks() has to be re-entrant because the task completion closures call back into it. P.S. A version based on Nat's suggestion is in patch set 9 if anyone wants to compare.
wow great job! thanks for implementing both solutions. below you'll find a few comments on the worker pool version. I think we should wait until https://codereview.chromium.org/12217105/ lands before we determine what's the best way to go. https://codereview.chromium.org/12194015/diff/27001/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/27001/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:29: picture_pile_ = picture_pile_->GetCloneForDrawingOnThread(thread); If we need this, I prefer if we didn't also do cloning in PostRasterTaskAndReply below. https://codereview.chromium.org/12194015/diff/27001/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/27001/cc/tile_manager.cc#newcod... cc/tile_manager.cc:669: if (raster_worker_pool_->RunCheapTasks()) this can be handled internally in the worker pool once my check at intervals cl lands. https://codereview.chromium.org/12194015/diff/27001/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/27001/cc/worker_pool.cc#newcode50 cc/worker_pool.cc:50: const int kMaxCheapTaskMilliseconds = 6; maybe you can use the same time limit for this as the tick interval once https://codereview.chromium.org/12217105/ lands. https://codereview.chromium.org/12194015/diff/27001/cc/worker_pool.h File cc/worker_pool.h (right): https://codereview.chromium.org/12194015/diff/27001/cc/worker_pool.h#newcode76 cc/worker_pool.h:76: void SetCheapTasksAllowed(bool allowed); would it be possible to leave this in the tile manager and just not post tasks with is_cheap=true if they are not allowed?
> https://codereview.chromium.org/12194015/diff/27001/cc/tile_manager.cc#newcod... > cc/tile_manager.cc:669: if (raster_worker_pool_->RunCheapTasks()) > this can be handled internally in the worker pool once my check at intervals cl > lands. Thanks David. I've implemented your other suggestions but I've got a question about this one. I'm not sure I understand how the worker pool would know to start running cheap tasks by itself. Did you mean something like scheduling a task on the cc thread to run the cheap tasks as soon as we have at least one in the queue? It would work, but cycling through message loop kind of defeats the purpose of cheap tasks -- or did I miss something?
On 2013/02/14 19:14:59, Sami wrote: > > > https://codereview.chromium.org/12194015/diff/27001/cc/tile_manager.cc#newcod... > > cc/tile_manager.cc:669: if (raster_worker_pool_->RunCheapTasks()) > > this can be handled internally in the worker pool once my check at intervals > cl > > lands. > > Thanks David. I've implemented your other suggestions but I've got a question > about this one. I'm not sure I understand how the worker pool would know to > start running cheap tasks by itself. Did you mean something like scheduling a > task on the cc thread to run the cheap tasks as soon as we have at least one in > the queue? It would work, but cycling through message loop kind of defeats the > purpose of cheap tasks -- or did I miss something? Yes, I was hoping we could schedule a RunCheapTask() whenever a cheap task is added and this would run as many cheap tasks as possible when called. It would only require one message loop cycle per 6ms cheap task run. If that ends up to costly we can add a ScheduleRunCheapTasks function to the WorkerPoolClient API but I'd prefer if we try without that first.
On 2013/02/14 19:38:49, David Reveman wrote: > Yes, I was hoping we could schedule a RunCheapTask() whenever a cheap task is > added and this would run as many cheap tasks as possible when called. It would > only require one message loop cycle per 6ms cheap task run. If that ends up to > costly we can add a ScheduleRunCheapTasks function to the WorkerPoolClient API > but I'd prefer if we try without that first. Okay, thanks for the clarification. This latest version should do exactly that. I'll play with it on some Android hardware tomorrow to see what the latency is like.
https://codereview.chromium.org/12194015/diff/35001/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/35001/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:31: picture_pile_->GetCloneForDrawingOnThread(thread); DCHECK(!picture_pile_clone_) before this statement? maybe this does the right thing when called multiple times but I think we should still check that it doesn't happen. https://codereview.chromium.org/12194015/diff/35001/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:63: worker_task->DeferToThread(worker); can we call this from WorkerPool::Worker::PostTask instead and not have this code make the assumption that a Worker is base::thread? https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode294 cc/worker_pool.cc:294: void WorkerPool::OnIdle() { do we need to make sure this is not called when we have pending cheap tasks? https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode344 cc/worker_pool.cc:344: origin_loop_->PostTask(FROM_HERE, cheap_task_callback_); Please add a ScheduleRunCheapTasks(). And you'll need to call ScheduleCheckForCompletedTasks() here too. https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode353 cc/worker_pool.cc:353: base::TimeDelta::FromMilliseconds(kMaxCheapTaskMs); Should the deadline for cheap tasks not be something like: nextVSyncTickTime - 10ms? You'll also need to set something like check_for_completed_tasks_time_ in ScheduleCheckForCompletedTasks and use that to determine when CheckForCompletedTasks() needs to be called. When reaching check_for_completed_tasks_time_ you can probably just make the call to CheckForCompletedTasks() and then keep running more cheap tasks until the deadline. https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode358 cc/worker_pool.cc:358: task->DidComplete(); Can we have this DidComplete call be done by CheckForCompletedTasks instead? That makes the flow more like the tasks that run on worker threads and if we ever need to add WorkerPoolClient::WillStartDispatchingWorkerPoolCompletionCallbacks() we could do that. https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode370 cc/worker_pool.cc:370: task->DeferToThread(worker); move DeferToThread into PostTask if possible.
Thanks David. Some concerns about the scheduling below. https://codereview.chromium.org/12194015/diff/35001/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/35001/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:31: picture_pile_->GetCloneForDrawingOnThread(thread); Good idea. This handles multiple calls fine, but we only expect it to be called once. https://codereview.chromium.org/12194015/diff/35001/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:63: worker_task->DeferToThread(worker); Yeah, that's much cleaner. https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode294 cc/worker_pool.cc:294: void WorkerPool::OnIdle() { On 2013/02/14 21:10:49, David Reveman wrote: > do we need to make sure this is not called when we have pending cheap tasks? Good point. I made it a no-op if there are pending cheap tasks -- as opposed to canceling the callback entirely since it's posted from the worker thread. https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode344 cc/worker_pool.cc:344: origin_loop_->PostTask(FROM_HERE, cheap_task_callback_); On 2013/02/14 21:10:49, David Reveman wrote: > Please add a ScheduleRunCheapTasks(). And you'll need to call > ScheduleCheckForCompletedTasks() here too. Done. See below for why I didn't add the ScheduleCheckForCompletedTasks(). https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode353 cc/worker_pool.cc:353: base::TimeDelta::FromMilliseconds(kMaxCheapTaskMs); On 2013/02/14 21:10:49, David Reveman wrote: > Should the deadline for cheap tasks not be something like: nextVSyncTickTime - > 10ms? I'm not sure that's ideal. One of the sites this optimization is aimed for (JSGameBench) tries to run at a constant 30 fps. That means that by the time we get around to running cheap tasks the "next" vsync is already gone. Perhaps we could try to estimate where we are in a vsync period and only allow this work during the first part, although I can see that becoming very unpredictable. I think allowing 6 ms of cheap tasks per tile management operation is a reasonable compromise. > You'll also need to set something like check_for_completed_tasks_time_ in > ScheduleCheckForCompletedTasks and use that to determine when > CheckForCompletedTasks() needs to be called. > > When reaching check_for_completed_tasks_time_ you can probably just make the > call to CheckForCompletedTasks() and then keep running more cheap tasks until > the deadline. Done. https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode358 cc/worker_pool.cc:358: task->DidComplete(); On 2013/02/14 21:10:49, David Reveman wrote: > Can we have this DidComplete call be done by CheckForCompletedTasks instead? > That makes the flow more like the tasks that run on worker threads and if we > ever need to add > WorkerPoolClient::WillStartDispatchingWorkerPoolCompletionCallbacks() we could > do that. Good idea, I've moved the DidComplete() check to CheckForCompletedTasks(). However, I think we should also call CheckCompletedTasks() in this function when we're done; otherwise a cheap task will always take ~6 ms to complete. Since the texture upload is only kicked off in the completion callback, this delay seems excessive. What do you think? https://codereview.chromium.org/12194015/diff/35001/cc/worker_pool.cc#newcode370 cc/worker_pool.cc:370: task->DeferToThread(worker); On 2013/02/14 21:10:49, David Reveman wrote: > move DeferToThread into PostTask if possible. Done.
This fixes a problem with the previous patch where it'd access already freed pictures because RunCheapTasks() executes outside the stack frame of ManageTiles(). It makes a clone of the picture even if it belongs to a cheap task. However, this introduces a performance issue when we first schedule a cheap task and later change our minds and chuck it to the thread pool. This requires making a second clone, and since the first clone doesn't have a clone cache (PicturePileImpl::clones_), we need to construct the second clone from scratch -- which can take 0.5-1 ms. The cheap tasks aren't looking so cheap any more and I'm not sure how to fix that. I think I'd like to have RunCheapTasks() called from TileManager again and have a contract that cheap tasks will only run within that frame or get explicitly deferred. Any other suggestions?
Here's my attempt to fix the above problem. TileManager calls ScheduleCheapTasks() and there's no cloning involved. CheckForCompletedTasks() runs between cheap rasters if needed. PTAL.
It's not clear to me why we were using already freed pictures in the previous patch. I don't think ManagedTiles provide anymore security here and we're already accessing picture piles outside the ManagedTiles stack frame. Can you describe the events that cause this inappropriate usage of freed pictures?
On 2013/02/16 00:41:46, David Reveman wrote: > It's not clear to me why we were using already freed pictures in the previous > patch. I don't think ManagedTiles provide anymore security here and we're > already accessing picture piles outside the ManagedTiles stack frame. Can you > describe the events that cause this inappropriate usage of freed pictures? The difference is that normal raster tasks access a refcounted clone of the picture pile, whereas cheap raster tasks access a raw pointer to the original picture pile (from Tile::picture_pile()). If the tile gets a new picture pile -- say, from PictureLayerTiling::DidBecomeActive -- the previous pile will be deleted. The clones stay valid even after the parent pile is deleted, but the raw pointer obviously does not. One way to fix this is to make a clone for the cheap tasks too, but the next problem is then deferring a cheap task into a normal task; we'd need another clone of the picture pile for the worker thread. Problem is, PicturePileImpl::GetCloneForDrawingOnThread uses a cache of clones which itself isn't cloned (and we can't touch the original picture), so we'd need to create a new clone every time (~0.5 ms on Nexus 10). I'm not sure I understand the need to create a separate picture pile clone for each worker thread; rasterizing a picture shouldn't change the picture itself in any way, right? If it turns out we only need *a* clone to hang on to a reference on the cc thread the problem would be much simplified. Alternatively we could make the clone cache transitive, but that's starting to feel like over engineering. Maybe we should move the cheap rasters back to TileManager to keep things simple?
Recap from vc discussion. - Cheap tasks keeping a ref to the picture pile should be enough. - Lets try to only process raster task completions at the current fixed interval even when it comes to cheap tasks and if this is not good enough maybe the interval need to change or be more dynamic. - Hard-coding 6ms allowed running of cheap task per frame is good enough as a start but we should probably follow up with some changes to make this vsync aware before turning it on by default.
Thanks for walking through the issues with me David. This version should match your summary. PTAL.
This looks great. Just a few minor things before this is ready to land. https://codereview.chromium.org/12194015/diff/46002/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/46002/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:58: Worker* worker = GetWorkerForNextTask(); Posting directly to a worker makes less sense now that we have WillRunOnThread(). How about we add a protected PostTask(internal::WorkerPoolTask* task, bool is_cheap) function to the WorkerPool class and just use that from here instead. GetWorkerForNextTask(), CanPostCheapTask() and PostCheapTask() can all be private to the WorkerPool class and you can probably unify some of the cheap/non-cheap task code in the WorkerPool class that way too. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:680: break; hm, this makes us enter the loop below in cases where we didn't before. is that ok? I think we should probably avoid that in this patch unless there's a good reason for it. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:969: bool is_predicted_cheap = picture_pile->IsCheapInRect(rect, contents_scale); nit: make this line <= 80 characters long https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.h#newcode174 cc/tile_manager.h:174: void OnRasterCompleted( we don't need OnRasterCompleted anymore, right? please remove it if not needed. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.h#newcode196 cc/tile_manager.h:196: static void PerformImageDecode(skia::LazyPixelRef* pixel_ref, Lets change PerformRaster back to RunRasterTask instead.
Sorry there's a lot of chatter here that I haven't read. Can someone update me where we stand relative to my comments way back in #18?
Thanks David, one more look? > Sorry there's a lot of chatter here that I haven't read. > Can someone update me where we stand relative to my > comments way back in #18? The latest patch should be equivalent to what you suggested except the queueing and execution of cheap tasks is now done inside WorkerPool. https://codereview.chromium.org/12194015/diff/46002/cc/raster_worker_pool.cc File cc/raster_worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/46002/cc/raster_worker_pool.cc#... cc/raster_worker_pool.cc:58: Worker* worker = GetWorkerForNextTask(); On 2013/02/19 18:05:23, David Reveman wrote: > Posting directly to a worker makes less sense now that we have > WillRunOnThread(). How about we add a protected > PostTask(internal::WorkerPoolTask* task, bool is_cheap) function to the > WorkerPool class and just use that from here instead. GetWorkerForNextTask(), > CanPostCheapTask() and PostCheapTask() can all be private to the WorkerPool > class and you can probably unify some of the cheap/non-cheap task code in the > WorkerPool class that way too. Good idea, done. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.cc File cc/tile_manager.cc (right): https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:680: break; On 2013/02/19 18:05:23, David Reveman wrote: > hm, this makes us enter the loop below in cases where we didn't before. is that > ok? I think we should probably avoid that in this patch unless there's a good > reason for it. You're right. We only need to make sure the allow_cheap_tasks_ flag is properly reset and we can do it in a simpler way. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.cc#newcod... cc/tile_manager.cc:969: bool is_predicted_cheap = picture_pile->IsCheapInRect(rect, contents_scale); On 2013/02/19 18:05:23, David Reveman wrote: > nit: make this line <= 80 characters long Whoops, done. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.h File cc/tile_manager.h (right): https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.h#newcode174 cc/tile_manager.h:174: void OnRasterCompleted( On 2013/02/19 18:05:23, David Reveman wrote: > we don't need OnRasterCompleted anymore, right? please remove it if not needed. Done. https://codereview.chromium.org/12194015/diff/46002/cc/tile_manager.h#newcode196 cc/tile_manager.h:196: static void PerformImageDecode(skia::LazyPixelRef* pixel_ref, On 2013/02/19 18:05:23, David Reveman wrote: > Lets change PerformRaster back to RunRasterTask instead. Done.
just one more thing. https://codereview.chromium.org/12194015/diff/52002/cc/worker_pool.cc File cc/worker_pool.cc (right): https://codereview.chromium.org/12194015/diff/52002/cc/worker_pool.cc#newcode108 cc/worker_pool.cc:108: worker_pool_->WillPostTask(); Lets remove WorkerPool::WillPostTask() and do this work in the new WorkerPool::PostTask() function instead. https://codereview.chromium.org/12194015/diff/52002/cc/worker_pool.cc#newcode364 cc/worker_pool.cc:364: ScheduleCheckForCompletedTasks(); move ScheduleCheckForCompletedTasks() call to PostTask() after removing WillPostTask. you can remove PostCheapTask() completely and fold all this into PostTask() if you like. https://codereview.chromium.org/12194015/diff/52002/cc/worker_pool.cc#newcode403 cc/worker_pool.cc:403: worker->PostTask(task.Pass()); PostTask(task, false) here instead.
Thanks, all comments addressed. It's getting close, I can feel it :)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/12194015/55001
Message was sent while issue was closed.
Change committed as 183608 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
