Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
523243002:
cc: Generalize raster task notifications (Closed)
Descriptioncc: Generalize raster task notifications
This patch generalizes the way we handle notifications for sets of raster
tasks. Previously, this was special cases for tiles that are required for
activation. The new implementation allows arbitrary sets of task and
notificatios for them, and it makes the rasterizer and the worker pools
agnostic of the semantics of those sets (only the TileManager knows about
this).
R=reveman@chromium.org
BUG=388030
Committed: https://crrev.com/69fedbd409dbc8adb204513fe88e95c040375c55
Cr-Commit-Position: refs/heads/master@{#295392}
Patch Set 1 #
Total comments: 34
Patch Set 2 : Clean-ups #Patch Set 3 : Use generalized notifications for all tasks. #Patch Set 4 : Fix cc_perftests and C++11 issue. Remove DidFinishRunningTasks. #Patch Set 5 : Remove unused did_throttle_raster_tasks. #Patch Set 6 : Remove obsolete HasPendingTasks #
Total comments: 24
Patch Set 7 : Renaming #
Total comments: 10
Patch Set 8 : Rebase #Patch Set 9 : Removed unused ctor #
Total comments: 77
Patch Set 10 : clean-ups #
Total comments: 14
Patch Set 11 : rebase #
Total comments: 2
Patch Set 12 : more clean-ups #
Total comments: 2
Patch Set 13 : Remove support for tasks that are in no set. #
Total comments: 2
Patch Set 14 : tasks_in_set -> tasks #
Messages
Total messages: 33 (2 generated)
PTAL Comments still need to be updated / added. The notifications for "all tasks" could probably be handled by the new generalized notifications as well, but I would prefer to do this in a separate patch. Please also see inline comments for open questions. https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:662: // TODO(ernstm): add With this addition, we would wait for pending uploads to complete for all task sets. Currently we only do this for the notification for all task sets. I think we should do this (crbug.com/389256) but in a separate patch. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:112: typedef std::bitset<kMaxTaskSet> TaskSetCollection; should we call this TaskSetFlags, because it is not just used to indicate membership in a set, but also to remember for which tasks we should send notifications, etc. in the worker pools? Because it is used not only on the Items, should we move it up into cc namesapce? https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:133: class TaskSetSizes { Should we move this into cc namespace, because it is used in various places?
On 2014/08/29 23:32:11, ernstm wrote: > PTAL > > Comments still need to be updated / added. > > The notifications for "all tasks" could probably be handled by the new > generalized notifications as well, but I would prefer to do this in a separate > patch. > > Please also see inline comments for open questions. > > https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... > File cc/resources/pixel_buffer_raster_worker_pool.cc (right): > > https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... > cc/resources/pixel_buffer_raster_worker_pool.cc:662: // TODO(ernstm): add > With this addition, we would wait for pending uploads to complete for all task > sets. Currently we only do this for the notification for all task sets. I think > we should do this (crbug.com/389256) but in a separate patch. > > https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h > File cc/resources/rasterizer.h (right): > > https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... > cc/resources/rasterizer.h:112: typedef std::bitset<kMaxTaskSet> > TaskSetCollection; > should we call this TaskSetFlags, because it is not just used to indicate > membership in a set, but also to remember for which tasks we should send > notifications, etc. in the worker pools? Because it is used not only on the > Items, should we move it up into cc namesapce? > > https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... > cc/resources/rasterizer.h:133: class TaskSetSizes { > Should we move this into cc namespace, because it is used in various places? ping
Sorry for the delay. I suspect this could be made cleaner by including "all tasks" in this patch. I think the naming used for "all tasks" in a lot of cases is what we want for this general mechanism but the existence of this old implementation is preventing us from using it. https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:76: std::vector<scoped_refptr<RasterizerTask> > new_task_set_finished_tasks( kMaxTaskSet or "kMaxTaskSet + 1"? you'll probably see a perftest regression here unless you use an array as all this RasterWorkerPool code has been tuned to do as few heap allocations as possible with each call to ScheduleTasks. https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.h:68: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; I think you can just use an array here instead. https://codereview.chromium.org/523243002/diff/1/cc/resources/image_copy_rast... File cc/resources/image_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_copy_rast... cc/resources/image_copy_raster_worker_pool.cc:89: kMaxTaskSet); array. kMaxTaskSet + 1? https://codereview.chromium.org/523243002/diff/1/cc/resources/image_copy_rast... File cc/resources/image_copy_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_copy_rast... cc/resources/image_copy_raster_worker_pool.h:106: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; array https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... cc/resources/image_raster_worker_pool.cc:70: kMaxTaskSet); array. kMaxTaskSet + 1? https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... File cc/resources/image_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... cc/resources/image_raster_worker_pool.h:68: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; array https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:203: // No longer required for activation. this comment need to change. https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:357: client_->ShouldForceTaskSetToComplete(task_set); How about we rename this client function to TasksThatShouldBeForcedToComplete and have it return a TaskSetCollection? I think that will clean up this code a bit. https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.h:132: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; array https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool.cc:110: unsigned RasterWorkerPool::kRasterRequiredForActivationFinishedTaskPriority = remove https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool.h:21: static unsigned kRasterRequiredForActivationFinishedTaskPriority; don't need this anymore https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:95: static const size_t kMaxTaskSet = 2; This is really number of task sets. Either set it to 1 or change the name to something like kNumberOfTaskSet. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:104: TaskSet task_set) const = 0; Not a fan of how much influence this synthetic delay debugging stuff has on the design. Is there any way we could hide all this in one place and not be part of the RasterizeClient API. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:112: typedef std::bitset<kMaxTaskSet> TaskSetCollection; On 2014/08/29 23:32:10, ernstm wrote: > should we call this TaskSetFlags, because it is not just used to indicate > membership in a set, but also to remember for which tasks we should send > notifications, etc. in the worker pools? Because it is used not only on the > Items, should we move it up into cc namesapce? Moving it up next to the TaskSet typedef sgtm. I would keep the current name though. TaskSetCollection name still makes sense to me when used for other things. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:150: bool VerifyTaskSetSizes() const; I'd prefer if this was not part of the API as it's just for internal debug checks. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:155: TaskSetSizes task_set_sizes; I think we can remove this and make it internal to RasterWorkerPool implementations instead. They all iterate over this in ScheduleTasks so they can compute it there instead and not having all this here would be much cleaner.
I'll do the "all tasks notification" in a separate patch set (but same patch). https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.cc:76: std::vector<scoped_refptr<RasterizerTask> > new_task_set_finished_tasks( On 2014/09/05 08:38:59, reveman wrote: > kMaxTaskSet or "kMaxTaskSet + 1"? you'll probably see a perftest regression here > unless you use an array as all this RasterWorkerPool code has been tuned to do > as few heap allocations as possible with each call to ScheduleTasks. Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... File cc/resources/gpu_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/gpu_raster_work... cc/resources/gpu_raster_worker_pool.h:68: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; On 2014/09/05 08:38:59, reveman wrote: > I think you can just use an array here instead. Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/image_copy_rast... File cc/resources/image_copy_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_copy_rast... cc/resources/image_copy_raster_worker_pool.h:106: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; On 2014/09/05 08:38:59, reveman wrote: > array Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... cc/resources/image_raster_worker_pool.cc:70: kMaxTaskSet); On 2014/09/05 08:38:59, reveman wrote: > array. kMaxTaskSet + 1? Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... File cc/resources/image_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/image_raster_wo... cc/resources/image_raster_worker_pool.h:68: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; On 2014/09/05 08:38:59, reveman wrote: > array Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:203: // No longer required for activation. On 2014/09/05 08:38:59, reveman wrote: > this comment need to change. Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:357: client_->ShouldForceTaskSetToComplete(task_set); On 2014/09/05 08:38:59, reveman wrote: > How about we rename this client function to TasksThatShouldBeForcedToComplete > and have it return a TaskSetCollection? I think that will clean up this code a > bit. Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.h:132: std::vector<scoped_refptr<RasterizerTask> > task_set_finished_tasks_; On 2014/09/05 08:38:59, reveman wrote: > array Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool.cc:110: unsigned RasterWorkerPool::kRasterRequiredForActivationFinishedTaskPriority = On 2014/09/05 08:38:59, reveman wrote: > remove Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool.h:21: static unsigned kRasterRequiredForActivationFinishedTaskPriority; On 2014/09/05 08:38:59, reveman wrote: > don't need this anymore Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:95: static const size_t kMaxTaskSet = 2; On 2014/09/05 08:38:59, reveman wrote: > This is really number of task sets. Either set it to 1 or change the name to > something like kNumberOfTaskSet. Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:104: TaskSet task_set) const = 0; On 2014/09/05 08:38:59, reveman wrote: > Not a fan of how much influence this synthetic delay debugging stuff has on the > design. Is there any way we could hide all this in one place and not be part of > the RasterizeClient API. I don't think so, because now only the TileManager knows the semantics of the TaskSets and you need that knowledge to determine the delay. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:112: typedef std::bitset<kMaxTaskSet> TaskSetCollection; On 2014/09/05 08:38:59, reveman wrote: > On 2014/08/29 23:32:10, ernstm wrote: > > should we call this TaskSetFlags, because it is not just used to indicate > > membership in a set, but also to remember for which tasks we should send > > notifications, etc. in the worker pools? Because it is used not only on the > > Items, should we move it up into cc namesapce? > > Moving it up next to the TaskSet typedef sgtm. I would keep the current name > though. TaskSetCollection name still makes sense to me when used for other > things. Done. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:150: bool VerifyTaskSetSizes() const; On 2014/09/05 08:38:59, reveman wrote: > I'd prefer if this was not part of the API as it's just for internal debug > checks. Done. We don't need it anymore, if TaskSetSizes is removed from the queue. https://codereview.chromium.org/523243002/diff/1/cc/resources/rasterizer.h#ne... cc/resources/rasterizer.h:155: TaskSetSizes task_set_sizes; On 2014/09/05 08:38:59, reveman wrote: > I think we can remove this and make it internal to RasterWorkerPool > implementations instead. They all iterate over this in ScheduleTasks so they can > compute it there instead and not having all this here would be much cleaner. Done. Although I'm not sure if this is the best trade-off. We now have to collect that information in ScheduleTasks _before_ we iterate over all the items, which means we iterate the queue twice. This is necessary, because we need to create the RasterFinishedTask before iterating the queue, and we need to know the sizes of the task sets at creation of the RasterFinishedTask to determine if a synthetic delay needs to be added. With the TaskSetSize on the queue, we were able to compute the size of the sets as a byproduct of adding items.
The latest version uses the generalized notifications for the all-tasks-finished case as well. Please take another look.
I've only had time to review GpuRasterWorkerPool so far but I assume that my comments most likely apply to other worker pool implementations as well. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:63: raster_task_sets_pending_.set(); is it worth adding a comment here that makes it clear that this marks all task sets as pending? https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:74: TaskSetSizes task_set_sizes(queue); We wouldn't need this if not for the synthetic delays. I think we need a better way to handle this. How about we implement this delay in the tile manager by delaying the actual raster tasks required by activation instead? We can land a patch that changes this prior to this patch and you simply don't have to deal with it here. We can then replace "TaskSetSizes task_set_sizes" with "size_t task_counts[lNumberOfTaskSets]" and compute it as we add tasks in the loop below. Wdyt? https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:97: if (!it->task_sets[task_set]) use item.task_sets instead of it->task_sets https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:120: task_set_finished_tasks_[task_set] = new_task_set_finished_tasks[task_set]; Maybe have |task_set_finished_tasks_| be a std::vector but keep new_task_set_finished_tasks an array so you could use std::copy here instead of a for loop. Current code is fine if you prefer it. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:8: #include <vector> Don't need this. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:47: void OnRasterTaskSetFinished(TaskSet task_set); how about just OnRasterFinished(TaskSet task_set) now that there are no other RasterFinished signals? https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:61: TaskSetCollection raster_task_sets_pending_; raster_tasks_pending_ or simply raster_pending_? Type type makes it clear that this is in terms of task sets. Wdyt? https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:65: scoped_refptr<RasterizerTask> task_set_finished_tasks_[kNumberOfTaskSets]; and you could use raster_finished_tasks_ here. https://codereview.chromium.org/523243002/diff/100001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:21: static unsigned kRasterTaskSetFinishedTaskPriority; I think you can just remove this and use kRasterFinishedTaskPriority instead
https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:63: raster_task_sets_pending_.set(); On 2014/09/10 16:41:49, reveman wrote: > is it worth adding a comment here that makes it clear that this marks all task > sets as pending? Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:74: TaskSetSizes task_set_sizes(queue); On 2014/09/10 16:41:49, reveman wrote: > We wouldn't need this if not for the synthetic delays. I think we need a better > way to handle this. How about we implement this delay in the tile manager by > delaying the actual raster tasks required by activation instead? We can land a > patch that changes this prior to this patch and you simply don't have to deal > with it here. > > We can then replace "TaskSetSizes task_set_sizes" with "size_t > task_counts[lNumberOfTaskSets]" and compute it as we add tasks in the loop > below. > > Wdyt? I'd prefer to do this as a follow-up. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:97: if (!it->task_sets[task_set]) On 2014/09/10 16:41:49, reveman wrote: > use item.task_sets instead of it->task_sets Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:120: task_set_finished_tasks_[task_set] = new_task_set_finished_tasks[task_set]; On 2014/09/10 16:41:49, reveman wrote: > Maybe have |task_set_finished_tasks_| be a std::vector but keep > new_task_set_finished_tasks an array so you could use std::copy here instead of > a for loop. Current code is fine if you prefer it. We can use std::copy on two arrays as well. I like the arrays a bit better for this case, so I kept them and just switched to std::copy. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:8: #include <vector> On 2014/09/10 16:41:50, reveman wrote: > Don't need this. Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:47: void OnRasterTaskSetFinished(TaskSet task_set); On 2014/09/10 16:41:49, reveman wrote: > how about just OnRasterFinished(TaskSet task_set) now that there are no other > RasterFinished signals? Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:61: TaskSetCollection raster_task_sets_pending_; On 2014/09/10 16:41:50, reveman wrote: > raster_tasks_pending_ or simply raster_pending_? Type type makes it clear that > this is in terms of task sets. Wdyt? Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.h:65: scoped_refptr<RasterizerTask> task_set_finished_tasks_[kNumberOfTaskSets]; On 2014/09/10 16:41:49, reveman wrote: > and you could use raster_finished_tasks_ here. Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:21: static unsigned kRasterTaskSetFinishedTaskPriority; On 2014/09/10 16:41:50, reveman wrote: > I think you can just remove this and use kRasterFinishedTaskPriority instead Done. https://codereview.chromium.org/523243002/diff/100001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/rasterizer... cc/resources/rasterizer.h:101: virtual void DidFinishRunningTaskSet(TaskSet task_set) = 0; I also renamed this to DidFinishRunningTasks(TaskSet task_set) to be consistent with the other naming changes. https://codereview.chromium.org/523243002/diff/100001/cc/resources/rasterizer... cc/resources/rasterizer.h:103: virtual base::debug::TraceEventSyntheticDelay* SyntheticDelayForTaskSet( and renamed this to SyntheticDelayForTasks(TaskSet task_set)
https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:74: TaskSetSizes task_set_sizes(queue); On 2014/09/10 18:26:18, ernstm wrote: > On 2014/09/10 16:41:49, reveman wrote: > > We wouldn't need this if not for the synthetic delays. I think we need a > better > > way to handle this. How about we implement this delay in the tile manager by > > delaying the actual raster tasks required by activation instead? We can land a > > patch that changes this prior to this patch and you simply don't have to deal > > with it here. > > > > We can then replace "TaskSetSizes task_set_sizes" with "size_t > > task_counts[lNumberOfTaskSets]" and compute it as we add tasks in the loop > > below. > > > > Wdyt? > > I'd prefer to do this as a follow-up. I have to insist on doing this prior to this patch unless you see a problem with my proposal. This patch is going to include awkward rasterizer API, more complexity and more code unless we do this refactor first. I can take a look at getting this done if you like? Or maybe you can ask Sami who added the original SyntheticDelay support to raster tasks. https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:120: task_set_finished_tasks_[task_set] = new_task_set_finished_tasks[task_set]; On 2014/09/10 18:26:18, ernstm wrote: > On 2014/09/10 16:41:49, reveman wrote: > > Maybe have |task_set_finished_tasks_| be a std::vector but keep > > new_task_set_finished_tasks an array so you could use std::copy here instead > of > > a for loop. Current code is fine if you prefer it. > > We can use std::copy on two arrays as well. I like the arrays a bit better for > this case, so I kept them and just switched to std::copy. Great. I didn't know std::copy worked with plain old arrays. https://codereview.chromium.org/523243002/diff/120001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:99: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { As the intent of this loop is to iterate over all task sets that the task is part of, adding a TaskSetIterator helper class to RasterWorkerPool that does this might be worthwhile. https://codereview.chromium.org/523243002/diff/120001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/rasterizer... cc/resources/rasterizer.h:128: TaskSetCollection task_sets; Should it be allowed to have a task not be part of any task set? I assuming, yes.
https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:74: TaskSetSizes task_set_sizes(queue); On 2014/09/10 19:41:27, reveman wrote: > On 2014/09/10 18:26:18, ernstm wrote: > > On 2014/09/10 16:41:49, reveman wrote: > > > We wouldn't need this if not for the synthetic delays. I think we need a > > better > > > way to handle this. How about we implement this delay in the tile manager by > > > delaying the actual raster tasks required by activation instead? We can land > a > > > patch that changes this prior to this patch and you simply don't have to > deal > > > with it here. > > > > > > We can then replace "TaskSetSizes task_set_sizes" with "size_t > > > task_counts[lNumberOfTaskSets]" and compute it as we add tasks in the loop > > > below. > > > > > > Wdyt? > > > > I'd prefer to do this as a follow-up. > > I have to insist on doing this prior to this patch unless you see a problem with > my proposal. This patch is going to include awkward rasterizer API, more > complexity and more code unless we do this refactor first. I can take a look at > getting this done if you like? Or maybe you can ask Sami who added the original > SyntheticDelay support to raster tasks. Could we simply put the delay into the REQUIRED _FOR_ACTIVATION branch of TileManger::DidFinishRunningTasks?
https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:74: TaskSetSizes task_set_sizes(queue); On 2014/09/10 20:48:06, ernstm wrote: > On 2014/09/10 19:41:27, reveman wrote: > > On 2014/09/10 18:26:18, ernstm wrote: > > > On 2014/09/10 16:41:49, reveman wrote: > > > > We wouldn't need this if not for the synthetic delays. I think we need a > > > better > > > > way to handle this. How about we implement this delay in the tile manager > by > > > > delaying the actual raster tasks required by activation instead? We can > land > > a > > > > patch that changes this prior to this patch and you simply don't have to > > deal > > > > with it here. > > > > > > > > We can then replace "TaskSetSizes task_set_sizes" with "size_t > > > > task_counts[lNumberOfTaskSets]" and compute it as we add tasks in the loop > > > > below. > > > > > > > > Wdyt? > > > > > > I'd prefer to do this as a follow-up. > > > > I have to insist on doing this prior to this patch unless you see a problem > with > > my proposal. This patch is going to include awkward rasterizer API, more > > complexity and more code unless we do this refactor first. I can take a look > at > > getting this done if you like? Or maybe you can ask Sami who added the > original > > SyntheticDelay support to raster tasks. > > Could we simply put the delay into the REQUIRED _FOR_ACTIVATION branch of > TileManger::DidFinishRunningTasks? I don't think so. That will cause us to block on the compositor thread instead of a worker thread. Not sure exactly how these delays are supposed to be used but it seems like we'd ideally begin the delay timer when scheduling tasks and wait for the delay when running the last raster task required for activation. That would probably be more correct than our current behavior of just always delaying the "finished" task by independent of how long it actually took to run the real raster tasks.
ernstm@chromium.org changed reviewers: + skyostil@chromium.org
On 2014/09/10 21:00:52, reveman wrote: > https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... > File cc/resources/gpu_raster_worker_pool.cc (right): > > https://codereview.chromium.org/523243002/diff/100001/cc/resources/gpu_raster... > cc/resources/gpu_raster_worker_pool.cc:74: TaskSetSizes task_set_sizes(queue); > On 2014/09/10 20:48:06, ernstm wrote: > > On 2014/09/10 19:41:27, reveman wrote: > > > On 2014/09/10 18:26:18, ernstm wrote: > > > > On 2014/09/10 16:41:49, reveman wrote: > > > > > We wouldn't need this if not for the synthetic delays. I think we need a > > > > better > > > > > way to handle this. How about we implement this delay in the tile > manager > > by > > > > > delaying the actual raster tasks required by activation instead? We can > > land > > > a > > > > > patch that changes this prior to this patch and you simply don't have to > > > deal > > > > > with it here. > > > > > > > > > > We can then replace "TaskSetSizes task_set_sizes" with "size_t > > > > > task_counts[lNumberOfTaskSets]" and compute it as we add tasks in the > loop > > > > > below. > > > > > > > > > > Wdyt? > > > > > > > > I'd prefer to do this as a follow-up. > > > > > > I have to insist on doing this prior to this patch unless you see a problem > > with > > > my proposal. This patch is going to include awkward rasterizer API, more > > > complexity and more code unless we do this refactor first. I can take a look > > at > > > getting this done if you like? Or maybe you can ask Sami who added the > > original > > > SyntheticDelay support to raster tasks. > > > > Could we simply put the delay into the REQUIRED _FOR_ACTIVATION branch of > > TileManger::DidFinishRunningTasks? > > I don't think so. That will cause us to block on the compositor thread instead > of a worker thread. > > Not sure exactly how these delays are supposed to be used but it seems like we'd > ideally begin the delay timer when scheduling tasks and wait for the delay when > running the last raster task required for activation. That would probably be > more correct than our current behavior of just always delaying the "finished" > task by independent of how long it actually took to run the real raster tasks. +skyostil
On 2014/09/10 23:25:11, ernstm wrote: > I don't think so. That will cause us to block on the compositor thread instead > of a worker thread. Right, the compositor should be able to run freely while the delay is being applied. > > Not sure exactly how these delays are supposed to be used but it seems like > we'd > > ideally begin the delay timer when scheduling tasks and wait for the delay > when > > running the last raster task required for activation. That would probably be > > more correct than our current behavior of just always delaying the "finished" > > task by independent of how long it actually took to run the real raster tasks. Yes, that's what would happen ideally: we want for the total raster time to be at least a given constant. I think applying the delay only in the finished task would work too because generally the delay >> raster time. However, I remember running into problems with that approach because the "finished" task is always scheduled regardless of whether we did any raster work. Maybe that has changed since? FWIW, you can see these delays in action by running: $ tools/perf/run_benchmark scheduler.tough_scheduling_cases --browser=release --page-filter=raster
On 2014/09/11 12:18:00, Sami wrote:
> On 2014/09/10 23:25:11, ernstm wrote:
> > I don't think so. That will cause us to block on the compositor thread
instead
> > of a worker thread.
>
> Right, the compositor should be able to run freely while the delay is being
> applied.
>
> > > Not sure exactly how these delays are supposed to be used but it seems
like
> > we'd
> > > ideally begin the delay timer when scheduling tasks and wait for the delay
> > when
> > > running the last raster task required for activation. That would probably
be
> > > more correct than our current behavior of just always delaying the
> "finished"
> > > task by independent of how long it actually took to run the real raster
> tasks.
>
> Yes, that's what would happen ideally: we want for the total raster time to be
> at
> least a given constant.
Ok, thanks. This is good to know as we refactor this.
>
> I think applying the delay only in the finished task would work too because
> generally the delay >> raster time. However, I remember running into problems
> with
> that approach because the "finished" task is always scheduled regardless of
> whether
> we did any raster work. Maybe that has changed since?
That's still the case. I think with the current behavior where we only apply the
delay when running 1+ tasks is not always working as intended. It can sometimes
fail to apply the delay. ie.
TileManger::ManageTiles()
ScheduleTasks(10_tasks_raster)
a "finished" task with a delay is scheduled (this is internally task 11)
The worker thread pool manages to run 10 tasks, the raster tasks. but not task
11 the "finished" task.
TileManger::ManageTiles()
CheckForCompletedTask()
tile manager notice that 10 raster tasks are done
ScheduleTasks(0_tasks_raster)
old "finished" is cancelled and new task "finish" without a delay is
scheduled
Tile manager is notified that tasks have finished without any delay having been
applied. It's probably rare that this will happen but it's something we should
keep in mind and fix as part of a refactor.
>
> FWIW, you can see these delays in action by running:
>
> $ tools/perf/run_benchmark scheduler.tough_scheduling_cases --browser=release
> --page-filter=raster
Cool, thanks.
https://codereview.chromium.org/523243002/diff/120001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:99: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/10 19:41:27, reveman wrote: > As the intent of this loop is to iterate over all task sets that the task is > part of, adding a TaskSetIterator helper class to RasterWorkerPool that does > this might be worthwhile. std::bitset doesn't have iterators, and I think adding a custom iterator makes the code less readable, because it will always be slightly different from a proper STL iterator. https://codereview.chromium.org/523243002/diff/120001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/rasterizer... cc/resources/rasterizer.h:128: TaskSetCollection task_sets; On 2014/09/10 19:41:28, reveman wrote: > Should it be allowed to have a task not be part of any task set? I assuming, > yes. Yes, and we do have that situation. See pixel_buffer_raster_worker_pool.cc:198.
On 2014/09/11 16:18:38, reveman wrote: > On 2014/09/11 12:18:00, Sami wrote: > > On 2014/09/10 23:25:11, ernstm wrote: > > > I don't think so. That will cause us to block on the compositor thread > instead > > > of a worker thread. > > > > Right, the compositor should be able to run freely while the delay is being > > applied. > > > > > > Not sure exactly how these delays are supposed to be used but it seems > like > > > we'd > > > > ideally begin the delay timer when scheduling tasks and wait for the delay > > > when > > > > running the last raster task required for activation. That would probably > be > > > > more correct than our current behavior of just always delaying the > > "finished" > > > > task by independent of how long it actually took to run the real raster > > tasks. > > > > Yes, that's what would happen ideally: we want for the total raster time to be > > at > > least a given constant. > > Ok, thanks. This is good to know as we refactor this. > > > > > I think applying the delay only in the finished task would work too because > > generally the delay >> raster time. However, I remember running into problems > > with > > that approach because the "finished" task is always scheduled regardless of > > whether > > we did any raster work. Maybe that has changed since? > > That's still the case. I think with the current behavior where we only apply the > delay when running 1+ tasks is not always working as intended. It can sometimes > fail to apply the delay. ie. > > TileManger::ManageTiles() > ScheduleTasks(10_tasks_raster) > a "finished" task with a delay is scheduled (this is internally task 11) > > The worker thread pool manages to run 10 tasks, the raster tasks. but not task > 11 the "finished" task. > > TileManger::ManageTiles() > CheckForCompletedTask() > tile manager notice that 10 raster tasks are done > ScheduleTasks(0_tasks_raster) > old "finished" is cancelled and new task "finish" without a delay is > scheduled > > Tile manager is notified that tasks have finished without any delay having been > applied. It's probably rare that this will happen but it's something we should > keep in mind and fix as part of a refactor. > > > > > FWIW, you can see these delays in action by running: > > > > $ tools/perf/run_benchmark scheduler.tough_scheduling_cases --browser=release > > --page-filter=raster > > Cool, thanks. In an offline discussion with Dave, we decided to split out the SyntheticDelay clean-up into a separate follow-up patch. Can we agree on the following temporary solution, which should be easy to remove/change in the follow-up patch: We count the size of the task sets on the RasterTaskQueue (we did that before). This way we avoid iterating over the queue a second time in the worker pools. To configure the delays, we could either use the solution currently in this patch (query RasterizerClient::SyntheticDelayForTasks from the RasterWorkerPools), or set the Delay from TileManager through RasterWorkerPool::SetSyntheticDelayForTasks. Both approaches are symmetric and similar in terms of impact on the interface. I leave it to Dave to decide which one he likes better as a temporary solution.
On 2014/09/11 22:35:02, ernstm wrote: > On 2014/09/11 16:18:38, reveman wrote: > > On 2014/09/11 12:18:00, Sami wrote: > > > On 2014/09/10 23:25:11, ernstm wrote: > > > > I don't think so. That will cause us to block on the compositor thread > > instead > > > > of a worker thread. > > > > > > Right, the compositor should be able to run freely while the delay is being > > > applied. > > > > > > > > Not sure exactly how these delays are supposed to be used but it seems > > like > > > > we'd > > > > > ideally begin the delay timer when scheduling tasks and wait for the > delay > > > > when > > > > > running the last raster task required for activation. That would > probably > > be > > > > > more correct than our current behavior of just always delaying the > > > "finished" > > > > > task by independent of how long it actually took to run the real raster > > > tasks. > > > > > > Yes, that's what would happen ideally: we want for the total raster time to > be > > > at > > > least a given constant. > > > > Ok, thanks. This is good to know as we refactor this. > > > > > > > > I think applying the delay only in the finished task would work too because > > > generally the delay >> raster time. However, I remember running into > problems > > > with > > > that approach because the "finished" task is always scheduled regardless of > > > whether > > > we did any raster work. Maybe that has changed since? > > > > That's still the case. I think with the current behavior where we only apply > the > > delay when running 1+ tasks is not always working as intended. It can > sometimes > > fail to apply the delay. ie. > > > > TileManger::ManageTiles() > > ScheduleTasks(10_tasks_raster) > > a "finished" task with a delay is scheduled (this is internally task 11) > > > > The worker thread pool manages to run 10 tasks, the raster tasks. but not task > > 11 the "finished" task. > > > > TileManger::ManageTiles() > > CheckForCompletedTask() > > tile manager notice that 10 raster tasks are done > > ScheduleTasks(0_tasks_raster) > > old "finished" is cancelled and new task "finish" without a delay is > > scheduled > > > > Tile manager is notified that tasks have finished without any delay having > been > > applied. It's probably rare that this will happen but it's something we should > > keep in mind and fix as part of a refactor. > > > > > > > > FWIW, you can see these delays in action by running: > > > > > > $ tools/perf/run_benchmark scheduler.tough_scheduling_cases > --browser=release > > > --page-filter=raster > > > > Cool, thanks. > > In an offline discussion with Dave, we decided to split out the SyntheticDelay > clean-up into a separate follow-up patch. Can we agree on the following > temporary solution, which should be easy to remove/change in the follow-up > patch: We count the size of the task sets on the RasterTaskQueue (we did that > before). This way we avoid iterating over the queue a second time in the worker > pools. I'm ok with that part. > To configure the delays, we could either use the solution currently in > this patch (query RasterizerClient::SyntheticDelayForTasks from the > RasterWorkerPools), or set the Delay from TileManager through > RasterWorkerPool::SetSyntheticDelayForTasks. Both approaches are symmetric and > similar in terms of impact on the interface. I leave it to Dave to decide which > one he likes better as a temporary solution. Neh, if we're not going to do this properly then let's not add any Rasterizer or RasterWorkerPool API. Either change the name of the synthetic delay from cc.RasterRequiredForActivation to cc.RasterTaskSet1. Or just assume that task_set=1 should use cc.RasterRequiredForActivation in the rasterworkerpool implementation. The second suggestion is of course super ugly but if there's an urgency to land this prior to fixing and cleaning up the synthetic delays then I prefer the uglier but isolated to one place approach. FYI, I haven't looked at the TaskSetSizes class in detail yet as I was expecting that it could be removed once our handling of synthetic delays had been cleaned up. I'll take a look at this asap now that you're interested in keeping the counting and I assume you're interested in keeping this class.
> FYI, I haven't looked at the TaskSetSizes class in detail yet as I was expecting > that it could be removed once our handling of synthetic delays had been cleaned > up. I'll take a look at this asap now that you're interested in keeping the > counting and I assume you're interested in keeping this class. Yes, please review it. We need the TaskSetSizes for other purposes as well, for example in PixelBufferRasterWorkerPool::HasPendingTasks.
https://codereview.chromium.org/523243002/diff/120001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:99: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/11 21:15:53, ernstm wrote: > On 2014/09/10 19:41:27, reveman wrote: > > As the intent of this loop is to iterate over all task sets that the task is > > part of, adding a TaskSetIterator helper class to RasterWorkerPool that does > > this might be worthwhile. > > std::bitset doesn't have iterators, and I think adding a custom iterator makes > the code less readable, because it will always be slightly different from a > proper STL iterator. While it might not make sense for std::bitset to have an iterator, TaskSetColletion having one would make a lot of sense to me. We have our own iterator classes in a lot of places in cc/ so I don't think adding another one would be bad but I'm fine leaving this code as is if you prefer. I might give the iterator a try in a follow up to see what it would look like. https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:23: class TaskSetSizes { While convenient, I find the use of this class with its overloaded operators a bit hard to read. It's fine for PBRWP as I don't see a better way to keep track of all these counts there. However, I'd like it to be constrained to that class (PixelBufferRasterWorkerPool) and simply use "size_t task_count[kNumberOfTaskSets]" elsewhere. The reasoning would be that all this counting will go away as we remove/refactor synthetic delays, except for PBRWP where we still need it (and PBRWP might just go away completely if 1-copy works out as expected). I'd rather not have it exposed through the Rasterizer API if not needed long term. I've been wanting to remove the existing count from the Rasterizer API for some time and adding something more complicated there would be sad.. https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:26: explicit TaskSetSizes(const RasterTaskQueue* queue); nit: blank line between ctor/dtor and other functions. https://codereview.chromium.org/523243002/diff/120001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (left): https://codereview.chromium.org/523243002/diff/120001/cc/resources/rasterizer... cc/resources/rasterizer.h:130: size_t required_for_activation_count; If we need to add the count back here, then please add it back as "size_t count[kNumberOfTaskSets]".
https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:23: class TaskSetSizes { On 2014/09/12 16:33:51, reveman wrote: > While convenient, I find the use of this class with its overloaded operators a > bit hard to read. It's fine for PBRWP as I don't see a better way to keep track > of all these counts there. However, I'd like it to be constrained to that class > (PixelBufferRasterWorkerPool) and simply use "size_t > task_count[kNumberOfTaskSets]" elsewhere. > > The reasoning would be that all this counting will go away as we remove/refactor > synthetic delays, except for PBRWP where we still need it (and PBRWP might just > go away completely if 1-copy works out as expected). I'd rather not have it > exposed through the Rasterizer API if not needed long term. I've been wanting to > remove the existing count from the Rasterizer API for some time and adding > something more complicated there would be sad.. Done. https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:26: explicit TaskSetSizes(const RasterTaskQueue* queue); On 2014/09/12 16:33:51, reveman wrote: > nit: blank line between ctor/dtor and other functions. Done.
On 2014/09/13 01:26:51, ernstm wrote: > https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... > File cc/resources/raster_worker_pool.h (right): > > https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... > cc/resources/raster_worker_pool.h:23: class TaskSetSizes { > On 2014/09/12 16:33:51, reveman wrote: > > While convenient, I find the use of this class with its overloaded operators a > > bit hard to read. It's fine for PBRWP as I don't see a better way to keep > track > > of all these counts there. However, I'd like it to be constrained to that > class > > (PixelBufferRasterWorkerPool) and simply use "size_t > > task_count[kNumberOfTaskSets]" elsewhere. > > > > The reasoning would be that all this counting will go away as we > remove/refactor > > synthetic delays, except for PBRWP where we still need it (and PBRWP might > just > > go away completely if 1-copy works out as expected). I'd rather not have it > > exposed through the Rasterizer API if not needed long term. I've been wanting > to > > remove the existing count from the Rasterizer API for some time and adding > > something more complicated there would be sad.. > > Done. > > https://codereview.chromium.org/523243002/diff/120001/cc/resources/raster_wor... > cc/resources/raster_worker_pool.h:26: explicit TaskSetSizes(const > RasterTaskQueue* queue); > On 2014/09/12 16:33:51, reveman wrote: > > nit: blank line between ctor/dtor and other functions. > > Done. ping
https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:80: task_count[task_set] = 0; How about "size_t task_count[kNumberOfTaskSets] = {0};" above instead? https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:95: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:108: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... File cc/resources/image_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:87: size_t task_count[kNumberOfTaskSets]; = {0} https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:107: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:120: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:231: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:235: raster_pending_[task_set]); can you use BeginArray here instead? and just replace the current "pending_count" value with this array? https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:70: size_t task_count[kNumberOfTaskSets]; = {0} https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:88: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:101: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:171: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:175: raster_pending_[task_set]); "tasks_pending" array instead? https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:322: TaskSetCollection task_sets_forced_to_complete = I prefer "tasks_that_should_be_forced_to_complete" instead of using a new name for this variable. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:339: // Force all uploads required for activation to complete. please update this comment. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:414: TaskSetCollection will_notify_client_that_no_tasks_in_set_are_pending = s/will_notify_client_that_no_tasks_in_set_are_pending/will_notify_client_that_no_tasks_are_pending/ https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:438: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:450: RasterTaskVector tasks_in_set[kNumberOfTaskSets]; Why is "RasterTaskVector tasks[kNumberOfTaskSets]" not enough? https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:457: TaskSetCollection did_throttle_raster_tasks_in_set; "did_throttle_raster_tasks" instead of adding the "_in_set" prefix? https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:532: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:672: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { nit: ++task_set https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:676: task_set_sizes_[task_set]); Use a "pending_count" array instead? https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:694: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) nit: ++task_set here and below https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:728: const TaskSetCollection& task_set_collection) { nit: s/task_set_collection/task_sets/ https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:736: const TaskSetCollection& task_set_collection) { nit: s/task_set_collection/task_sets/ https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:85: void operator+=(const TaskSetCollection& task_set_collection); nit: s/task_set_collection/task_sets/ https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:86: void operator-=(const TaskSetCollection& task_set_collection); nit: s/task_set_collection/task_sets/ https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:90: }; I failed to think of this before but a number of explicit static utility functions that operate on "size_t task_counts[kNumberOfTaskSets]" would likely make the code easier to read and that might also allow you to hide all this in the .cc file. ie. void AddTaskSetsToTaskCounts( size_t *task_counts, const TaskSetCollection& tasks_sets); https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:106: TaskSetCollection HasPendingTasks() const; Something awkward about a function named "HasPendingTasks" returning a TaskSetCollection. I think your options are: bool HasPendingTasks(TaskSet task_set), bool HasPendingTasks(const TaskSetCollection& task_sets) or TaskSetCollection PendingTasks() I prefer one of the HasPendingTasks versions if possible. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... File cc/resources/raster_worker_pool_perftest.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_perftest.cc:274: if (task_set == ALL) do we need this conditional? It's more realistic to check in all situations. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... File cc/resources/raster_worker_pool_unittest.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_unittest.cc:184: raster_worker_pool_->AsRasterizer()->CheckForCompletedTasks(); remove the conditional for this if possible. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_unittest.cc:185: base::MessageLoop::current()->Quit(); We should switch this test over to using TestSimpleTaskRunner but you can do "if (task_set == ALL) base::MessageLoop::current()->Quit();" for now unless you feel like including that change in this patch. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_unittest.cc:215: TaskSetCollection task_set_collection; nit: s/task_set_collection/task_sets/ https://codereview.chromium.org/523243002/diff/160001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/rasterizer... cc/resources/rasterizer.h:19: } no need for this anymore https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... cc/resources/tile_manager.cc:14: #include "base/lazy_instance.h" do we need this? https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... cc/resources/tile_manager.cc:544: } else if (task_set == REQUIRED_FOR_ACTIVATION) { nit: I prefer early out instead of else-ifs when possible and I think that's common in cc/. https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... cc/resources/tile_manager.h:91: HIGH_RESOLUTION_IN_NOW_BIN_ON_ACTIVE_TREE = 0, I think we should add this new task set in a follow up instead. A follow up that makes use of it rather than adding it right now without using it. https://codereview.chromium.org/523243002/diff/160001/cc/test/fake_tile_manag... File cc/test/fake_tile_manager.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/test/fake_tile_manag... cc/test/fake_tile_manager.cc:97: void FakeTileManager::DidFinishRunningTasksForTesting() { should this function take a TaskSet as parameter?
https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... File cc/resources/gpu_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:80: task_count[task_set] = 0; On 2014/09/16 22:49:03, reveman wrote: > How about "size_t task_count[kNumberOfTaskSets] = {0};" above instead? Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:95: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:03, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/gpu_raster... cc/resources/gpu_raster_worker_pool.cc:108: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:03, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... File cc/resources/image_copy_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:87: size_t task_count[kNumberOfTaskSets]; On 2014/09/16 22:49:03, reveman wrote: > = {0} Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:107: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:03, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:120: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:03, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:231: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:03, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_copy... cc/resources/image_copy_raster_worker_pool.cc:235: raster_pending_[task_set]); On 2014/09/16 22:49:03, reveman wrote: > can you use BeginArray here instead? and just replace the current > "pending_count" value with this array? Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... File cc/resources/image_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:70: size_t task_count[kNumberOfTaskSets]; On 2014/09/16 22:49:04, reveman wrote: > = {0} Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:88: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:04, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:101: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:04, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:171: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:04, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/image_rast... cc/resources/image_raster_worker_pool.cc:175: raster_pending_[task_set]); On 2014/09/16 22:49:04, reveman wrote: > "tasks_pending" array instead? Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:322: TaskSetCollection task_sets_forced_to_complete = On 2014/09/16 22:49:05, reveman wrote: > I prefer "tasks_that_should_be_forced_to_complete" instead of using a new name > for this variable. Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:339: // Force all uploads required for activation to complete. On 2014/09/16 22:49:04, reveman wrote: > please update this comment. Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:414: TaskSetCollection will_notify_client_that_no_tasks_in_set_are_pending = On 2014/09/16 22:49:05, reveman wrote: > s/will_notify_client_that_no_tasks_in_set_are_pending/will_notify_client_that_no_tasks_are_pending/ Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:450: RasterTaskVector tasks_in_set[kNumberOfTaskSets]; On 2014/09/16 22:49:04, reveman wrote: > Why is "RasterTaskVector tasks[kNumberOfTaskSets]" not enough? We can't make any assumptions about the task sets in the raster worker pools. In particular, we don't know that a set for ALL tasks exists. Theoretically, there might be tasks that are not in any task set, but we still need to schedule them. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:457: TaskSetCollection did_throttle_raster_tasks_in_set; On 2014/09/16 22:49:04, reveman wrote: > "did_throttle_raster_tasks" instead of adding the "_in_set" prefix? Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:532: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:04, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:672: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) { On 2014/09/16 22:49:04, reveman wrote: > nit: ++task_set Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:676: task_set_sizes_[task_set]); On 2014/09/16 22:49:04, reveman wrote: > Use a "pending_count" array instead? Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:694: for (TaskSet task_set = 0; task_set < kNumberOfTaskSets; task_set++) On 2014/09/16 22:49:04, reveman wrote: > nit: ++task_set here and below Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:728: const TaskSetCollection& task_set_collection) { On 2014/09/16 22:49:04, reveman wrote: > nit: s/task_set_collection/task_sets/ Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:736: const TaskSetCollection& task_set_collection) { On 2014/09/16 22:49:04, reveman wrote: > nit: s/task_set_collection/task_sets/ Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:85: void operator+=(const TaskSetCollection& task_set_collection); On 2014/09/16 22:49:05, reveman wrote: > nit: s/task_set_collection/task_sets/ Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:86: void operator-=(const TaskSetCollection& task_set_collection); On 2014/09/16 22:49:05, reveman wrote: > nit: s/task_set_collection/task_sets/ Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:90: }; On 2014/09/16 22:49:05, reveman wrote: > I failed to think of this before but a number of explicit static utility > functions that operate on "size_t task_counts[kNumberOfTaskSets]" would likely > make the code easier to read and that might also allow you to hide all this in > the .cc file. ie. > > void AddTaskSetsToTaskCounts( > size_t *task_counts, const TaskSetCollection& tasks_sets); Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:106: TaskSetCollection HasPendingTasks() const; On 2014/09/16 22:49:05, reveman wrote: > Something awkward about a function named "HasPendingTasks" returning a > TaskSetCollection. I think your options are: > > bool HasPendingTasks(TaskSet task_set), > bool HasPendingTasks(const TaskSetCollection& task_sets) or > TaskSetCollection PendingTasks() > > I prefer one of the HasPendingTasks versions if possible. Done. "TaskSetCollection PendingTasks()" works best, because it doesn't require iterating over the task sets in the call sites (we can use the bitset operations). https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... File cc/resources/raster_worker_pool_perftest.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_perftest.cc:274: if (task_set == ALL) On 2014/09/16 22:49:05, reveman wrote: > do we need this conditional? It's more realistic to check in all situations. Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... File cc/resources/raster_worker_pool_unittest.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_unittest.cc:184: raster_worker_pool_->AsRasterizer()->CheckForCompletedTasks(); On 2014/09/16 22:49:05, reveman wrote: > remove the conditional for this if possible. Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_unittest.cc:185: base::MessageLoop::current()->Quit(); On 2014/09/16 22:49:05, reveman wrote: > We should switch this test over to using TestSimpleTaskRunner but you can do "if > (task_set == ALL) base::MessageLoop::current()->Quit();" for now unless you feel > like including that change in this patch. Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/raster_wor... cc/resources/raster_worker_pool_unittest.cc:215: TaskSetCollection task_set_collection; On 2014/09/16 22:49:05, reveman wrote: > nit: s/task_set_collection/task_sets/ Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/rasterizer.h File cc/resources/rasterizer.h (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/rasterizer... cc/resources/rasterizer.h:19: } On 2014/09/16 22:49:05, reveman wrote: > no need for this anymore Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... File cc/resources/tile_manager.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... cc/resources/tile_manager.cc:14: #include "base/lazy_instance.h" On 2014/09/16 22:49:05, reveman wrote: > do we need this? Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... cc/resources/tile_manager.cc:544: } else if (task_set == REQUIRED_FOR_ACTIVATION) { On 2014/09/16 22:49:05, reveman wrote: > nit: I prefer early out instead of else-ifs when possible and I think that's > common in cc/. Done. https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/tile_manag... cc/resources/tile_manager.h:91: HIGH_RESOLUTION_IN_NOW_BIN_ON_ACTIVE_TREE = 0, On 2014/09/16 22:49:05, reveman wrote: > I think we should add this new task set in a follow up instead. A follow up that > makes use of it rather than adding it right now without using it. Done. https://codereview.chromium.org/523243002/diff/160001/cc/test/fake_tile_manag... File cc/test/fake_tile_manager.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/test/fake_tile_manag... cc/test/fake_tile_manager.cc:97: void FakeTileManager::DidFinishRunningTasksForTesting() { On 2014/09/16 22:49:05, reveman wrote: > should this function take a TaskSet as parameter? This isn't used anywhere. I just removed it.
A few minor adjustments and this is ready to land. https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:29: void SetTaskCountsToZero(size_t* task_counts) { I think you can remove this and use std::fill instead. https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:34: TaskSetCollection TaskCountsToTaskSets(const size_t* task_counts) { NonEmptyTaskSetsFromTaskCounts? https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:35: TaskSetCollection task_set_collection; task_sets? https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:37: if (task_counts[task_set] > 0) nit: s/task_counts[task_set] > 0/task_counts[task_set]/ https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:57: } can you move the functions above to anonymous namespace? https://codereview.chromium.org/523243002/diff/180001/cc/resources/rasterizer.cc File cc/resources/rasterizer.cc (right): https://codereview.chromium.org/523243002/diff/180001/cc/resources/rasterizer... cc/resources/rasterizer.cc:61: : task(task), task_sets(task_sets) { Can you add a DCHECK(item.task_sets.any()) here for now unless there's a valid use case for having a task not be part of any task set? https://codereview.chromium.org/523243002/diff/180001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/523243002/diff/180001/cc/resources/tile_manag... cc/resources/tile_manager.h:94: // rasterizer.h Note: I think we might be able to use some template specialization to ensure that things don't compile if kNumberOfTaskSets is not in sync with this enum. But let's look at that as a follow up instead. https://codereview.chromium.org/523243002/diff/200001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/200001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:90: }; This class is no longer used, right?
https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:29: void SetTaskCountsToZero(size_t* task_counts) { On 2014/09/17 20:46:28, reveman wrote: > I think you can remove this and use std::fill instead. Done. https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:34: TaskSetCollection TaskCountsToTaskSets(const size_t* task_counts) { On 2014/09/17 20:46:28, reveman wrote: > NonEmptyTaskSetsFromTaskCounts? Done. https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:35: TaskSetCollection task_set_collection; On 2014/09/17 20:46:28, reveman wrote: > task_sets? Done. https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:37: if (task_counts[task_set] > 0) On 2014/09/17 20:46:28, reveman wrote: > nit: s/task_counts[task_set] > 0/task_counts[task_set]/ Done. https://codereview.chromium.org/523243002/diff/180001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:57: } On 2014/09/17 20:46:28, reveman wrote: > can you move the functions above to anonymous namespace? Done. https://codereview.chromium.org/523243002/diff/180001/cc/resources/rasterizer.cc File cc/resources/rasterizer.cc (right): https://codereview.chromium.org/523243002/diff/180001/cc/resources/rasterizer... cc/resources/rasterizer.cc:61: : task(task), task_sets(task_sets) { On 2014/09/17 20:46:28, reveman wrote: > Can you add a DCHECK(item.task_sets.any()) here for now unless there's a valid > use case for having a task not be part of any task set? Done. https://codereview.chromium.org/523243002/diff/180001/cc/resources/tile_manag... File cc/resources/tile_manager.h (right): https://codereview.chromium.org/523243002/diff/180001/cc/resources/tile_manag... cc/resources/tile_manager.h:94: // rasterizer.h On 2014/09/17 20:46:28, reveman wrote: > Note: I think we might be able to use some template specialization to ensure > that things don't compile if kNumberOfTaskSets is not in sync with this enum. > But let's look at that as a follow up instead. That would be great. Relaying on comments for such things is fragile. https://codereview.chromium.org/523243002/diff/200001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/523243002/diff/200001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.h:90: }; On 2014/09/17 20:46:28, reveman wrote: > This class is no longer used, right? Yes. Forgot to remove from the header.
LGTM after addressing these last two comments. This is a nice cleanup, Thanks! https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:450: RasterTaskVector tasks_in_set[kNumberOfTaskSets]; On 2014/09/17 19:57:14, ernstm wrote: > On 2014/09/16 22:49:04, reveman wrote: > > Why is "RasterTaskVector tasks[kNumberOfTaskSets]" not enough? > > We can't make any assumptions about the task sets in the raster worker pools. In > particular, we don't know that a set for ALL tasks exists. Theoretically, there > might be tasks that are not in any task set, but we still need to schedule them. I forgot to comment on this in my last review. I think we should assume that all tasks belong to some set for now and DCHECK if that's not the case. We can refactor this in the future if we actually have to support the case where a task doesn't belong to any task set. For now, let's keep things as simple as possible. https://codereview.chromium.org/523243002/diff/220001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/220001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:303: "should_notify_client_if_no_tasks_are_pending_[task_set]", if you want to add this last argument please call it "should_notify_client_if_no_tasks_are_pending" instead
You might want to take another quick look at PBRWP before I hit commit. https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/160001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:450: RasterTaskVector tasks_in_set[kNumberOfTaskSets]; On 2014/09/17 22:08:23, reveman wrote: > On 2014/09/17 19:57:14, ernstm wrote: > > On 2014/09/16 22:49:04, reveman wrote: > > > Why is "RasterTaskVector tasks[kNumberOfTaskSets]" not enough? > > > > We can't make any assumptions about the task sets in the raster worker pools. > In > > particular, we don't know that a set for ALL tasks exists. Theoretically, > there > > might be tasks that are not in any task set, but we still need to schedule > them. > > I forgot to comment on this in my last review. I think we should assume that all > tasks belong to some set for now and DCHECK if that's not the case. We can > refactor this in the future if we actually have to support the case where a task > doesn't belong to any task set. For now, let's keep things as simple as > possible. Done. https://codereview.chromium.org/523243002/diff/220001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/220001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:303: "should_notify_client_if_no_tasks_are_pending_[task_set]", On 2014/09/17 22:08:23, reveman wrote: > if you want to add this last argument please call it > "should_notify_client_if_no_tasks_are_pending" instead Done.
lgtm with nit https://codereview.chromium.org/523243002/diff/240001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/240001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:479: RasterTaskVector tasks_in_set[kNumberOfTaskSets]; nit: s/tasks_in_set/tasks/
https://codereview.chromium.org/523243002/diff/240001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/523243002/diff/240001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:479: RasterTaskVector tasks_in_set[kNumberOfTaskSets]; On 2014/09/18 00:13:05, reveman wrote: > nit: s/tasks_in_set/tasks/ Done.
The CQ bit was checked by ernstm@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/523243002/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as 6f1939e93d475bd1058331701ae28e283e89c065
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/69fedbd409dbc8adb204513fe88e95c040375c55 Cr-Commit-Position: refs/heads/master@{#295392} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
