Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
73923003:
Shared Raster Worker Threads (Closed)
|
Created:
7 years, 1 month ago by sohanjg Modified:
5 years, 1 month ago Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionShared Raster Worker Threads
This CL will share worker pool threads across LTHI for a single process.
Idea is to reduce the number of threads used per process.
For this, we implement WorkerPool::Inner as singleton, and LTHI/WorkerPool
would share the same instance per process.
Reviewer: reveman@chromium.org
BUG=239423
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245526
Patch Set 1 #
Total comments: 8
Patch Set 2 : Task namespace for WorkerPool #
Total comments: 12
Patch Set 3 : WIP - Mapped Task Pool for Worker Threads #
Total comments: 2
Patch Set 4 : WIP - Mapped Task Struct and Priority Queue #
Total comments: 21
Patch Set 5 : WIP - Clean up and Review Comments Changes #Patch Set 6 : Code cleanup and pre-submit issue changes #
Total comments: 13
Patch Set 7 : WIP - code review changes #
Total comments: 49
Patch Set 8 : WIP - Code Review Changes #
Total comments: 5
Patch Set 9 : WIP - code review changes #
Total comments: 90
Patch Set 10 : WIP - code review changes #
Total comments: 53
Patch Set 11 : WIP - nits + code review changes #
Total comments: 52
Patch Set 12 : WIP - redisgn shutdown logic + other nits #
Total comments: 22
Patch Set 13 : WIP - Shutdown re-design review + nits #Patch Set 14 : WIP - Shutdown re-design review 2 #
Total comments: 8
Patch Set 15 : WIP - make API to retrieve number of raster threads consistent + nits #
Total comments: 10
Patch Set 16 : WIP - realigning code to get number of raster threads + nits #
Total comments: 30
Patch Set 17 : WIP - number of raster thread code review + nits #
Total comments: 22
Patch Set 18 : WIP - code review changes #
Total comments: 33
Patch Set 19 : Rebase + ready_to_run_task_namespaces_ changes #
Total comments: 8
Patch Set 20 : Moving number of raster thread API to RasterWorkerPool + comments #
Total comments: 16
Patch Set 21 : Number of raster threads API comments #
Total comments: 35
Patch Set 22 : Code review changes #
Total comments: 17
Patch Set 23 : comments + nits #
Total comments: 11
Patch Set 24 : cc_unittest, cc_perftest changes + unit test + nits #
Total comments: 9
Patch Set 25 : Unit test updated #
Total comments: 8
Patch Set 26 : Unit test updated #
Total comments: 13
Patch Set 27 : Unit test updated #
Total comments: 11
Patch Set 28 : Unit Test nits #
Total comments: 2
Patch Set 29 : Change file mode + nit #
Total comments: 1
Patch Set 30 : Rebase for Commit #
Total comments: 4
Patch Set 31 : Rebase errors resolved #Patch Set 32 : Build Error for Win, non dll-interface class used as base class for dll-interface class. #Patch Set 33 : Build Error for Mac, macro conflict, moving setter to WorkerPool. #Messages
Total messages: 97 (1 generated)
This is WIP patch, PTAL and let me know your thoughts.
Some initial feedback. The patch is missing the critical part of task namespaces, without that, LTHI instances will simply override each others tasks. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:88: Inner(size_t num_threads, const std::string& thread_name_prefix); One ctor please. How about we create a derived class with only a default ctor that calls this existing Inner ctor with proper parameters to use for a global Inner instance? The default ctor of the derived class can probably read cc::switches::kNumRasterThreads directly unless we can think of a better place to do that, in which case it should be a global variable that can be adjusted using a static setter. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:107: void SetNumThreads(size_t val) {num_threads_ = val;} This is not thread safe. One LTHI might change the value of num_threads_ while another is running Start(). However, if you follow my suggestion above none of this is needed. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:423: WorkerPool::WorkerPool(size_t num_threads, num_threads can't be a LTHI setting anymore as the threads are now process wide. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:427: g_workerpool_inner.Pointer()->Start(); What's stopping Start() from being called multiple times? https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:437: g_workerpool_inner.Pointer()->Shutdown(); You need to introduce a task namespace for each WorkerPool instance and only shutdown the tasks related to this namespace. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:446: g_workerpool_inner.Pointer()->CollectCompletedTasks(&completed_tasks); Same here. Task namespace needed for this to work. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... cc/resources/worker_pool.cc:476: g_workerpool_inner.Pointer()->SetTaskGraph(graph); And here. Task namespace needed for this to work. https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.h#ne... cc/resources/worker_pool.h:113: friend class Inner; I don't think it should be a subclass anymore if you need to move it here.
On 2013/11/15 17:21:40, David Reveman wrote: > Some initial feedback. The patch is missing the critical part of task > namespaces, without that, LTHI instances will simply override each others tasks. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (right): > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:88: Inner(size_t num_threads, const std::string& > thread_name_prefix); > One ctor please. > > How about we create a derived class with only a default ctor that calls this > existing Inner ctor with proper parameters to use for a global Inner instance? > > The default ctor of the derived class can probably read > cc::switches::kNumRasterThreads directly unless we can think of a better place > to do that, in which case it should be a global variable that can be adjusted > using a static setter. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:107: void SetNumThreads(size_t val) {num_threads_ = > val;} > This is not thread safe. One LTHI might change the value of num_threads_ while > another is running Start(). However, if you follow my suggestion above none of > this is needed. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:423: WorkerPool::WorkerPool(size_t num_threads, > num_threads can't be a LTHI setting anymore as the threads are now process wide. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:427: g_workerpool_inner.Pointer()->Start(); > What's stopping Start() from being called multiple times? > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:437: g_workerpool_inner.Pointer()->Shutdown(); > You need to introduce a task namespace for each WorkerPool instance and only > shutdown the tasks related to this namespace. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:446: > g_workerpool_inner.Pointer()->CollectCompletedTasks(&completed_tasks); > Same here. Task namespace needed for this to work. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.cc#n... > cc/resources/worker_pool.cc:476: > g_workerpool_inner.Pointer()->SetTaskGraph(graph); > And here. Task namespace needed for this to work. > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.h > File cc/resources/worker_pool.h (right): > > https://codereview.chromium.org/73923003/diff/1/cc/resources/worker_pool.h#ne... > cc/resources/worker_pool.h:113: friend class Inner; > I don't think it should be a subclass anymore if you need to move it here. PTAL and let me know your comments.
https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:202: int GetNumRasterThreads() { nit: indent 2 spaces not 4 https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:202: int GetNumRasterThreads() { return size_t instead of int? https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:209: num_threads = 1; // Min num of threads we should also put a limit on the max number of threads. maybe you could do something like this: const int kMinRasterThreads = 1; const int kMaxRasterThreads = 64; return std::max(kMinRasterThreads, std::min(num_threads, kMaxRasterThreads)); https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.cc:147: TaskVector completed_tasks_; I don't think we should move any of these out of this class. Everything that can be accessed from multiple threads should stay in this class. Consider making each one of these something like: std::map<const WorkerPool*, TaskVector> completed_tasks_; https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.cc:89: void Shutdown(WorkerPool* wp); I don't think we should change Shutdown(). That's confusing. I think we should instead add a new function: void WaitForTasksToComplete(const WorkerPool* worker_pool); https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.cc:98: void SetTaskGraph(TaskGraph* graph, WorkerPool* wp); Prefer if you pass WorkerPool pointer as first parameter to all these functions and it should have the const modifier as this class should not be allowed to modify it. Only use it as a unique task namespace key. https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.cc:101: void CollectCompletedTasks(TaskVector* completed_tasks, WorkerPool* wp); Here and above. Please don't abbreviate the parameter name. Use "worker_pool" instead of "wp". https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.cc:148: WorkerPool* wp_; If you keep a pointer to one WorkerPool instance then you're still limiting the usage to one instance. Multiple WorkerPool instances need to be able to use this class at the same time from different threads. https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.h:112: class Inner; This doesn't have to and I don't think it should be a subclass anymore. Let's come up with a better name and move the implementation to an anonymous namespace in the .cc file.
https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:205: command_line.GetSwitchValueASCII(cc::switches::kNumRasterThreads); In case kNumRasterThreads command line switch is not provided, the string_value would be empty. We can bailout early by using std::string::empty() and returning the minimum number i.e. 1.
https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... cc/base/switches.cc:205: command_line.GetSwitchValueASCII(cc::switches::kNumRasterThreads); On 2013/11/21 17:18:12, vivekg__ wrote: > In case kNumRasterThreads command line switch is not provided, the string_value > would be empty. We can bailout early by using std::string::empty() and returning > the minimum number i.e. 1. Yes, please add a command_line.HasSwitch check and a kDefaultNumRasterThreads constant that is returned when switch is not present.
https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... cc/resources/worker_pool.cc:217: wp_ = wp; Wouldn't it be better to have a DCHECK(wp) to ensure we have the real pointer indeed?
On 2013/11/21 16:36:09, David Reveman wrote: > https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc > File cc/base/switches.cc (right): > > https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... > cc/base/switches.cc:202: int GetNumRasterThreads() { > nit: indent 2 spaces not 4 > > https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... > cc/base/switches.cc:202: int GetNumRasterThreads() { > return size_t instead of int? > > https://codereview.chromium.org/73923003/diff/130001/cc/base/switches.cc#newc... > cc/base/switches.cc:209: num_threads = 1; // Min num of threads > we should also put a limit on the max number of threads. maybe you could do > something like this: > > const int kMinRasterThreads = 1; > const int kMaxRasterThreads = 64; > return std::max(kMinRasterThreads, std::min(num_threads, kMaxRasterThreads)); > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (left): > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:147: TaskVector completed_tasks_; > I don't think we should move any of these out of this class. Everything that can > be accessed from multiple threads should stay in this class. > > Consider making each one of these something like: std::map<const WorkerPool*, > TaskVector> completed_tasks_; > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (right): > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:89: void Shutdown(WorkerPool* wp); > I don't think we should change Shutdown(). That's confusing. I think we should > instead add a new function: > > void WaitForTasksToComplete(const WorkerPool* worker_pool); > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:98: void SetTaskGraph(TaskGraph* graph, WorkerPool* > wp); > Prefer if you pass WorkerPool pointer as first parameter to all these functions > and it should have the const modifier as this class should not be allowed to > modify it. Only use it as a unique task namespace key. > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:101: void CollectCompletedTasks(TaskVector* > completed_tasks, WorkerPool* wp); > Here and above. Please don't abbreviate the parameter name. Use "worker_pool" > instead of "wp". > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:148: WorkerPool* wp_; > If you keep a pointer to one WorkerPool instance then you're still limiting the > usage to one instance. Multiple WorkerPool instances need to be able to use this > class at the same time from different threads. > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool.h > File cc/resources/worker_pool.h (right): > > https://codereview.chromium.org/73923003/diff/130001/cc/resources/worker_pool... > cc/resources/worker_pool.h:112: class Inner; > This doesn't have to and I don't think it should be a subclass anymore. Let's > come up with a better name and move the implementation to an anonymous namespace > in the .cc file. PTAL
We'll need a shared ready_to_run queue but I think it will be much simpler if you maintained separate ready_to_run queues for each worker pool instance and create the shared ready_to_run queue as priority queue of priority queues. https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... cc/resources/worker_pool.cc:233: // Build new "ready to run" tasks queue. new_ready_to_run_tasks need to also include the tasks from other worker pool instances. https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... cc/resources/worker_pool.cc:324: // Iterate in Pending Task Map to find task We can't afford this. You need to get the worker pool instance from the ready_to_run_tasks_ queue somehow.
On 2013/12/04 16:25:49, David Reveman wrote: > We'll need a shared ready_to_run queue but I think it will be much simpler if > you maintained separate ready_to_run queues for each worker pool instance and > create the shared ready_to_run queue as priority queue of priority queues. > what do u think will be the compare function for the priority queue of priority queues ? whether its workerpool is visible/active ? how can we check that ? the workerpool instance of priority queue will it be the same as now (TaskQueue)? we need to accomodate workerpool ptr as well to check in compare func? > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (right): > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:233: // Build new "ready to run" tasks queue. > new_ready_to_run_tasks need to also include the tasks from other worker pool > instances. > can't the current design handle this ? if we send the graph from diff workerpool instance, task will still be added to the ready_to_run queue ? isnt it ? but, priority wont be maintained ? > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:324: // Iterate in Pending Task Map to find task > We can't afford this. You need to get the worker pool instance from the > ready_to_run_tasks_ queue somehow. is the prob with iteraction ? or iteration on pending_tasks_pool_ ? if we use shared ready_to_run queue, then also we may need to iterate to get the corresponding workerpool instance. Please let me know your opinion.
On 2013/12/05 13:56:48, sohanjg wrote: > On 2013/12/04 16:25:49, David Reveman wrote: > > We'll need a shared ready_to_run queue but I think it will be much simpler if > > you maintained separate ready_to_run queues for each worker pool instance and > > create the shared ready_to_run queue as priority queue of priority queues. > > > what do u think will be the compare function for the priority queue of priority > queues ? > whether its workerpool is visible/active ? how can we check that ? > the workerpool instance of priority queue will it be the same as now > (TaskQueue)? we need to accomodate workerpool ptr as well to check in compare > func? You would just compare the priority of the top tasks of each priority queue in the same way we compare tasks in individual queues. > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool.cc > > File cc/resources/worker_pool.cc (right): > > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > > cc/resources/worker_pool.cc:233: // Build new "ready to run" tasks queue. > > new_ready_to_run_tasks need to also include the tasks from other worker pool > > instances. > > > can't the current design handle this ? if we send the graph from diff workerpool > instance, task will still be added to the ready_to_run queue ? isnt it ? but, > priority wont be maintained ? > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > > cc/resources/worker_pool.cc:324: // Iterate in Pending Task Map to find task > > We can't afford this. You need to get the worker pool instance from the > > ready_to_run_tasks_ queue somehow. > is the prob with iteraction ? or iteration on pending_tasks_pool_ ? > if we use shared ready_to_run queue, then also we may need to iterate to get the > corresponding workerpool instance. > > Please let me know your opinion.
On 2013/12/05 13:56:48, sohanjg wrote: > On 2013/12/04 16:25:49, David Reveman wrote: > > We'll need a shared ready_to_run queue but I think it will be much simpler if > > you maintained separate ready_to_run queues for each worker pool instance and > > create the shared ready_to_run queue as priority queue of priority queues. > > > what do u think will be the compare function for the priority queue of priority > queues ? > whether its workerpool is visible/active ? how can we check that ? > the workerpool instance of priority queue will it be the same as now > (TaskQueue)? we need to accomodate workerpool ptr as well to check in compare > func? > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool.cc > > File cc/resources/worker_pool.cc (right): > > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > > cc/resources/worker_pool.cc:233: // Build new "ready to run" tasks queue. > > new_ready_to_run_tasks need to also include the tasks from other worker pool > > instances. > > > can't the current design handle this ? if we send the graph from diff workerpool > instance, task will still be added to the ready_to_run queue ? isnt it ? but, > priority wont be maintained ? You need to remove the tasks from this workerpool instance but not others. The current code doesn't seem to be handling that. > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > > cc/resources/worker_pool.cc:324: // Iterate in Pending Task Map to find task > > We can't afford this. You need to get the worker pool instance from the > > ready_to_run_tasks_ queue somehow. > is the prob with iteraction ? or iteration on pending_tasks_pool_ ? > if we use shared ready_to_run queue, then also we may need to iterate to get the > corresponding workerpool instance. Instead of having a workerpool instance map for each graph/vector you can add a struct that contains them all and just one map from workerpool instance to that struct. Keep a priority queue of these structs and you have everything you need when you pull out the top item. > > Please let me know your opinion.
On 2013/12/05 16:25:28, David Reveman wrote: > On 2013/12/05 13:56:48, sohanjg wrote: > > On 2013/12/04 16:25:49, David Reveman wrote: > > > We'll need a shared ready_to_run queue but I think it will be much simpler > if > > > you maintained separate ready_to_run queues for each worker pool instance > and > > > create the shared ready_to_run queue as priority queue of priority queues. > > > > > what do u think will be the compare function for the priority queue of > priority > > queues ? > > whether its workerpool is visible/active ? how can we check that ? > > the workerpool instance of priority queue will it be the same as now > > (TaskQueue)? we need to accomodate workerpool ptr as well to check in compare > > func? > > > > > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool.cc > > > File cc/resources/worker_pool.cc (right): > > > > > > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > > > cc/resources/worker_pool.cc:233: // Build new "ready to run" tasks queue. > > > new_ready_to_run_tasks need to also include the tasks from other worker pool > > > instances. > > > > > can't the current design handle this ? if we send the graph from diff > workerpool > > instance, task will still be added to the ready_to_run queue ? isnt it ? but, > > priority wont be maintained ? > > You need to remove the tasks from this workerpool instance but not others. The > current code doesn't seem to be handling that. > > > > > > > > > https://codereview.chromium.org/73923003/diff/310001/cc/resources/worker_pool... > > > cc/resources/worker_pool.cc:324: // Iterate in Pending Task Map to find task > > > We can't afford this. You need to get the worker pool instance from the > > > ready_to_run_tasks_ queue somehow. > > is the prob with iteraction ? or iteration on pending_tasks_pool_ ? > > if we use shared ready_to_run queue, then also we may need to iterate to get > the > > corresponding workerpool instance. > > Instead of having a workerpool instance map for each graph/vector you can add a > struct that contains them all and just one map from workerpool instance to that > struct. Keep a priority queue of these structs and you have everything you need > when you pull out the top item. > > > > > Please let me know your opinion. Can you please have a look at this, and let me know your opinion. But, here the graph/vector size's (ready_to_run_task_set_->running_tasks_ , ready_to_run_task_set_->completed_tasks_ etc.) when it is accessed from ::Run, seems to be getting populated by junk, even though they are set as size 0 from SetTaskGraph.
This is still pretty rough but I think you're getting closer. I think the problems you have with the current patch is mostly coding errors and not design issues. The general design is starting to look like what we need. I'd suggest you try to clean it up a bit and remove all unnecessary code as that will make it easier for me to review it. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:94: typedef std::map<const WorkerPool*, TaskSet*> TaskMapper; Could you just store TaskSets by value instead of pointers? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:217: delete task_set; I don't think you want to delete the task set here as you just inserted this pointer into tasks_ and you use it below. None of this would be needed if you store TaskSets by value instead of pointers as I suggested above. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:223: if (!tasks_[worker_pool]->completed_tasks_.empty()) { why is this empty() check now needed? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:224: for (TaskVector::iterator it = tasks_[worker_pool]->completed_tasks_.begin(); Please avoid a lookup in tasks_ for every use and instead perform one lookup at the top of this function and use the result throughout. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:241: if (!tasks_[worker_pool]->running_tasks_.empty()) { why the empty() check? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:272: if (tasks_[worker_pool]->pending_tasks_.size() > 0) why the size() check? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:277: if (!tasks_[worker_pool]->pending_tasks_.empty()) and why check empty() here? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:281: if (!tasks_[worker_pool]->pending_tasks_.empty()) { and here https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:294: shared_ready_to_run_tasks_.push(tasks_[worker_pool]); what happens to the old TaskSet pointer in shared_ready_to_run_tasks_? I think you have to rebuild shared_ready_to_run_tasks_ from scratch. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:312: if (!shared_ready_to_run_tasks_.empty()) why the empty check? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:323: TaskSet* ready_to_run_task_set_ = new TaskSet(); why this heap allocation? Also don't use underscore at the end of local variable names. That's reserved for member variables and makes the code hard to review. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:338: if (ready_to_run_task_set_ == NULL || ready_to_run_task_set_->ready_to_run_tasks_.empty()) { I don't understand why you have these checks here. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:346: ready_to_run_task_set_->ready_to_run_tasks_.pop(); You'll have to insert the TaskSet in the shared queue again after you've taken the top task from the set. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:375: if (ready_to_run_task_set_->running_tasks_.size() > 0) { why the size check?
Please let me know your thoughts. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:94: typedef std::map<const WorkerPool*, TaskSet*> TaskMapper; On 2013/12/09 20:19:18, David Reveman wrote: > Could you just store TaskSets by value instead of pointers? We cannot use values, as we discussed in earlier implementation, because TaskSet has ScopedPtrHashMap and there are some inheritance issue while making a pair with it. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:217: delete task_set; On 2013/12/09 20:19:18, David Reveman wrote: > I don't think you want to delete the task set here as you just inserted this > pointer into tasks_ and you use it below. None of this would be needed if you > store TaskSets by value instead of pointers as I suggested above. As mentioned above, there is inheritance issue with TaskSet by value. Yes. we shouldnt be deleting the task here, i thought insert will make a copy of it. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:223: if (!tasks_[worker_pool]->completed_tasks_.empty()) { On 2013/12/09 20:19:18, David Reveman wrote: > why is this empty() check now needed? This and other size/empty checks are preventive checks because we are observing crashes while accessing empty tasks. We will remove them, once it stops crashing. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:294: shared_ready_to_run_tasks_.push(tasks_[worker_pool]); On 2013/12/09 20:19:18, David Reveman wrote: > what happens to the old TaskSet pointer in shared_ready_to_run_tasks_? I think > you have to rebuild shared_ready_to_run_tasks_ from scratch. shared_ready_to_run_tasks_ is a prioirity queue, so on pushing the new TaskSet the old TaskSet will be re-arranged based on priority of its ready_to_run_tasks_ queue. isnt it ? Or we still need to re-build again ? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:323: TaskSet* ready_to_run_task_set_ = new TaskSet(); On 2013/12/09 20:19:18, David Reveman wrote: > why this heap allocation? Also don't use underscore at the end of local variable > names. That's reserved for member variables and makes the code hard to review. Will take care of it. https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:338: if (ready_to_run_task_set_ == NULL || ready_to_run_task_set_->ready_to_run_tasks_.empty()) { On 2013/12/09 20:19:18, David Reveman wrote: > I don't understand why you have these checks here. This check is to ensure that, ready_to_run_task_set is setup only once till ready_to_run_tasks_ becomes empty. Without this on every iteration, it will get new ready_to_run_task_set from shared_ready_to_run_tasks_, even though all tasks incurrent ready_to_run_tasks_ has not been run. isnt it ? https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... cc/resources/worker_pool.cc:346: ready_to_run_task_set_->ready_to_run_tasks_.pop(); On 2013/12/09 20:19:18, David Reveman wrote: > You'll have to insert the TaskSet in the shared queue again after you've taken > the top task from the set. Why would we need to re-insert again ? Wont SetTaskGraph do it ?
On 2013/12/10 07:19:37, sohanjg wrote: > Please let me know your thoughts. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (right): > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:94: typedef std::map<const WorkerPool*, TaskSet*> > TaskMapper; > On 2013/12/09 20:19:18, David Reveman wrote: > > Could you just store TaskSets by value instead of pointers? > > We cannot use values, as we discussed in earlier implementation, because TaskSet > has ScopedPtrHashMap and there are some inheritance issue while making a pair > with it. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:217: delete task_set; > On 2013/12/09 20:19:18, David Reveman wrote: > > I don't think you want to delete the task set here as you just inserted this > > pointer into tasks_ and you use it below. None of this would be needed if you > > store TaskSets by value instead of pointers as I suggested above. > > As mentioned above, there is inheritance issue with TaskSet by value. > Yes. we shouldnt be deleting the task here, i thought insert will make a copy of > it. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:223: if > (!tasks_[worker_pool]->completed_tasks_.empty()) { > On 2013/12/09 20:19:18, David Reveman wrote: > > why is this empty() check now needed? > > This and other size/empty checks are preventive checks because we are observing > crashes while accessing empty tasks. We will remove them, once it stops > crashing. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:294: > shared_ready_to_run_tasks_.push(tasks_[worker_pool]); > On 2013/12/09 20:19:18, David Reveman wrote: > > what happens to the old TaskSet pointer in shared_ready_to_run_tasks_? I think > > you have to rebuild shared_ready_to_run_tasks_ from scratch. > > shared_ready_to_run_tasks_ is a prioirity queue, so on pushing the new TaskSet > the old TaskSet will be re-arranged based on priority of its ready_to_run_tasks_ > queue. isnt it ? > Or we still need to re-build again ? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:323: TaskSet* ready_to_run_task_set_ = new > TaskSet(); > On 2013/12/09 20:19:18, David Reveman wrote: > > why this heap allocation? Also don't use underscore at the end of local > variable > > names. That's reserved for member variables and makes the code hard to review. > > Will take care of it. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:338: if (ready_to_run_task_set_ == NULL || > ready_to_run_task_set_->ready_to_run_tasks_.empty()) { > On 2013/12/09 20:19:18, David Reveman wrote: > > I don't understand why you have these checks here. > > This check is to ensure that, ready_to_run_task_set is setup only once till > ready_to_run_tasks_ becomes empty. Without this on every iteration, it will get > new ready_to_run_task_set from shared_ready_to_run_tasks_, even though all tasks > incurrent ready_to_run_tasks_ has not been run. isnt it ? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:346: > ready_to_run_task_set_->ready_to_run_tasks_.pop(); > On 2013/12/09 20:19:18, David Reveman wrote: > > You'll have to insert the TaskSet in the shared queue again after you've taken > > the top task from the set. > > Why would we need to re-insert again ? Wont SetTaskGraph do it ? PTAL. The single lookup in SetTaskGraph is pending. Crashes are resolved now. Thanks.
On 2013/12/09 20:19:17, David Reveman wrote: > This is still pretty rough but I think you're getting closer. I think the > problems you have with the current patch is mostly coding errors and not design > issues. The general design is starting to look like what we need. > > I'd suggest you try to clean it up a bit and remove all unnecessary code as that > will make it easier for me to review it. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (right): > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:94: typedef std::map<const WorkerPool*, TaskSet*> > TaskMapper; > Could you just store TaskSets by value instead of pointers? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:217: delete task_set; > I don't think you want to delete the task set here as you just inserted this > pointer into tasks_ and you use it below. None of this would be needed if you > store TaskSets by value instead of pointers as I suggested above. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:223: if > (!tasks_[worker_pool]->completed_tasks_.empty()) { > why is this empty() check now needed? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:224: for (TaskVector::iterator it = > tasks_[worker_pool]->completed_tasks_.begin(); > Please avoid a lookup in tasks_ for every use and instead perform one lookup at > the top of this function and use the result throughout. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:241: if > (!tasks_[worker_pool]->running_tasks_.empty()) { > why the empty() check? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:272: if (tasks_[worker_pool]->pending_tasks_.size() > > 0) > why the size() check? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:277: if > (!tasks_[worker_pool]->pending_tasks_.empty()) > and why check empty() here? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:281: if > (!tasks_[worker_pool]->pending_tasks_.empty()) { > and here > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:294: > shared_ready_to_run_tasks_.push(tasks_[worker_pool]); > what happens to the old TaskSet pointer in shared_ready_to_run_tasks_? I think > you have to rebuild shared_ready_to_run_tasks_ from scratch. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:312: if (!shared_ready_to_run_tasks_.empty()) > why the empty check? > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:323: TaskSet* ready_to_run_task_set_ = new > TaskSet(); > why this heap allocation? Also don't use underscore at the end of local variable > names. That's reserved for member variables and makes the code hard to review. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:338: if (ready_to_run_task_set_ == NULL || > ready_to_run_task_set_->ready_to_run_tasks_.empty()) { > I don't understand why you have these checks here. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:346: > ready_to_run_task_set_->ready_to_run_tasks_.pop(); > You'll have to insert the TaskSet in the shared queue again after you've taken > the top task from the set. > > https://codereview.chromium.org/73923003/diff/330001/cc/resources/worker_pool... > cc/resources/worker_pool.cc:375: if > (ready_to_run_task_set_->running_tasks_.size() > 0) { > why the size check? Can you please take a look. Thanks
https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:190: void WorkerInner::SetTaskGraph(TaskGraph* graph, WorkerPool* worker_pool) { Let's focus on getting this function right first. Still a number of problems here.. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:197: TaskVector new_completed_tasks; why do you need this? https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:198: GraphNodeMap temp_pending_tasks; This doesn't seem useful either. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:207: if (tasks_.count(worker_pool) == 0) { I think we should add ::Register/Unregister(const WorkerPool* worker_pool) functions that add/remove the relevant taskset instead of creating it here first time you see a new workerPool instance. That way this can just become: DCHECK(tasks_.find(worker_pool) != tasks_.end()); TaskSet* task_set = tasks_[worker_pool]; and all code below that use tasks_[worker_pool] can just use task_set isntead. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:219: new_completed_tasks = tasks_[worker_pool]->completed_tasks_; This is making a complete copy of the vector, which is expensive and I don't see the purpose of it. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:265: if (!node->num_dependencies()) { please avoid small changes like this as it makes it harder to review the code. I have to look at it closely to see if you actually changed something meaningful. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:294: shared_ready_to_run_tasks_.push(tasks_[worker_pool]); this doesn't seem right. what are you trying to do here? you pop the top priority taskset and then push this worker pool's task set..
PTAL Thanks https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:197: TaskVector new_completed_tasks; On 2013/12/14 05:22:40, David Reveman wrote: > why do you need this? Done. This and below temp variables were added for creating new taskset, but with register/unregister style, it is not reqd. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:198: GraphNodeMap temp_pending_tasks; On 2013/12/14 05:22:40, David Reveman wrote: > This doesn't seem useful either. Done. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:207: if (tasks_.count(worker_pool) == 0) { On 2013/12/14 05:22:40, David Reveman wrote: > I think we should add ::Register/Unregister(const WorkerPool* worker_pool) > functions that add/remove the relevant taskset instead of creating it here first > time you see a new workerPool instance. That way this can just become: > > DCHECK(tasks_.find(worker_pool) != tasks_.end()); > TaskSet* task_set = tasks_[worker_pool]; > > and all code below that use tasks_[worker_pool] can just use task_set isntead. Done. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:219: new_completed_tasks = tasks_[worker_pool]->completed_tasks_; On 2013/12/14 05:22:40, David Reveman wrote: > This is making a complete copy of the vector, which is expensive and I don't see > the purpose of it. Done. This i had added because i had wrongly interpreted one of your earlier comment about look-up in tasks_. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:265: if (!node->num_dependencies()) { On 2013/12/14 05:22:40, David Reveman wrote: > please avoid small changes like this as it makes it harder to review the code. I > have to look at it closely to see if you actually changed something meaningful. Done. https://codereview.chromium.org/73923003/diff/370001/cc/resources/worker_pool... cc/resources/worker_pool.cc:294: shared_ready_to_run_tasks_.push(tasks_[worker_pool]); On 2013/12/14 05:22:40, David Reveman wrote: > this doesn't seem right. what are you trying to do here? you pop the top > priority taskset and then push this worker pool's task set.. Done. The idea was to recreate the shared_ready_to_run_tasks_ everytime, with the use case i was running its size was not excedding 1, so the single pop. std::priority_queue do not have an API to clear the existing entries except for the pop operation.
https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:186: DCHECK_EQ(0u, completed_tasks_.size()); I think all these DCHECKs should move to the new Unregister function. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:29: class WorkerInner : public base::DelegateSimpleThread::Delegate { We need a better name than "WorkerInner" for this. It used to be WorkerPool::Inner which would make it WorkerPoolInner but I don't think that's a good name anymore. It's possible that we want to move this into a separate file with unit tests in the future so a name the represents what it does would be good. Maybe TaskGraphRunner? The comment above also need to be updated to reflect that this is now able to run task graphs from multiple workerpool instances. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:38: typedef GraphNodeMap TaskGraph; How about "typedef WorkerPool::TaskGraph TaskGraph;" instead and avoid using GraphNodeMap in this class? I don't think we should redefine these here. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:40: TaskMap; Not a task map. Should it be TaskGraphMap? Also think this should be ScopedPtrHashMap<const WorkerPool*, TaskGraph> instead. We're not allowed to use anything but "const WorkerPool*" in this class. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:49: void SetTaskGraph(TaskGraph* graph, WorkerPool* worker_pool); "const WorkerPool*" instead and I prefer the worker_pool key as first parameter. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:53: WorkerPool* worker_pool); same here. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:56: void Unregister(const WorkerPool* worker_pool); Please keep these sorted the same way as the implementation. ie. move above Shutdown(). https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:77: class TaskSet { TaskNamespace? https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:78: public: Make this a struct instead and remove "_" suffix from members. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: class TaskComparator { This is a priority comparator too. How about renaming the above PriorityComparator to TaskPriorityComparator and this becomes TaskNamespacePriorityComparator? https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:96: } Could you reuse the logic from the above comparator here? Maybe add PriorityComparator as a member of this comparator and use it in the compare operator. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:99: typedef std::map<const WorkerPool*, TaskSet*> TaskMapper; TaskNamespaceMap? Also, please use linked_ptr<TaskNamespace> instead of a raw pointer to help manage the memory. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:101: TaskMapper tasks_; namespaces_? Also, please move all member variables below member functions. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:105: TaskComparator> TaskPriorityQueue; NamespaceQueue? https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:107: TaskPriorityQueue shared_ready_to_run_tasks_; ready_to_run_namespaces_? Also move this below member functions. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:175: // Add TaskSet I don't think this comment adds any valuable information. Please remove it. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:176: TaskSet* task_set= new TaskSet(); use linked_ptr here. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:177: tasks_.insert(std::pair<const WorkerPool*, Add DCHECK(tasks_.find(worker_pool) == tasks_.end()) above this line. You can also just do "tasks_[worker_pool] = task_set" instead of using insert. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:184: // Remove TaskSet remove comment https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:185: delete tasks_[worker_pool]; this goes away when using linked_ptr https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:186: tasks_.erase(worker_pool); Add DCHECK(tasks_.find(worker_pool) != tasks_.end()) above this line. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:187: no need for this blankline https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:253: } no need to add a space here https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:299: shared_ready_to_run_tasks_.push(task_set); This will clear the queue and only add back task_set. What about all other task sets? I think you need to iterate over all task sets and build a new_shared_ready_to_run_tasks. Then std::swap(shared_ready_to_run_tasks_, new_shared_ready_to_run_tasks);
PTAL Thanks https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:29: class WorkerInner : public base::DelegateSimpleThread::Delegate { On 2013/12/16 17:06:30, David Reveman wrote: > We need a better name than "WorkerInner" for this. It used to be > WorkerPool::Inner which would make it WorkerPoolInner but I don't think that's a > good name anymore. > > It's possible that we want to move this into a separate file with unit tests in > the future so a name the represents what it does would be good. Maybe > TaskGraphRunner? The comment above also need to be updated to reflect that this > is now able to run task graphs from multiple workerpool instances. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:38: typedef GraphNodeMap TaskGraph; On 2013/12/16 17:06:30, David Reveman wrote: > How about "typedef WorkerPool::TaskGraph TaskGraph;" instead and avoid using > GraphNodeMap in this class? I don't think we should redefine these here. Done. I had to pull the TaskGraph and Taskvector from protected to public in workerpool, if thats ok with you. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:40: TaskMap; On 2013/12/16 17:06:30, David Reveman wrote: > Not a task map. Should it be TaskGraphMap? Also think this should be > ScopedPtrHashMap<const WorkerPool*, TaskGraph> instead. We're not allowed to use > anything but "const WorkerPool*" in this class. Done. This code was not used https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:49: void SetTaskGraph(TaskGraph* graph, WorkerPool* worker_pool); On 2013/12/16 17:06:30, David Reveman wrote: > "const WorkerPool*" instead and I prefer the worker_pool key as first parameter. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:53: WorkerPool* worker_pool); On 2013/12/16 17:06:30, David Reveman wrote: > same here. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:56: void Unregister(const WorkerPool* worker_pool); On 2013/12/16 17:06:30, David Reveman wrote: > Please keep these sorted the same way as the implementation. ie. move above > Shutdown(). Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:77: class TaskSet { On 2013/12/16 17:06:30, David Reveman wrote: > TaskNamespace? Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:78: public: On 2013/12/16 17:06:30, David Reveman wrote: > Make this a struct instead and remove "_" suffix from members. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: class TaskComparator { On 2013/12/16 17:06:30, David Reveman wrote: > This is a priority comparator too. How about renaming the above > PriorityComparator to TaskPriorityComparator and this becomes > TaskNamespacePriorityComparator? Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:96: } On 2013/12/16 17:06:30, David Reveman wrote: > Could you reuse the logic from the above comparator here? Maybe add > PriorityComparator as a member of this comparator and use it in the compare > operator. There are some issue with constantness when invoking operator() of PriorityComparator, so kept it as is. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:99: typedef std::map<const WorkerPool*, TaskSet*> TaskMapper; On 2013/12/16 17:06:30, David Reveman wrote: > TaskNamespaceMap? Also, please use linked_ptr<TaskNamespace> instead of a raw > pointer to help manage the memory. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:101: TaskMapper tasks_; On 2013/12/16 17:06:30, David Reveman wrote: > namespaces_? Also, please move all member variables below member functions. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:105: TaskComparator> TaskPriorityQueue; On 2013/12/16 17:06:30, David Reveman wrote: > NamespaceQueue? Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:107: TaskPriorityQueue shared_ready_to_run_tasks_; On 2013/12/16 17:06:30, David Reveman wrote: > ready_to_run_namespaces_? Also move this below member functions. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:175: // Add TaskSet On 2013/12/16 17:06:30, David Reveman wrote: > I don't think this comment adds any valuable information. Please remove it. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:176: TaskSet* task_set= new TaskSet(); On 2013/12/16 17:06:30, David Reveman wrote: > use linked_ptr here. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:177: tasks_.insert(std::pair<const WorkerPool*, On 2013/12/16 17:06:30, David Reveman wrote: > Add DCHECK(tasks_.find(worker_pool) == tasks_.end()) above this line. You can > also just do "tasks_[worker_pool] = task_set" instead of using insert. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:184: // Remove TaskSet On 2013/12/16 17:06:30, David Reveman wrote: > remove comment Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:185: delete tasks_[worker_pool]; On 2013/12/16 17:06:30, David Reveman wrote: > this goes away when using linked_ptr Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:186: tasks_.erase(worker_pool); On 2013/12/16 17:06:30, David Reveman wrote: > Add DCHECK(tasks_.find(worker_pool) != tasks_.end()) above this line. Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:187: On 2013/12/16 17:06:30, David Reveman wrote: > no need for this blankline Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:253: } On 2013/12/16 17:06:30, David Reveman wrote: > no need to add a space here Done. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:299: shared_ready_to_run_tasks_.push(task_set); On 2013/12/16 17:06:30, David Reveman wrote: > This will clear the queue and only add back task_set. What about all other task > sets? > > I think you need to iterate over all task sets and build a > new_shared_ready_to_run_tasks. Then std::swap(shared_ready_to_run_tasks_, > new_shared_ready_to_run_tasks); There will be only 1 taskset/workerpool, so each time settaskgraph is invoked, should we need to maintain the previous taskset for the same workerpool ? When settaskgraph is invoked with new workerpool instance, the old taskset's for previous workerpool have already been executed in Run, and is popped out there. Am i missing something here ? I did try iterating and recreating the shared_ready_to_run_tasks_, but same workerpool instance is invoking settaskgraph, with diff numbers of pending,running,completed etc. tasks each time, and i thought we shouldnt maintain multiple entries for same workerpool in the priority queue. Maybe we can check if we have the same taskset already present, then pop it, and update it with new taskset.
https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:299: shared_ready_to_run_tasks_.push(task_set); On 2013/12/17 14:58:12, sohanjg wrote: > On 2013/12/16 17:06:30, David Reveman wrote: > > This will clear the queue and only add back task_set. What about all other > task > > sets? > > > > I think you need to iterate over all task sets and build a > > new_shared_ready_to_run_tasks. Then std::swap(shared_ready_to_run_tasks_, > > new_shared_ready_to_run_tasks); > > There will be only 1 taskset/workerpool, so each time settaskgraph is invoked, > should we need to maintain the previous taskset for the same workerpool ? > When settaskgraph is invoked with new workerpool instance, the old taskset's for > previous workerpool have already been executed in Run, and is popped out there. > Am i missing something here ? we'll have calls to settaskgraph from multiple threads with different workerpool instances and there's no guarantee that the "Run" method running on a worker thread have done any work at the time. > > I did try iterating and recreating the shared_ready_to_run_tasks_, but same > workerpool instance is invoking settaskgraph, with diff numbers of > pending,running,completed etc. tasks each time, and i thought we shouldnt > maintain multiple entries for same workerpool in the priority queue. > Maybe we can check if we have the same taskset already present, then pop it, and > update it with new taskset. you shouldn't have to worry about this if you just iterate over all task sets and build a completely new shared_ready_to_run_tasks_ that replaces the old. https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool... cc/resources/worker_pool.cc:83: const linked_ptr<TaskNamespace> b) { I like to see you reuse the TaskPriorityComparator logic above here instead of duplicating the code. https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool... cc/resources/worker_pool.cc:98: std::vector<linked_ptr<TaskNamespace> >, no need for linked_ptr here. this can hold raw pointers to TaskNamespaces "std::vector<TaskNamespace*>" as the lifetime is guaranteed by the map. https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool... cc/resources/worker_pool.cc:218: linked_ptr<TaskNamespace> task_set = namespaces_[worker_pool]; task_namespace rather than task_set please. too bad "namespace" is a reserved word..
PTAL Thanks. https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool... cc/resources/worker_pool.cc:83: const linked_ptr<TaskNamespace> b) { On 2013/12/19 02:09:38, David Reveman wrote: > I like to see you reuse the TaskPriorityComparator logic above here instead of > duplicating the code. Done. I had to loose the constantness of TaskPriorityComparator compare.will that be ok ? https://codereview.chromium.org/73923003/diff/410001/cc/resources/worker_pool... cc/resources/worker_pool.cc:98: std::vector<linked_ptr<TaskNamespace> >, On 2013/12/19 02:09:38, David Reveman wrote: > no need for linked_ptr here. this can hold raw pointers to TaskNamespaces > "std::vector<TaskNamespace*>" as the lifetime is guaranteed by the map. Done.
Sorry, missed this. Let me know your thoughts. https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/390001/cc/resources/worker_pool... cc/resources/worker_pool.cc:299: shared_ready_to_run_tasks_.push(task_set); On 2013/12/19 02:09:38, David Reveman wrote: > On 2013/12/17 14:58:12, sohanjg wrote: > > On 2013/12/16 17:06:30, David Reveman wrote: > > > This will clear the queue and only add back task_set. What about all other > > task > > > sets? > > > > > > I think you need to iterate over all task sets and build a > > > new_shared_ready_to_run_tasks. Then std::swap(shared_ready_to_run_tasks_, > > > new_shared_ready_to_run_tasks); > > > > There will be only 1 taskset/workerpool, so each time settaskgraph is invoked, > > should we need to maintain the previous taskset for the same workerpool ? > > When settaskgraph is invoked with new workerpool instance, the old taskset's > for > > previous workerpool have already been executed in Run, and is popped out > there. > > Am i missing something here ? > > we'll have calls to settaskgraph from multiple threads with different workerpool > instances and there's no guarantee that the "Run" method running on a worker > thread have done any work at the time. > > > > > I did try iterating and recreating the shared_ready_to_run_tasks_, but same > > workerpool instance is invoking settaskgraph, with diff numbers of > > pending,running,completed etc. tasks each time, and i thought we shouldnt > > maintain multiple entries for same workerpool in the priority queue. > > Maybe we can check if we have the same taskset already present, then pop it, > and > > update it with new taskset. > > you shouldn't have to worry about this if you just iterate over all task sets > and build a completely new shared_ready_to_run_tasks_ that replaces the old. Done. When i check the run-time state of the shared_ready_to_run_tasks_, it keeps on adding new taskset's for the same workerpool. While loading google.com in content shell, its size goes till 70 for the same workerpool instance, even though i pop shared_ready_to_run_tasks_ when the ready_to_run_task_set is empty. I think we need to handle something more in "Run"
FYI, I haven't started looking at TaskGraphRunner::Run() yet. I'm still focusing on SetTaskGraph and we'll move on to ::Run() once that's done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:80: class TaskNamespacePriorityComparator { nit: remove one space between class and TaskNamespace.. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:81: public: nit: "public:" should be indented one space. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:83: TaskNamespace* b) { nit: "TaskNamespace* b" vertically aligned with "TaskNamespace* a" https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: b->ready_to_run_tasks.top()); nit: "b->ready_to_run_tasks.top()" veritcally aligned with "a->ready_to_run_tasks.top()" https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:87: TaskPriorityComparator comparator_; nit: s/comparator_/task_comparator_/ https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:91: linked_ptr<TaskNamespace> > TaskNamespaceMap; nit: fix style: typedef std::map<const WorkerPool*, linked_ptr<TaskNamespace> > TaskNamespaceMap; https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:95: TaskNamespacePriorityComparator> NamespaceQueue; nit: TaskNamespaceQueue https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:200: TaskGraph* graph) { nit: "TaskGraph* graph" vertically aligned with "const WorkerPool* worker_pool" https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:215: linked_ptr<TaskNamespace> task_namespace = namespaces_[worker_pool]; This can be a raw pointer: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:235: task_namespace->running_tasks.begin(); nit: move to previous line if it fits. otherwise 4 space indent relative to "TaskGraph::". https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:236: it != task_namespace->running_tasks.end(); ++it) { nit: "it != task_n..." vertically aligned with "TaskGraph::iter.." https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:243: } nit: no need to add this space. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:274: task_namespace->pending_tasks.begin(); nit: move to previous line if it fits. otherwise 4 space indent relative to "TaskGraph::". https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:275: it != task_namespace->pending_tasks.end(); nit: "it != task_n..." vertically aligned with "TaskGraph::const_iter.." https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:276: ++it) { please move this to previous line to be consistent with other loops in this file. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:277: task_namespace->completed_tasks.push_back(it->first); nit: this should be indented 2 spaces, not 4 https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:290: } Please ignore ready_to_run_namespaces_ here and simply build new_ready_to_run_namespaces by iterating over task_namespaces_ instead. Add only add task namespaces with non-empty ready_to_run_tasks. Also, please move this above line 280 so all swapping is done in one place. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:292: ready_to_run_namespaces_.push(task_namespace.get()); this line should not be here if you do what I suggested above. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:298: !task_namespace->running_tasks.empty())); nit: this last line should be indented one more space as it's part of the (a || b) group https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:301: if (!task_namespace->ready_to_run_tasks.empty()) Please change this to: if (!ready_to_run_namespaces_.empty())
Here's some more feedback that covers the implementation of ::Run(). Please make sure you look at all the previous feedback I send too. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:342: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:29: // by |lock_|. nit: s/will be able to/can process/ and I think you can fit the comment on 2 lines https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:33: virtual ~TaskGraphRunner(); nit: blankline between ctor/dtor and other member functions https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:40: typedef WorkerPool::TaskVector TaskVector; can these typedefs be private? https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:52: TaskVector* completed_tasks); nit: "TaskVector* co.." should vertically align with "const WorkerPo.." https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:93: typedef std::priority_queue<TaskNamespace*, please add this comment above this: // Ordered set of tasks namespaces that have ready to run tasks. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:126: class CC_EXPORT DerivedInner : public TaskGraphRunner { This needs a better name. I think CompositorRasterTaskGraphRunner will do for now. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:128: DerivedInner(); the implementation of this can be inlined. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:129: }; please move this class below the TaskGraphRunner implementation. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:131: base::LazyInstance<DerivedInner> g_workerpool_inner; I think this should be "g_task_graph_runner = LAZY_INSTANCE_INITIALIZER;" and this of course needs to be moved too. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:171: } nit: add blank line between these functions https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:307: (const WorkerPool* worker_pool, TaskVector* completed_tasks) { nit: "(" on previous line. "const WorkerPool* wor.." indented 4 spaces https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:311: if (!ready_to_run_namespaces_.empty()) no need for this conditional. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:320: TaskNamespace* ready_to_run_task_set = NULL; move to "while (true) {" scope https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:323: if ((ready_to_run_namespaces_.empty()) || this check should only be "ready_to_run_namespaces_.empty()" https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:326: // Exit when shutdown is set and no more tasks are pending. nit: this and following lines are indented incorrectly. should be 2 spaces. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:327: if (shutdown_ && ready_to_run_task_set && Let's just make this "if (shutdown_)". We'll make that work properly later when we rework the shutdown code. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:329: break; nit: wrong indent here too https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:337: ready_to_run_task_set = ready_to_run_namespaces_.top(); You need to pop the namespace too. Think this should be: TaskNamespace* task_namespace = ready_to_run_namespaces_.top(); ready_to_run_namespaces_.pop(); DCHECK(!task_namespace->ready_to_run_tasks.empty()); https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:343: Here after taking the top task off the queue, you need to add the task_namespace back to ready_to_run_namespaces_ if task_namespace->ready_to_run_tasks is not empty. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:350: task.get(), ready_to_run_task_set->pending_tasks.take_and_erase nit: move "ready_to_run_task_set->pendi" to the next line https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:356: nit: no need to add this line https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:373: if (node) { nit: not need to change indention of this and following lines https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:383: ready_to_run_task_set->ready_to_run_tasks.push(dependent_node); you need to add the task namespace to |ready_to_run_namespaces_| if it was empty before this https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:389: ready_to_run_namespaces_.pop(); what's the purpose of these lines? I think they should just be removed.
PTAL. Thanks https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:29: // by |lock_|. On 2013/12/20 01:53:24, David Reveman wrote: > nit: s/will be able to/can process/ and I think you can fit the comment on 2 > lines Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:33: virtual ~TaskGraphRunner(); On 2013/12/20 01:53:24, David Reveman wrote: > nit: blankline between ctor/dtor and other member functions Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:40: typedef WorkerPool::TaskVector TaskVector; On 2013/12/20 01:53:24, David Reveman wrote: > can these typedefs be private? SetTaskGraph, CollectCompletedTasks will need the typedef's. either we make the 2 func's pvt and inherit WorkerPool from TaskGraphRunner, or use some gloabl scope, whats your opinion ? https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:52: TaskVector* completed_tasks); On 2013/12/20 01:53:24, David Reveman wrote: > nit: "TaskVector* co.." should vertically align with "const WorkerPo.." Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:80: class TaskNamespacePriorityComparator { On 2013/12/19 19:35:49, David Reveman wrote: > nit: remove one space between class and TaskNamespace.. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:81: public: On 2013/12/19 19:35:49, David Reveman wrote: > nit: "public:" should be indented one space. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:83: TaskNamespace* b) { On 2013/12/19 19:35:49, David Reveman wrote: > nit: "TaskNamespace* b" vertically aligned with "TaskNamespace* a" Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: b->ready_to_run_tasks.top()); On 2013/12/19 19:35:49, David Reveman wrote: > nit: "b->ready_to_run_tasks.top()" veritcally aligned with > "a->ready_to_run_tasks.top()" Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:87: TaskPriorityComparator comparator_; On 2013/12/19 19:35:49, David Reveman wrote: > nit: s/comparator_/task_comparator_/ Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:91: linked_ptr<TaskNamespace> > TaskNamespaceMap; On 2013/12/19 19:35:49, David Reveman wrote: > nit: fix style: > > typedef std::map<const WorkerPool*, linked_ptr<TaskNamespace> > > TaskNamespaceMap; Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:93: typedef std::priority_queue<TaskNamespace*, On 2013/12/20 01:53:24, David Reveman wrote: > please add this comment above this: > // Ordered set of tasks namespaces that have ready to run tasks. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:95: TaskNamespacePriorityComparator> NamespaceQueue; On 2013/12/19 19:35:49, David Reveman wrote: > nit: TaskNamespaceQueue Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:126: class CC_EXPORT DerivedInner : public TaskGraphRunner { On 2013/12/20 01:53:24, David Reveman wrote: > This needs a better name. I think CompositorRasterTaskGraphRunner will do for > now. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:128: DerivedInner(); On 2013/12/20 01:53:24, David Reveman wrote: > the implementation of this can be inlined. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:129: }; On 2013/12/20 01:53:24, David Reveman wrote: > please move this class below the TaskGraphRunner implementation. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:131: base::LazyInstance<DerivedInner> g_workerpool_inner; On 2013/12/20 01:53:24, David Reveman wrote: > I think this should be "g_task_graph_runner = LAZY_INSTANCE_INITIALIZER;" and > this of course needs to be moved too. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:171: } On 2013/12/20 01:53:24, David Reveman wrote: > nit: add blank line between these functions Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:200: TaskGraph* graph) { On 2013/12/19 19:35:49, David Reveman wrote: > nit: "TaskGraph* graph" vertically aligned with "const WorkerPool* worker_pool" Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:215: linked_ptr<TaskNamespace> task_namespace = namespaces_[worker_pool]; On 2013/12/19 19:35:49, David Reveman wrote: > This can be a raw pointer: > TaskNamespace* task_namespace = namespaces_[worker_pool].get(); Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:235: task_namespace->running_tasks.begin(); On 2013/12/19 19:35:49, David Reveman wrote: > nit: move to previous line if it fits. otherwise 4 space indent relative to > "TaskGraph::". Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:236: it != task_namespace->running_tasks.end(); ++it) { On 2013/12/19 19:35:49, David Reveman wrote: > nit: "it != task_n..." vertically aligned with "TaskGraph::iter.." Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:243: } On 2013/12/19 19:35:49, David Reveman wrote: > nit: no need to add this space. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:274: task_namespace->pending_tasks.begin(); On 2013/12/19 19:35:49, David Reveman wrote: > nit: move to previous line if it fits. otherwise 4 space indent relative to > "TaskGraph::". Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:275: it != task_namespace->pending_tasks.end(); On 2013/12/19 19:35:49, David Reveman wrote: > nit: "it != task_n..." vertically aligned with "TaskGraph::const_iter.." Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:276: ++it) { On 2013/12/19 19:35:49, David Reveman wrote: > please move this to previous line to be consistent with other loops in this > file. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:277: task_namespace->completed_tasks.push_back(it->first); On 2013/12/19 19:35:49, David Reveman wrote: > nit: this should be indented 2 spaces, not 4 Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:290: } On 2013/12/19 19:35:49, David Reveman wrote: > Please ignore ready_to_run_namespaces_ here and simply build > new_ready_to_run_namespaces by iterating over task_namespaces_ instead. Add only > add task namespaces with non-empty ready_to_run_tasks. > > Also, please move this above line 280 so all swapping is done in one place. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:292: ready_to_run_namespaces_.push(task_namespace.get()); On 2013/12/19 19:35:49, David Reveman wrote: > this line should not be here if you do what I suggested above. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:298: !task_namespace->running_tasks.empty())); On 2013/12/19 19:35:49, David Reveman wrote: > nit: this last line should be indented one more space as it's part of the (a || > b) group Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:301: if (!task_namespace->ready_to_run_tasks.empty()) On 2013/12/19 19:35:49, David Reveman wrote: > Please change this to: > > if (!ready_to_run_namespaces_.empty()) Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:307: (const WorkerPool* worker_pool, TaskVector* completed_tasks) { On 2013/12/20 01:53:24, David Reveman wrote: > nit: "(" on previous line. "const WorkerPool* wor.." indented 4 spaces Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:311: if (!ready_to_run_namespaces_.empty()) On 2013/12/20 01:53:24, David Reveman wrote: > no need for this conditional. i am seeing a crash when we try to access top if its empty, please let me know your opinion. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:320: TaskNamespace* ready_to_run_task_set = NULL; On 2013/12/20 01:53:24, David Reveman wrote: > move to "while (true) {" scope Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:323: if ((ready_to_run_namespaces_.empty()) || On 2013/12/20 01:53:24, David Reveman wrote: > this check should only be "ready_to_run_namespaces_.empty()" Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:326: // Exit when shutdown is set and no more tasks are pending. On 2013/12/20 01:53:24, David Reveman wrote: > nit: this and following lines are indented incorrectly. should be 2 spaces. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:327: if (shutdown_ && ready_to_run_task_set && On 2013/12/20 01:53:24, David Reveman wrote: > Let's just make this "if (shutdown_)". We'll make that work properly later when > we rework the shutdown code. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:329: break; On 2013/12/20 01:53:24, David Reveman wrote: > nit: wrong indent here too Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:337: ready_to_run_task_set = ready_to_run_namespaces_.top(); On 2013/12/20 01:53:24, David Reveman wrote: > You need to pop the namespace too. Think this should be: > > TaskNamespace* task_namespace = ready_to_run_namespaces_.top(); > ready_to_run_namespaces_.pop(); > DCHECK(!task_namespace->ready_to_run_tasks.empty()); Done. The pop i had done in the end of the loop, when ready_to_run_tasks was empty, so the next push you refer below could have been avoided. essentially you are suggesting, pop every time, and push again if ready_to_run_tasks is not empty. and what i did was pop only if ready_to_run_tasks was empty. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:343: On 2013/12/20 01:53:24, David Reveman wrote: > Here after taking the top task off the queue, you need to add the task_namespace > back to ready_to_run_namespaces_ if task_namespace->ready_to_run_tasks is not > empty. Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:350: task.get(), ready_to_run_task_set->pending_tasks.take_and_erase On 2013/12/20 01:53:24, David Reveman wrote: > nit: move "ready_to_run_task_set->pendi" to the next line Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:356: On 2013/12/20 01:53:24, David Reveman wrote: > nit: no need to add this line Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:373: if (node) { On 2013/12/20 01:53:24, David Reveman wrote: > nit: not need to change indention of this and following lines Done. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:383: ready_to_run_task_set->ready_to_run_tasks.push(dependent_node); On 2013/12/20 01:53:24, David Reveman wrote: > you need to add the task namespace to |ready_to_run_namespaces_| if it was empty > before this Done. This could have been taken care of, if we popped at the end of loop based on ready_to_run_tasks is empty or not. https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:389: ready_to_run_namespaces_.pop(); On 2013/12/20 01:53:24, David Reveman wrote: > what's the purpose of these lines? I think they should just be removed. Done. purpose i have explained above.
https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430001/cc/resources/worker_pool... cc/resources/worker_pool.cc:40: typedef WorkerPool::TaskVector TaskVector; On 2013/12/20 10:09:29, sohanjg wrote: > On 2013/12/20 01:53:24, David Reveman wrote: > > can these typedefs be private? > > SetTaskGraph, CollectCompletedTasks will need the typedef's. either we make the > 2 func's pvt and inherit WorkerPool from TaskGraphRunner, or use some gloabl > scope, whats your opinion ? Current code is fine. Just move the typedefs above the ctor. Types are typically above ctor unless paired with a member. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:195: no need to remove this line. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:342: still no need to remove this line https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:389: no need to remove this space https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:44: // |completed_tasks| queue even if they later get canceled by another do you mind fixing a typo while here? s/even if they later get canceled/even if it later get canceled/ https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:50: TaskVector* completed_tasks); this is still incorrectly indented. please make sure your editor is using spaces for indent and not tabs. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:81: TaskNamespace* b) { still incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:83: b->ready_to_run_tasks.top()); still incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:86: }; indent 2 spaces https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:192: TaskGraph* graph) { still incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:228: it != task_namespace->running_tasks.end(); ++it) { still incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:235: } still incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:256: no need for this blank line. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:266: it != task_namespace->pending_tasks.end(); ++it) { still incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:271: for (TaskNamespaceMap::iterator it =namespaces_.begin(); should be a space between "=" and "namespaces_.beg.." https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:272: it != namespaces_.end(); ++it) { incorrectly indented https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:275: } sorry, I just realized that this loop needs to be done after you swap task_namespace->ready_to_run_tasks. please move it below just above the: if (!ready_to_run_namespaces_.empty()) has_ready_to_run_tasks_cv_.Signal(); code below. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:282: std::swap(ready_to_run_namespaces_, new_ready_to_run_namespaces); and this last line needs to move down too of course https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:297: (const WorkerPool* worker_pool, TaskVector* completed_tasks) { "(" should be on previous line https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:302: completed_tasks->swap(ready_to_run_namespaces_.top()->completed_tasks); this is not right. you want to collect completed tasks for the specified worker pool instance. ready_to_run_namespaces_ has nothing to do with that. How about something like this: DCHECK(namespaces_.find(worker_pool) != namespaces_.end()); completed_tasks->swap(namespaces_[worker_pool]->completed_tasks); https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:315: break; this should be indented 2 spaces. not 4. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:333: } no need for "{" and "}" above https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:342: (task.get())); move "(task.get()));" to previous line. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:362: task.get()); move "task.get());" to previous line https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:367: bool wasempty = false; s/wasempty/was_empty/ and this can be moved to the "if (!dependent_node->num_dependencies())" scope below https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:378: ready_to_run_namespaces_.push(task_namespace); I think this should be: if (!dependent_node->num_dependencies()) { bool was_empty = task_namespace->ready_to_run_tasks.empty(); task_namespace->ready_to_run_tasks.push(dependent_node); if (was_empty) ready_to_run_namespaces_.push(task_namespace); } otherwise you'll push the namespace even if no dependent_node was added. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:393: CompositorRasterTaskGraphRunner(): TaskGraphRunner need a space between "()" and ":" https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:394: (switches::GetNumRasterThreads(), "CompositorRaster") {} move "(" to previous line https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:475: g_task_graph_runner.Pointer()->Shutdown(); There's one more part to this patch that needs to be done and that's the Shutdown logic. WorkerPool::Shutdown() should not shutdown the TaskGraphRunner as there might be other worker pool instances using it. This function should just wait for all tasks associated with this worker pool instance to finish running. You'll need to add a ::WaitForTasksToFinishRunning(const WorkerPool* worker_pool) function to the TaskGraphRunner class, which you'll call here instead of Shutdown(). However, before you start implementing that function I'd suggest that you write a small proposal for how you intend to do it. As getting this right is going to be the hardest part of this patch.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:44: // |completed_tasks| queue even if they later get canceled by another On 2013/12/20 16:16:02, David Reveman wrote: > do you mind fixing a typo while here? > s/even if they later get canceled/even if it later get canceled/ Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:50: TaskVector* completed_tasks); On 2013/12/20 16:16:02, David Reveman wrote: > this is still incorrectly indented. please make sure your editor is using spaces > for indent and not tabs. Done. yes, there was some prob with editor. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:81: TaskNamespace* b) { On 2013/12/20 16:16:02, David Reveman wrote: > still incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:83: b->ready_to_run_tasks.top()); On 2013/12/20 16:16:02, David Reveman wrote: > still incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:86: }; On 2013/12/20 16:16:02, David Reveman wrote: > indent 2 spaces Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:192: TaskGraph* graph) { On 2013/12/20 16:16:02, David Reveman wrote: > still incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:228: it != task_namespace->running_tasks.end(); ++it) { On 2013/12/20 16:16:02, David Reveman wrote: > still incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:235: } On 2013/12/20 16:16:02, David Reveman wrote: > still incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:256: On 2013/12/20 16:16:02, David Reveman wrote: > no need for this blank line. Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:266: it != task_namespace->pending_tasks.end(); ++it) { On 2013/12/20 16:16:02, David Reveman wrote: > still incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:271: for (TaskNamespaceMap::iterator it =namespaces_.begin(); On 2013/12/20 16:16:02, David Reveman wrote: > should be a space between "=" and "namespaces_.beg.." Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:272: it != namespaces_.end(); ++it) { On 2013/12/20 16:16:02, David Reveman wrote: > incorrectly indented Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:275: } On 2013/12/20 16:16:02, David Reveman wrote: > sorry, I just realized that this loop needs to be done after you swap > task_namespace->ready_to_run_tasks. please move it below just above the: > > if (!ready_to_run_namespaces_.empty()) > has_ready_to_run_tasks_cv_.Signal(); > > code below. Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:282: std::swap(ready_to_run_namespaces_, new_ready_to_run_namespaces); On 2013/12/20 16:16:02, David Reveman wrote: > and this last line needs to move down too of course Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:297: (const WorkerPool* worker_pool, TaskVector* completed_tasks) { On 2013/12/20 16:16:02, David Reveman wrote: > "(" should be on previous line Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:302: completed_tasks->swap(ready_to_run_namespaces_.top()->completed_tasks); On 2013/12/20 16:16:02, David Reveman wrote: > this is not right. you want to collect completed tasks for the specified worker > pool instance. ready_to_run_namespaces_ has nothing to do with that. How about > something like this: > > DCHECK(namespaces_.find(worker_pool) != namespaces_.end()); > completed_tasks->swap(namespaces_[worker_pool]->completed_tasks); Done. Yes.this looks good. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:315: break; On 2013/12/20 16:16:02, David Reveman wrote: > this should be indented 2 spaces. not 4. Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:333: } On 2013/12/20 16:16:02, David Reveman wrote: > no need for "{" and "}" above Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:342: (task.get())); On 2013/12/20 16:16:02, David Reveman wrote: > move "(task.get()));" to previous line. Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:362: task.get()); On 2013/12/20 16:16:02, David Reveman wrote: > move "task.get());" to previous line Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:367: bool wasempty = false; On 2013/12/20 16:16:02, David Reveman wrote: > s/wasempty/was_empty/ and this can be moved to the "if > (!dependent_node->num_dependencies())" scope below Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:378: ready_to_run_namespaces_.push(task_namespace); On 2013/12/20 16:16:02, David Reveman wrote: > I think this should be: > > if (!dependent_node->num_dependencies()) { > bool was_empty = task_namespace->ready_to_run_tasks.empty(); > task_namespace->ready_to_run_tasks.push(dependent_node); > if (was_empty) > ready_to_run_namespaces_.push(task_namespace); > } > > otherwise you'll push the namespace even if no dependent_node was added. Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:393: CompositorRasterTaskGraphRunner(): TaskGraphRunner On 2013/12/20 16:16:02, David Reveman wrote: > need a space between "()" and ":" Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:394: (switches::GetNumRasterThreads(), "CompositorRaster") {} On 2013/12/20 16:16:02, David Reveman wrote: > move "(" to previous line Done. https://codereview.chromium.org/73923003/diff/450001/cc/resources/worker_pool... cc/resources/worker_pool.cc:475: g_task_graph_runner.Pointer()->Shutdown(); On 2013/12/20 16:16:02, David Reveman wrote: > There's one more part to this patch that needs to be done and that's the > Shutdown logic. WorkerPool::Shutdown() should not shutdown the TaskGraphRunner > as there might be other worker pool instances using it. This function should > just wait for all tasks associated with this worker pool instance to finish > running. > > You'll need to add a ::WaitForTasksToFinishRunning(const WorkerPool* > worker_pool) function to the TaskGraphRunner class, which you'll call here > instead of Shutdown(). > > However, before you start implementing that function I'd suggest that you write > a small proposal for how you intend to do it. As getting this right is going to > be the hardest part of this patch. I will update my proposal in google doc, and share with you asap.
Consider using clang formatting "git cl format" to avoid all these style mistakes. https://codereview.chromium.org/73923003/diff/490001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/490001/cc/base/switches.cc#newc... cc/base/switches.cc:213: size_t GetNumRasterThreads() { You need to remove LayerTreeSettings::num_raster_threads in cc/trees/layer_tree_settings.h as number of raster threads is no longer a per LTHI setting. It's critical that all code the depend on this setting use the exact same value. That's not the case at the moment as the code used here has some subtle difference compared to the code used in RenderWidgetCompositor. We should also not be duplicating the code. It should only live in once place as anything else would be a nightmare to maintain. I think the best is to remove LayerTreeSettings::num_raster_threads completely and in resources/picture_pile.cc:PicturePile::Update change: picture->CloneForDrawing(num_raster_threads_); to picture->CloneForDrawing(switches::GetNumRasterThreads()); However, this means that the GetNumRasterThreads() function is now on the critical path so we'll need to use a static initializer to avoid a performance regression. Basically just like IsGPURasterizationEnabled() above. I would move the code here (after fixing my comments below) into a CheckNumRasterThreads() function and change this function to: static size_t num_raster_threads = CheckNumRasterThreads(); return num_raster_threads; FYI, there's a subtle race in this code but the code above has the same problem so I think it's better to be consistent and fix that in a follow up patch instead. https://codereview.chromium.org/73923003/diff/490001/cc/base/switches.cc#newc... cc/base/switches.cc:225: std::min(num_threads, kMaxRasterThreads)); This is different from the parsing of this value in RenderWidgetCompositor. RenderWidgetCompositor doesn't clamp the value but instead print a warning and use the default value if it's outside the valid range. It seems like a good idea to keep doing that instead of clamping the value. https://codereview.chromium.org/73923003/diff/490001/cc/base/switches.cc#newc... cc/base/switches.cc:226: } else { nit: style guide prefers no "else" if you return a value above. https://codereview.chromium.org/73923003/diff/490001/cc/base/switches.h File cc/base/switches.h (right): https://codereview.chromium.org/73923003/diff/490001/cc/base/switches.h#newco... cc/base/switches.h:11: #include "cc/base/math_util.h" why are you adding this include? https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:195: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:228: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:240: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:251: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:318: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:342: nit: no need to remove this line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:389: nit: no need to remove this blank line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:429: nit: no need to remove this blank line https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:32: typedef WorkerPool::TaskVector TaskVector; nit: please add a blank line between types and ctor. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:82: return task_comparators_.operator()(a->ready_to_run_tasks.top(), nit: should be indented 2 spaces, not 4 https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:93: std::vector<TaskNamespace*>, nit: "std::vector<TaskName.." vertically aligned with "TaskNames.." https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:94: TaskNamespacePriorityComparator> TaskNamespaceQueue; nit: "TaskNamespacePri.." vertically aligned with "TaskNames.." and TaskNamespaceQueue on the next line and indented 4 spaces. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:227: it != task_namespace->running_tasks.end(); ++it) { nit: "it != task_name.." vertically aligned with "TaskGraph::it.." https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:263: it != task_namespace->pending_tasks.end(); ++it) { nit: "it != task_name.." vertically aligned with "TaskGraph::it.." https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:280: for (TaskNamespaceMap::iterator it =namespaces_.begin(); nit: space between "=" and "namespaces_.b.." https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:281: it != namespaces_.end(); ++it) { nit: "it !=.." vertically aligned with "TaskNamespa.." https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:294: const WorkerPool* worker_pool, TaskVector* completed_tasks) { nit: this line should be indented 4 spaces, not 2 https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:328: if (!task_namespace->ready_to_run_tasks.empty()) Please add something like this comment here: // Add task namespace back to |ready_to_run_namespaces_| if not // empty after taking top priority task. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:334: Please don't add this blank line. The line below should be grouped with the comment and DCHECKs above. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:368: task_namespace->ready_to_run_tasks.push(dependent_node); Please add something like this comment here: // Task namespace is ready if it has at least one ready // to run task. Add it to |ready_to_run_namespaces_| if // it just become ready. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:374: // Finally add task to |completed_tasks|. nit: please keep the blank line between "}" and "// Finally add.." https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:386: CompositorRasterTaskGraphRunner() : TaskGraphRunner( nit: indent 1 space relative to "public:" https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:387: switches::GetNumRasterThreads(), "CompositorRaster") {} nit: indent 4 spaces relative to "CompositorRas.." also, putting the "}" on the next line and indented 1 space relative to "public:" will be more consistent with the rest of this file. ie. WorkerPoolTask ctor below. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:453: nit: only one blank line between "} // namesp.." and "WorkerPool::Wo.." is enough
PTAL Thanks. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:195: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:228: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:240: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:251: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:318: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:342: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:389: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this blank line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:429: On 2013/12/22 15:49:25, David Reveman wrote: > nit: no need to remove this blank line Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:32: typedef WorkerPool::TaskVector TaskVector; On 2013/12/22 15:49:25, David Reveman wrote: > nit: please add a blank line between types and ctor. Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:82: return task_comparators_.operator()(a->ready_to_run_tasks.top(), On 2013/12/22 15:49:25, David Reveman wrote: > nit: should be indented 2 spaces, not 4 Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:93: std::vector<TaskNamespace*>, On 2013/12/22 15:49:25, David Reveman wrote: > nit: "std::vector<TaskName.." vertically aligned with "TaskNames.." Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:94: TaskNamespacePriorityComparator> TaskNamespaceQueue; On 2013/12/22 15:49:25, David Reveman wrote: > nit: "TaskNamespacePri.." vertically aligned with "TaskNames.." and > TaskNamespaceQueue on the next line and indented 4 spaces. Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:227: it != task_namespace->running_tasks.end(); ++it) { On 2013/12/22 15:49:25, David Reveman wrote: > nit: "it != task_name.." vertically aligned with "TaskGraph::it.." Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:263: it != task_namespace->pending_tasks.end(); ++it) { On 2013/12/22 15:49:25, David Reveman wrote: > nit: "it != task_name.." vertically aligned with "TaskGraph::it.." Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:280: for (TaskNamespaceMap::iterator it =namespaces_.begin(); On 2013/12/22 15:49:25, David Reveman wrote: > nit: space between "=" and "namespaces_.b.." Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:281: it != namespaces_.end(); ++it) { On 2013/12/22 15:49:25, David Reveman wrote: > nit: "it !=.." vertically aligned with "TaskNamespa.." Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:294: const WorkerPool* worker_pool, TaskVector* completed_tasks) { On 2013/12/22 15:49:25, David Reveman wrote: > nit: this line should be indented 4 spaces, not 2 Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:328: if (!task_namespace->ready_to_run_tasks.empty()) On 2013/12/22 15:49:25, David Reveman wrote: > Please add something like this comment here: > > // Add task namespace back to |ready_to_run_namespaces_| if not > // empty after taking top priority task. Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:334: On 2013/12/22 15:49:25, David Reveman wrote: > Please don't add this blank line. The line below should be grouped with the > comment and DCHECKs above. Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:368: task_namespace->ready_to_run_tasks.push(dependent_node); On 2013/12/22 15:49:25, David Reveman wrote: > Please add something like this comment here: > > // Task namespace is ready if it has at least one ready > // to run task. Add it to |ready_to_run_namespaces_| if > // it just become ready. Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:374: // Finally add task to |completed_tasks|. On 2013/12/22 15:49:25, David Reveman wrote: > nit: please keep the blank line between "}" and "// Finally add.." Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:386: CompositorRasterTaskGraphRunner() : TaskGraphRunner( On 2013/12/22 15:49:25, David Reveman wrote: > nit: indent 1 space relative to "public:" Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:387: switches::GetNumRasterThreads(), "CompositorRaster") {} On 2013/12/22 15:49:25, David Reveman wrote: > nit: indent 4 spaces relative to "CompositorRas.." > > also, putting the "}" on the next line and indented 1 space relative to > "public:" will be more consistent with the rest of this file. ie. WorkerPoolTask > ctor below. Done. https://codereview.chromium.org/73923003/diff/490001/cc/resources/worker_pool... cc/resources/worker_pool.cc:453: On 2013/12/22 15:49:25, David Reveman wrote: > nit: only one blank line between "} // namesp.." and "WorkerPool::Wo.." is > enough Done.
https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:186: DCHECK_EQ(0u, completed_tasks_.size()); Move these DCHECKs to Unregister. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:96: TaskNamespaceQueue; nit: last line should be indented 4 spaces relative to "typedef std::prio.." https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:111: // tasks associated with a namespace finishes. This cv is not waited on by worker threads. It's waited on by origin threads until a namespace has finished running all associated tasks. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:112: base::ConditionVariable tasks_finished_running_cv_; I'd like the name to better represent how this is used. how about has_namespaces_with_finished_running_tasks_cv_? https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:160: DCHECK_EQ(0u, ready_to_run_namespaces_.size()); Please add a DCHECK_EQ(0u, namespaces_.size()) here too and a blank line between this and DCHECK(!shutdown). https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:195: { this scope is unnecessary. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:198: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); DCHECK(namespaces_.find(worker_pool) != namespaces_.end()) before this line https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:205: else nit: style guide prefer no "else" when "if" has a return statement https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:206: tasks_finished_running_cv_.Wait(); There might be other namespaces for which tasks have finished running. You'll need to wake up another origin thread using tasks_finished_running_cv_.Signal() before you return from this function. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:302: // Re-create the ready_to_run_namespaces_ For consistency, change this comment to: // Build new "ready to run" task namespaces queue. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:407: // Signal that all tasks of this namespace has finished nit: missing "." at end of line. how about we change this comment to: // If namespace has finished running all tasks, wake up origin thread. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:409: task_namespace->running_tasks.empty()) You have this code in at least 2 places. How about we add a TaskNamespace::has_finished_running_tasks() { return task_namespace->pending_tasks.empty() && task_namespace->running_tasks.empty(); }
PTAL Thanks. regarding the switch file changes, you mentioned, you will take care, or should i make the changes ? https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:186: DCHECK_EQ(0u, completed_tasks_.size()); On 2013/12/26 18:08:01, David Reveman wrote: > Move these DCHECKs to Unregister. Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:96: TaskNamespaceQueue; On 2013/12/26 18:08:01, David Reveman wrote: > nit: last line should be indented 4 spaces relative to "typedef std::prio.." Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:111: // tasks associated with a namespace finishes. On 2013/12/26 18:08:01, David Reveman wrote: > This cv is not waited on by worker threads. It's waited on by origin threads > until a namespace has finished running all associated tasks. Done. By origin thread, are you referring to the Compositor thread ? https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:112: base::ConditionVariable tasks_finished_running_cv_; On 2013/12/26 18:08:01, David Reveman wrote: > I'd like the name to better represent how this is used. > how about has_namespaces_with_finished_running_tasks_cv_? Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:160: DCHECK_EQ(0u, ready_to_run_namespaces_.size()); On 2013/12/26 18:08:01, David Reveman wrote: > Please add a DCHECK_EQ(0u, namespaces_.size()) here too and a blank line between > this and DCHECK(!shutdown). Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:195: { On 2013/12/26 18:08:01, David Reveman wrote: > this scope is unnecessary. Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:205: else On 2013/12/26 18:08:01, David Reveman wrote: > nit: style guide prefer no "else" when "if" has a return statement Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:302: // Re-create the ready_to_run_namespaces_ On 2013/12/26 18:08:01, David Reveman wrote: > For consistency, change this comment to: > // Build new "ready to run" task namespaces queue. Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:407: // Signal that all tasks of this namespace has finished On 2013/12/26 18:08:01, David Reveman wrote: > nit: missing "." at end of line. > > how about we change this comment to: > // If namespace has finished running all tasks, wake up origin thread. Done. https://codereview.chromium.org/73923003/diff/560001/cc/resources/worker_pool... cc/resources/worker_pool.cc:409: task_namespace->running_tasks.empty()) On 2013/12/26 18:08:01, David Reveman wrote: > You have this code in at least 2 places. How about we add a > TaskNamespace::has_finished_running_tasks() { > return > task_namespace->pending_tasks.empty() && > task_namespace->running_tasks.empty(); > } Done.
Just some nits. We'll need to make changes to how we handle the command line switch as part of this patch. You can take care of that. See my previous comments for what needs to be done. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:101: // Check finished running tasks nit: remove this comment. I don't think it adds anything on top of the function name. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:117: // namespace has finished running all associated tasks nit: "." at end of comment https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:215: // Wait for finishing all tasks for current namespace nit: I don't think this comment is necessary. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:218: // Wake up origin thread Please change this comment to: // There may be other namespaces that have finished running // tasks, so wake up another origin thread.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:101: // Check finished running tasks On 2013/12/27 09:58:50, David Reveman wrote: > nit: remove this comment. I don't think it adds anything on top of the function > name. Done. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:117: // namespace has finished running all associated tasks On 2013/12/27 09:58:50, David Reveman wrote: > nit: "." at end of comment Done. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:215: // Wait for finishing all tasks for current namespace On 2013/12/27 09:58:50, David Reveman wrote: > nit: I don't think this comment is necessary. Done. https://codereview.chromium.org/73923003/diff/640001/cc/resources/worker_pool... cc/resources/worker_pool.cc:218: // Wake up origin thread On 2013/12/27 09:58:50, David Reveman wrote: > Please change this comment to: > > // There may be other namespaces that have finished running > // tasks, so wake up another origin thread. Done.
https://codereview.chromium.org/73923003/diff/690001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/690001/cc/base/switches.cc#newc... cc/base/switches.cc:213: size_t CheckNumRasterThreads() { move this function to anonymous namespace. https://codereview.chromium.org/73923003/diff/690001/cc/base/switches.cc#newc... cc/base/switches.cc:224: num_threads >= kMinRasterThreads && num_threads <= kMaxRasterThreads)) { This is wrong. You're checking the value of num_threads if base::StringToInt fails. That doesn't make sense. You probably meant to do: if (!(base::StringToInt(string_value, &num_threads) || num_threads < kMinRasterThreads || num_threads > kMaxRasterThreads) Might be cleaner to do: if (base::StringToInt(string_value, &num_threads) && num_threads >= kMinRasterThreads && num_threads <= kMaxRasterThreads) return num_threads; LOG(WARNING) << .... https://codereview.chromium.org/73923003/diff/690001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/690001/cc/resources/picture_pil... cc/resources/picture_pile.cc:238: picture->CloneForDrawing(switches::GetNumRasterThreads()); please remove |num_raster_threads_| and set_num_raster_threads() as they are no longer used. https://codereview.chromium.org/73923003/diff/690001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/690001/cc/resources/worker_pool... cc/resources/worker_pool.h:121: WorkerPool(size_t num_threads, const std::string& thread_name_prefix); |num_threads| and |thread_name_prefix| are no longer used. Please remove them. https://codereview.chromium.org/73923003/diff/690001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/73923003/diff/690001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1725: switches::GetNumRasterThreads(), num_raster_threads is not used by TileManager, PixelBufferRasterWorkerPool, ImageRasterWorkerPool or WorkerPool anymore. Please remove it.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/690001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/690001/cc/base/switches.cc#newc... cc/base/switches.cc:213: size_t CheckNumRasterThreads() { On 2013/12/27 14:57:13, David Reveman wrote: > move this function to anonymous namespace. Done. https://codereview.chromium.org/73923003/diff/690001/cc/base/switches.cc#newc... cc/base/switches.cc:224: num_threads >= kMinRasterThreads && num_threads <= kMaxRasterThreads)) { On 2013/12/27 14:57:13, David Reveman wrote: > This is wrong. You're checking the value of num_threads if base::StringToInt > fails. That doesn't make sense. You probably meant to do: > if (!(base::StringToInt(string_value, &num_threads) || > num_threads < kMinRasterThreads || > num_threads > kMaxRasterThreads) > > Might be cleaner to do: > if (base::StringToInt(string_value, &num_threads) && > num_threads >= kMinRasterThreads && > num_threads <= kMaxRasterThreads) > return num_threads; > > LOG(WARNING) << .... Done. https://codereview.chromium.org/73923003/diff/690001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/690001/cc/resources/picture_pil... cc/resources/picture_pile.cc:238: picture->CloneForDrawing(switches::GetNumRasterThreads()); On 2013/12/27 14:57:13, David Reveman wrote: > please remove |num_raster_threads_| and set_num_raster_threads() as they are no > longer used. Done. https://codereview.chromium.org/73923003/diff/690001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/690001/cc/resources/worker_pool... cc/resources/worker_pool.h:121: WorkerPool(size_t num_threads, const std::string& thread_name_prefix); On 2013/12/27 14:57:13, David Reveman wrote: > |num_threads| and |thread_name_prefix| are no longer used. Please remove them. Done. https://codereview.chromium.org/73923003/diff/690001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/73923003/diff/690001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:1725: switches::GetNumRasterThreads(), On 2013/12/27 14:57:13, David Reveman wrote: > num_raster_threads is not used by TileManager, PixelBufferRasterWorkerPool, > ImageRasterWorkerPool or WorkerPool anymore. Please remove it. Done.
https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:196: int num_threads = kDefaultNumRasterThreads; Please move this just above the "base::StringToInt(.." line now that you return early. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:202: return num_threads; nit: this line should be indented 2 spaces, not 4 https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:203: LOG(WARNING) << "Failed to parse switch " << nit: maybe add a blank line between this and the above return statement https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:206: return num_threads; nit: return kDefaultNumRasterThreads here instead https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:233: static size_t num_raster_threads = CheckNumRasterThreads(); FYI, you'll have to update this if https://codereview.chromium.org/122063002 lands before this change. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.h File cc/base/switches.h (right): https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.h#newco... cc/base/switches.h:11: #include "cc/base/math_util.h" I still don't know why you're adding this include. Please remove it if not needed. https://codereview.chromium.org/73923003/diff/770001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/layers/picture_layer.... cc/layers/picture_layer.cc:7: #include "cc/base/switches.h" no need to add this include. please remove it. https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... cc/resources/picture_pile.cc:235: size_t num_raster_threads = switches::GetNumRasterThreads(); FYI, you'll need to add lazy initialization of this if https://codereview.chromium.org/122063002/ lands before this patch. https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... cc/resources/picture_pile_impl.cc:55: this, switches::GetNumRasterThreads())) { nit: this line should be indented 4 spaces relative to "ClonesForDrawi..". https://codereview.chromium.org/73923003/diff/770001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:464: : WorkerPool(), nit: WorkerPool() not needed. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:11: #include "base/command_line.h" please remove this include if not needed. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:216: // There maybe other namespaces that have finished running nit: s/maybe/may be/ https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:217: // running tasks, so wake up another origin thread nit: "running" twice. remove "running" from this line and add a "." at the end of the line. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:503: : in_dispatch_completion_callbacks_(false) { nit: this will fit on previous line. https://codereview.chromium.org/73923003/diff/770001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:20: #include "cc/base/switches.h" no need for this include. please remove it.
PTAL. Can you please add/suggest me to add, other owners for this review, pre-submit is throwing up some warnings for that. Thanks. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:196: int num_threads = kDefaultNumRasterThreads; On 2013/12/28 14:45:48, David Reveman wrote: > Please move this just above the "base::StringToInt(.." line now that you return > early. Done. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:202: return num_threads; On 2013/12/28 14:45:48, David Reveman wrote: > nit: this line should be indented 2 spaces, not 4 Done. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:203: LOG(WARNING) << "Failed to parse switch " << On 2013/12/28 14:45:48, David Reveman wrote: > nit: maybe add a blank line between this and the above return statement Done. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:206: return num_threads; On 2013/12/28 14:45:48, David Reveman wrote: > nit: return kDefaultNumRasterThreads here instead Done. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.cc#newc... cc/base/switches.cc:233: static size_t num_raster_threads = CheckNumRasterThreads(); On 2013/12/28 14:45:48, David Reveman wrote: > FYI, you'll have to update this if https://codereview.chromium.org/122063002 > lands before this change. OK.will take care of it, if that happens. https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.h File cc/base/switches.h (right): https://codereview.chromium.org/73923003/diff/770001/cc/base/switches.h#newco... cc/base/switches.h:11: #include "cc/base/math_util.h" On 2013/12/28 14:45:48, David Reveman wrote: > I still don't know why you're adding this include. Please remove it if not > needed. Done. I had added this for size_t and LOG usage, i have now replaced it by base/basictypes.h here, and base/logging.h in the .cc file, hope its fine. https://codereview.chromium.org/73923003/diff/770001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/layers/picture_layer.... cc/layers/picture_layer.cc:7: #include "cc/base/switches.h" On 2013/12/28 14:45:48, David Reveman wrote: > no need to add this include. please remove it. Done. https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... cc/resources/picture_pile.cc:235: size_t num_raster_threads = switches::GetNumRasterThreads(); On 2013/12/28 14:45:48, David Reveman wrote: > FYI, you'll need to add lazy initialization of this if > https://codereview.chromium.org/122063002/ lands before this patch. OK.will take care of it, if that happens. https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/picture_pil... cc/resources/picture_pile_impl.cc:55: this, switches::GetNumRasterThreads())) { On 2013/12/28 14:45:48, David Reveman wrote: > nit: this line should be indented 4 spaces relative to "ClonesForDrawi..". Done. https://codereview.chromium.org/73923003/diff/770001/cc/resources/raster_work... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/raster_work... cc/resources/raster_worker_pool.cc:464: : WorkerPool(), On 2013/12/28 14:45:48, David Reveman wrote: > nit: WorkerPool() not needed. Done. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:11: #include "base/command_line.h" On 2013/12/28 14:45:48, David Reveman wrote: > please remove this include if not needed. Done. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:216: // There maybe other namespaces that have finished running On 2013/12/28 14:45:48, David Reveman wrote: > nit: s/maybe/may be/ Done. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:217: // running tasks, so wake up another origin thread On 2013/12/28 14:45:48, David Reveman wrote: > nit: "running" twice. remove "running" from this line and add a "." at the end > of the line. Done. https://codereview.chromium.org/73923003/diff/770001/cc/resources/worker_pool... cc/resources/worker_pool.cc:503: : in_dispatch_completion_callbacks_(false) { On 2013/12/28 14:45:48, David Reveman wrote: > nit: this will fit on previous line. Done. https://codereview.chromium.org/73923003/diff/770001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/73923003/diff/770001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_impl.cc:20: #include "cc/base/switches.h" On 2013/12/28 14:45:48, David Reveman wrote: > no need for this include. please remove it. Done.
Looking good. A few minor issues. Please update the description and have Vlad look at this next. Vlad, fyi I'm planning to follow up with a cleanup/refactor change that removes the base WorkerPool class and makes RasterWorkerPool use a TaskGraphRunner instance directly. https://codereview.chromium.org/73923003/diff/910001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/910001/cc/base/switches.cc#newc... cc/base/switches.cc:201: if ((base::StringToInt(string_value, &num_threads) && just "(" instead of "((" https://codereview.chromium.org/73923003/diff/910001/cc/base/switches.cc#newc... cc/base/switches.cc:202: num_threads >= kMinRasterThreads && num_threads <= kMaxRasterThreads)) just ")" instead of "))" https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:134: // This set contains all pending tasks. nit: please keep this comment by moving it to "struct TaskNamespace" https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:143: // This set contains all currently running tasks. nit: please keep this comment by moving it to "struct TaskNamespace" https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:146: // Completed tasks not yet collected by origin thread. nit: please keep this comment by moving it to "struct TaskNamespace" https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:38: void WaitForTasksToFinishRunning(const WorkerPool* worker_pool); Please move the declaration of "WaitForTasksToFinishRunning" below SetTaskGraph and add this comment above it: // Wait for all scheduled tasks to finish running. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:66: // Ordered set of tasks that are ready to run. This comment is supposed to be next to "ready_to_run_tasks" in TaskNamespace. Please move it. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:91: // Ordered set of tasks namespaces that have ready to run tasks. Please move this comment above |ready_to_run_namespaces_| instead. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:126: ScopedPtrDeque<base::DelegateSimpleThread> workers_; nit: Let's keep this at the bottom, below |ready_to_run_namespaces_| https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:128: TaskNamespaceMap namespaces_; Add this comment here: // This set contains all registered namespaces. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:227: DCHECK(graph->empty() || !shutdown_); This DCHECK is no longer correct. Please make it "DCHECK(!shutdown_)" and move it below "base::AutoLock lock(lock_);" as shutdown_ is protected by lock.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/910001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/910001/cc/base/switches.cc#newc... cc/base/switches.cc:201: if ((base::StringToInt(string_value, &num_threads) && On 2013/12/30 08:57:06, David Reveman wrote: > just "(" instead of "((" Done. https://codereview.chromium.org/73923003/diff/910001/cc/base/switches.cc#newc... cc/base/switches.cc:202: num_threads >= kMinRasterThreads && num_threads <= kMaxRasterThreads)) On 2013/12/30 08:57:06, David Reveman wrote: > just ")" instead of "))" Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:134: // This set contains all pending tasks. On 2013/12/30 08:57:06, David Reveman wrote: > nit: please keep this comment by moving it to "struct TaskNamespace" Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:143: // This set contains all currently running tasks. On 2013/12/30 08:57:06, David Reveman wrote: > nit: please keep this comment by moving it to "struct TaskNamespace" Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:146: // Completed tasks not yet collected by origin thread. On 2013/12/30 08:57:06, David Reveman wrote: > nit: please keep this comment by moving it to "struct TaskNamespace" Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:38: void WaitForTasksToFinishRunning(const WorkerPool* worker_pool); On 2013/12/30 08:57:06, David Reveman wrote: > Please move the declaration of "WaitForTasksToFinishRunning" below SetTaskGraph > and add this comment above it: > > // Wait for all scheduled tasks to finish running. Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:66: // Ordered set of tasks that are ready to run. On 2013/12/30 08:57:06, David Reveman wrote: > This comment is supposed to be next to "ready_to_run_tasks" in TaskNamespace. > Please move it. Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:91: // Ordered set of tasks namespaces that have ready to run tasks. On 2013/12/30 08:57:06, David Reveman wrote: > Please move this comment above |ready_to_run_namespaces_| instead. Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:126: ScopedPtrDeque<base::DelegateSimpleThread> workers_; On 2013/12/30 08:57:06, David Reveman wrote: > nit: Let's keep this at the bottom, below |ready_to_run_namespaces_| Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:128: TaskNamespaceMap namespaces_; On 2013/12/30 08:57:06, David Reveman wrote: > Add this comment here: > // This set contains all registered namespaces. Done. https://codereview.chromium.org/73923003/diff/910001/cc/resources/worker_pool... cc/resources/worker_pool.cc:227: DCHECK(graph->empty() || !shutdown_); On 2013/12/30 08:57:06, David Reveman wrote: > This DCHECK is no longer correct. Please make it "DCHECK(!shutdown_)" and move > it below "base::AutoLock lock(lock_);" as shutdown_ is protected by lock. Done.
https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... cc/resources/picture_pile.cc:239: picture->CloneForDrawing(static_cast<int>(num_raster_threads)); Might as well switch CloneForDrawing to take size_t instead, there's no good reason it's an int https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:103: bool operator()(const internal::GraphNode* a, Why non-const? https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: bool operator()(TaskNamespace* a, If we define GraphNode::operator<() instead of TaskPriorityComparator, then this can just be "return a->ready_to_run_tasks.top() < b->ready_to_run_tasks.top()" (accounting for possibility of empty queues of course) and you don't need task_comparators_ member then. This would make it less explicit that operator< compares priorities of two graph nodes, but I think that's OK? Either way is fine, I guess... This is just slightly more complicated https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:87: return task_comparators_.operator()(a->ready_to_run_tasks.top(), DCHECK that the queues aren't empty, please. (I know that it shouldn't be possible, but an extra check won't hurt) https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:90: TaskPriorityComparator task_comparators_; nit: task_comparator_? https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:203: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); Save the iterator from find here, so that you can erase by iterator https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:219: while (true) { I think this is equivalent to the following: while (!has_finished_running_tasks(task_namespace)) has_namespace_with_finished_running_tasks_cv_.Wait(); has_namespace_with_finished_running_tasks_cv_.Signal(); https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:416: ready_to_run_namespaces_.push(task_namespace); I think this might not be correct. By adding a new ready_to_run_tasks node, you might be changing the relative priority of the task_namespace (ie, the new node is now top of the queue in the namespace, so the whole namespace has a higher priority). So, if the ready_to_run_namespaces already contained task_namespace, its position in the queue might not be correct.
https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:47: // Wait for all scheduled tasks to finish running nit: end line with "." https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:133: // Ordered set of tasks namespaces that have ready to run tasks. nit: s/set of tasks namespaces/set of task namespaces/ https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:203: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); On 2013/12/30 21:13:01, vmpstr wrote: > Save the iterator from find here, so that you can erase by iterator Alternatively, use "namespaces_[worker_pool]" directly in the DCHECKs where we don't care about performance. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:416: ready_to_run_namespaces_.push(task_namespace); On 2013/12/30 21:13:01, vmpstr wrote: > I think this might not be correct. By adding a new ready_to_run_tasks node, you > might be changing the relative priority of the task_namespace (ie, the new node > is now top of the queue in the namespace, so the whole namespace has a higher > priority). So, if the ready_to_run_namespaces already contained task_namespace, > its position in the queue might not be correct. Good call. I guess we'll have to iterate over all namespaces and build a new ready_to_run_namespaces queue after having added one or more dependent nodes to a namespace with a non-empty |ready_to_run_tasks|.
https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:416: ready_to_run_namespaces_.push(task_namespace); On 2013/12/31 01:16:52, David Reveman wrote: > On 2013/12/30 21:13:01, vmpstr wrote: > > I think this might not be correct. By adding a new ready_to_run_tasks node, > you > > might be changing the relative priority of the task_namespace (ie, the new > node > > is now top of the queue in the namespace, so the whole namespace has a higher > > priority). So, if the ready_to_run_namespaces already contained > task_namespace, > > its position in the queue might not be correct. > > Good call. I guess we'll have to iterate over all namespaces and build a new > ready_to_run_namespaces queue after having added one or more dependent nodes to > a namespace with a non-empty |ready_to_run_tasks|. An alternative would be to just make ready_to_run_namespaces a std::vector and just sort it when needed. That might just be the cleanest and most efficient.
Please let me know your opinion on the comments. Thanks https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... cc/resources/picture_pile.cc:239: picture->CloneForDrawing(static_cast<int>(num_raster_threads)); On 2013/12/30 21:13:01, vmpstr wrote: > Might as well switch CloneForDrawing to take size_t instead, there's no good > reason it's an int If we make CloneForDrawing() take size_t param, then again we need to do a cast while calling, SkPicture::clone(SkPicture* pictures, int count). Not sure what would be the best thing, make CloneForDrawing() take size_t and cast while calling clone(), or cast here, and keep the rest of the code as is. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:103: bool operator()(const internal::GraphNode* a, On 2013/12/30 21:13:01, vmpstr wrote: > Why non-const? Had to drop the const to avoid code duplication, otherwise we had to put the same comparison check in TaskNamespacePriorityComparator, as a->ready_to_run_tasks.top() and b->ready_to_run_tasks.top() are non-constant. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:47: // Wait for all scheduled tasks to finish running On 2013/12/31 01:16:52, David Reveman wrote: > nit: end line with "." Done. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: bool operator()(TaskNamespace* a, On 2013/12/30 21:13:01, vmpstr wrote: > If we define GraphNode::operator<() instead of TaskPriorityComparator, then this > can just be "return a->ready_to_run_tasks.top() < b->ready_to_run_tasks.top()" > (accounting for possibility of empty queues of course) and you don't need > task_comparators_ member then. > > This would make it less explicit that operator< compares priorities of two graph > nodes, but I think that's OK? Either way is fine, I guess... This is just > slightly more complicated As you pointed out, doing it with "operator <()" will not be as explicit as doing it with TaskpriorityComparator, which is what we are actually doing, comparing the task priority of the nodes. And here comparing the TaskNamespace priority based on ready_to_run_tasks nodes for each WorkerPool instance. imo this should be fine. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:87: return task_comparators_.operator()(a->ready_to_run_tasks.top(), On 2013/12/30 21:13:01, vmpstr wrote: > DCHECK that the queues aren't empty, please. (I know that it shouldn't be > possible, but an extra check won't hurt) Should we do it without holding the lock here ? https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:90: TaskPriorityComparator task_comparators_; On 2013/12/30 21:13:01, vmpstr wrote: > nit: task_comparator_? Done. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:133: // Ordered set of tasks namespaces that have ready to run tasks. On 2013/12/31 01:16:52, David Reveman wrote: > nit: s/set of tasks namespaces/set of task namespaces/ Done. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:203: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); On 2013/12/30 21:13:01, vmpstr wrote: > Save the iterator from find here, so that you can erase by iterator Done. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:203: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); On 2013/12/31 01:16:52, David Reveman wrote: > On 2013/12/30 21:13:01, vmpstr wrote: > > Save the iterator from find here, so that you can erase by iterator > > Alternatively, use "namespaces_[worker_pool]" directly in the DCHECKs where we > don't care about performance. Think Vlad was referring to namespaces_.find(worker_pool). Should we still need to access "namespaces_[worker_pool]" multiple times in DCHECK's? https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:219: while (true) { On 2013/12/30 21:13:01, vmpstr wrote: > I think this is equivalent to the following: > > while (!has_finished_running_tasks(task_namespace)) > has_namespace_with_finished_running_tasks_cv_.Wait(); > > has_namespace_with_finished_running_tasks_cv_.Signal(); Done. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:416: ready_to_run_namespaces_.push(task_namespace); On 2013/12/31 03:37:29, David Reveman wrote: > On 2013/12/31 01:16:52, David Reveman wrote: > > On 2013/12/30 21:13:01, vmpstr wrote: > > > I think this might not be correct. By adding a new ready_to_run_tasks node, > > you > > > might be changing the relative priority of the task_namespace (ie, the new > > node > > > is now top of the queue in the namespace, so the whole namespace has a > higher > > > priority). So, if the ready_to_run_namespaces already contained > > task_namespace, > > > its position in the queue might not be correct. > > > > Good call. I guess we'll have to iterate over all namespaces and build a new > > ready_to_run_namespaces queue after having added one or more dependent nodes > to > > a namespace with a non-empty |ready_to_run_tasks|. > > An alternative would be to just make ready_to_run_namespaces a std::vector and > just sort it when needed. That might just be the cleanest and most efficient. I am not too sure about this. When we push a new ready_to_run_tasks node it will also do a push_heap to re-order the queue based on node priority, and it may not be top of the queue in namespace. And, if ready_to_run_namespaces already has task_namespace, it will again be re-ordered correctly based on priority of the top node of ready_to_run_tasks. Am i missing something here ?
https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... cc/resources/picture_pile.cc:239: picture->CloneForDrawing(static_cast<int>(num_raster_threads)); On 2013/12/31 06:31:25, sohanjg wrote: > On 2013/12/30 21:13:01, vmpstr wrote: > > Might as well switch CloneForDrawing to take size_t instead, there's no good > > reason it's an int > > If we make CloneForDrawing() take size_t param, then again we need to do a cast > while calling, SkPicture::clone(SkPicture* pictures, int count). Not sure what > would be the best thing, make CloneForDrawing() take size_t and cast while > calling clone(), or cast here, and keep the rest of the code as is. > Maybe switches::GetNumRasterThreads() should return an int instead. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:85: bool operator()(TaskNamespace* a, On 2013/12/31 06:31:25, sohanjg wrote: > On 2013/12/30 21:13:01, vmpstr wrote: > > If we define GraphNode::operator<() instead of TaskPriorityComparator, then > this > > can just be "return a->ready_to_run_tasks.top() < b->ready_to_run_tasks.top()" > > (accounting for possibility of empty queues of course) and you don't need > > task_comparators_ member then. > > > > This would make it less explicit that operator< compares priorities of two > graph > > nodes, but I think that's OK? Either way is fine, I guess... This is just > > slightly more complicated > > As you pointed out, doing it with "operator <()" will not be as explicit as > doing it with TaskpriorityComparator, which is what we are actually doing, > comparing the task priority of the nodes. And here comparing the TaskNamespace > priority based on ready_to_run_tasks nodes for each WorkerPool instance. > imo this should be fine. Please rebase this onto: https://codereview.chromium.org/123113002/ and add a new CompareTaskNamespacePriority function that uses CompareTaskPriority. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:87: return task_comparators_.operator()(a->ready_to_run_tasks.top(), On 2013/12/31 06:31:25, sohanjg wrote: > On 2013/12/30 21:13:01, vmpstr wrote: > > DCHECK that the queues aren't empty, please. (I know that it shouldn't be > > possible, but an extra check won't hurt) > > Should we do it without holding the lock here ? No need to worry about the lock here. That's the callers responsibility and it should always be held when this is used. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:203: TaskNamespace* task_namespace = namespaces_[worker_pool].get(); On 2013/12/31 06:31:25, sohanjg wrote: > On 2013/12/31 01:16:52, David Reveman wrote: > > On 2013/12/30 21:13:01, vmpstr wrote: > > > Save the iterator from find here, so that you can erase by iterator > > > > Alternatively, use "namespaces_[worker_pool]" directly in the DCHECKs where we > > don't care about performance. > > Think Vlad was referring to namespaces_.find(worker_pool). > Should we still need to access "namespaces_[worker_pool]" multiple times in > DCHECK's? Multiple lookups in DCHECKs are fine but we shouldn't have that in release builds. If you just remove the "task_namespace = name..." line above and change all DCHECKs to DCHECK_EQ(0u, namespaces_[worker_pool].*_tasks.size()) then we're good, imo. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:416: ready_to_run_namespaces_.push(task_namespace); On 2013/12/31 06:31:25, sohanjg wrote: > On 2013/12/31 03:37:29, David Reveman wrote: > > On 2013/12/31 01:16:52, David Reveman wrote: > > > On 2013/12/30 21:13:01, vmpstr wrote: > > > > I think this might not be correct. By adding a new ready_to_run_tasks > node, > > > you > > > > might be changing the relative priority of the task_namespace (ie, the new > > > node > > > > is now top of the queue in the namespace, so the whole namespace has a > > higher > > > > priority). So, if the ready_to_run_namespaces already contained > > > task_namespace, > > > > its position in the queue might not be correct. > > > > > > Good call. I guess we'll have to iterate over all namespaces and build a new > > > ready_to_run_namespaces queue after having added one or more dependent nodes > > to > > > a namespace with a non-empty |ready_to_run_tasks|. > > > > An alternative would be to just make ready_to_run_namespaces a std::vector and > > just sort it when needed. That might just be the cleanest and most efficient. > > I am not too sure about this. > When we push a new ready_to_run_tasks node it will also do a push_heap to > re-order the queue based on node priority, and it may not be top of the queue in > namespace. > And, if ready_to_run_namespaces already has task_namespace, it will again be > re-ordered correctly based on priority of the top node of ready_to_run_tasks. > Am i missing something here ? The priority queue will not detect changes to elements already in the queue. If the top task of a namespace already in ready_to_run_namespaces changes, then ready_to_run_namespaces will be incorrect. I think we should use std::make_heap, std::pop_heap, and std::push_heap directly instead of std::priority_queue. That allows us to call std::make_heap when necessary, which is more efficient than building a new std::priority_queue. This makes sense on its own so I put together a patch that we can land before shared worker threads: https://codereview.chromium.org/123113002/ Please rebase this patch onto that and switch to using std::make_heap etc for |ready_to_run_task_namespaces_| in a way that's consistent with how |ready_to_run_tasks_| is used. https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:438: switches::GetNumRasterThreads(), "CompositorRaster") { FYI, we're probably going to have to remove the use of switches:: here and instead add some API for setting number of raster threads and retrieving the value here and in PicturePile::Update(). What has to be done depends on the discussion here: https://codereview.chromium.org/122063002/
https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/picture_pil... cc/resources/picture_pile.cc:239: picture->CloneForDrawing(static_cast<int>(num_raster_threads)); On 2014/01/02 03:50:08, David Reveman wrote: > On 2013/12/31 06:31:25, sohanjg wrote: > > On 2013/12/30 21:13:01, vmpstr wrote: > > > Might as well switch CloneForDrawing to take size_t instead, there's no good > > > reason it's an int > > > > If we make CloneForDrawing() take size_t param, then again we need to do a > cast > > while calling, SkPicture::clone(SkPicture* pictures, int count). Not sure what > > would be the best thing, make CloneForDrawing() take size_t and cast while > > calling clone(), or cast here, and keep the rest of the code as is. > > > > Maybe switches::GetNumRasterThreads() should return an int instead. My thought was just if we can do this with no cast, it would be better. Returning an int would work, I think. I mean, SkPicture::clone signature is the limiting factor here https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:416: ready_to_run_namespaces_.push(task_namespace); On 2014/01/02 03:50:08, David Reveman wrote: > On 2013/12/31 06:31:25, sohanjg wrote: > > On 2013/12/31 03:37:29, David Reveman wrote: > > > On 2013/12/31 01:16:52, David Reveman wrote: > > > > On 2013/12/30 21:13:01, vmpstr wrote: > > > > > I think this might not be correct. By adding a new ready_to_run_tasks > > node, > > > > you > > > > > might be changing the relative priority of the task_namespace (ie, the > new > > > > node > > > > > is now top of the queue in the namespace, so the whole namespace has a > > > higher > > > > > priority). So, if the ready_to_run_namespaces already contained > > > > task_namespace, > > > > > its position in the queue might not be correct. > > > > > > > > Good call. I guess we'll have to iterate over all namespaces and build a > new > > > > ready_to_run_namespaces queue after having added one or more dependent > nodes > > > to > > > > a namespace with a non-empty |ready_to_run_tasks|. > > > > > > An alternative would be to just make ready_to_run_namespaces a std::vector > and > > > just sort it when needed. That might just be the cleanest and most > efficient. > > > > I am not too sure about this. > > When we push a new ready_to_run_tasks node it will also do a push_heap to > > re-order the queue based on node priority, and it may not be top of the queue > in > > namespace. > > And, if ready_to_run_namespaces already has task_namespace, it will again be > > re-ordered correctly based on priority of the top node of ready_to_run_tasks. > > Am i missing something here ? > > The priority queue will not detect changes to elements already in the queue. If > the top task of a namespace already in ready_to_run_namespaces changes, then > ready_to_run_namespaces will be incorrect. > > I think we should use std::make_heap, std::pop_heap, and std::push_heap directly > instead of std::priority_queue. That allows us to call std::make_heap when > necessary, which is more efficient than building a new std::priority_queue. This > makes sense on its own so I put together a patch that we can land before shared > worker threads: > https://codereview.chromium.org/123113002/ > > Please rebase this patch onto that and switch to using std::make_heap etc for > |ready_to_run_task_namespaces_| in a way that's consistent with how > |ready_to_run_tasks_| is used. I think make_heap is a good solution to this. I don't think we expect too many namespaces for this to be inefficient, right? I mean we could even sort every time, but I think make_heap is better.
https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc File cc/resources/worker_pool.cc (left): https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... cc/resources/worker_pool.cc:103: bool operator()(const internal::GraphNode* a, On 2013/12/31 06:31:25, sohanjg wrote: > On 2013/12/30 21:13:01, vmpstr wrote: > > Why non-const? > > Had to drop the const to avoid code duplication, otherwise we had to put the > same comparison check in TaskNamespacePriorityComparator, as > a->ready_to_run_tasks.top() and b->ready_to_run_tasks.top() are non-constant. I'm not sure I understand. priority_queue::top() is const, I think. That and a non-const to const cast is implicit, so it shouldn't be a problem... (I could be missing something though :P)
On 2014/01/02 05:38:55, vmpstr wrote: > https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool.cc > File cc/resources/worker_pool.cc (left): > > https://codereview.chromium.org/73923003/diff/430003/cc/resources/worker_pool... > cc/resources/worker_pool.cc:103: bool operator()(const internal::GraphNode* a, > On 2013/12/31 06:31:25, sohanjg wrote: > > On 2013/12/30 21:13:01, vmpstr wrote: > > > Why non-const? > > > > Had to drop the const to avoid code duplication, otherwise we had to put the > > same comparison check in TaskNamespacePriorityComparator, as > > a->ready_to_run_tasks.top() and b->ready_to_run_tasks.top() are non-constant. > > I'm not sure I understand. priority_queue::top() is const, I think. That and a > non-const to const cast is implicit, so it shouldn't be a problem... (I could be > missing something though :P) I have rebased and updated ready_to_run_task_namespaces_ code. Regarding racy initialization code ins switches, should we do as follow up, or part of this patch?
I think we should remove the changes to cc/base/switches.* and instead add: // Sets the number of threads to use for running raster tasks. // Can only be called once prior to GetNumRasterThreads(). // Caller is responsible for correct ordering. static void SetNumRasterThreads(int num_threads); // Gets the number of threads to use for running raster tasks. static int GetNumRasterThreads(); to the RasterWorkerPool interface and have RenderThreadImpl::Init() call cc::RasterWorkerPool::SetNumRasterThreads(). https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:187: DCHECK_EQ(0u, namespaces_[worker_pool]->pending_tasks.size()); if you're using "namespaces_[worker_pool]" here, then there's no need for the iterator above. You can make the DCHECK above: DCHECK(namespaces_.find(worker_pool) == namespaces_.end()); and erase by key below instead: namespaces_.erase(worker_pool); https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:208: return; no need for a return statement here https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:368: ready_to_run_namespaces_.push_back(task_namespace); you're missing a std::push_heap call here. https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:414: if (was_empty) { Please add this DCHECK here: DCHECK(std::find(ready_to_run_namespaces_.begin(), ready_to_run_namespaces_.end(), task_namespace) == ready_to_run_namespaces_.end());
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:187: DCHECK_EQ(0u, namespaces_[worker_pool]->pending_tasks.size()); On 2014/01/03 19:14:03, David Reveman wrote: > if you're using "namespaces_[worker_pool]" here, then there's no need for the > iterator above. You can make the DCHECK above: > DCHECK(namespaces_.find(worker_pool) == namespaces_.end()); > and erase by key below instead: > namespaces_.erase(worker_pool); Done. https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:208: return; On 2014/01/03 19:14:03, David Reveman wrote: > no need for a return statement here Done. https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:368: ready_to_run_namespaces_.push_back(task_namespace); On 2014/01/03 19:14:03, David Reveman wrote: > you're missing a std::push_heap call here. Done. https://codereview.chromium.org/73923003/diff/1310001/cc/resources/worker_poo... cc/resources/worker_pool.cc:414: if (was_empty) { On 2014/01/03 19:14:03, David Reveman wrote: > Please add this DCHECK here: > DCHECK(std::find(ready_to_run_namespaces_.begin(), > ready_to_run_namespaces_.end(), > task_namespace) == > ready_to_run_namespaces_.end()); Done.
https://codereview.chromium.org/73923003/diff/1390001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/base/switches.cc#new... cc/base/switches.cc:8: #include "base/logging.h" no need to add this include https://codereview.chromium.org/73923003/diff/1390001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/picture_pi... cc/resources/picture_pile.cc:12: #include "cc/base/switches.h" no need to add this include https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:471: int RasterWorkerPool::num_raster_threads_ = kDefaultNumRasterThreads; move this to anonymous namespace above. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:472: void RasterWorkerPool::SetNumRasterThreads() { the logic below should not be here. the idea is to not have any command line parsing in cc/. this function should take a "num_thread" value instead and be called from content/renderer/render_thread_impl.cc where command line parsing can be done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:286: static int num_raster_threads_; no need for this to be in the header. put it in an anonymous namespace in the .cc file instead. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... cc/resources/worker_pool.cc:20: #include "cc/resources/raster_worker_pool.h" please avoid to include raster_worker_pool.h here. I have a follow up patch that will fix this circular dependency put please do something like add a "static int WorkerPool::GetNumWorkerThreads()" method that is implemented in raster_worker_pool.cc by just calling RasterWorkerPool::GetNumRasterThreads() for now instead of including this header here. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... cc/resources/worker_pool.cc:419: ready_to_run_namespaces_.end(), nit: indent should vertically align with "ready_to_run_namesp.." https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... cc/resources/worker_pool.cc:420: task_namespace) == ready_to_run_namespaces_.end()); nit: indent should vertically align with "ready_to_run_namespaces_.begin()". if you need to break the line. break it just after "==" and "ready_to_run_namespaces_.end()" should in that case vertically align with "std::find".
On 2014/01/06 07:13:11, David Reveman wrote: > https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... > cc/resources/raster_worker_pool.cc:472: void > RasterWorkerPool::SetNumRasterThreads() { > the logic below should not be here. the idea is to not have any command line > parsing in cc/. this function should take a "num_thread" value instead and be > called from content/renderer/render_thread_impl.cc where command line parsing > can be done. > For render_thread_impl.cc, init() cannot recognize switches::kNumRasterThreads, so i had moved it to RWP. Will RenderThread know about the switch ?
On 2014/01/06 07:13:11, David Reveman wrote: > https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... > cc/resources/raster_worker_pool.cc:472: void > RasterWorkerPool::SetNumRasterThreads() { > the logic below should not be here. the idea is to not have any command line > parsing in cc/. this function should take a "num_thread" value instead and be > called from content/renderer/render_thread_impl.cc where command line parsing > can be done. > For render_thread_impl.cc, init() cannot recognize switches::kNumRasterThreads, so i had moved it to RWP. Will RenderThreadImpl know about the switch ?
On 2014/01/06 07:34:17, sohanjg wrote: > On 2014/01/06 07:13:11, David Reveman wrote: > > > > https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... > > cc/resources/raster_worker_pool.cc:472: void > > RasterWorkerPool::SetNumRasterThreads() { > > the logic below should not be here. the idea is to not have any command line > > parsing in cc/. this function should take a "num_thread" value instead and be > > called from content/renderer/render_thread_impl.cc where command line parsing > > can be done. > > > > For render_thread_impl.cc, init() cannot recognize switches::kNumRasterThreads, > so i had moved it to RWP. > Will RenderThreadImpl know about the switch ? My mistake, i didnt take into account, switches and cc::switches, cc::switch inclusion should take care of it. I will make the changes and update.
PTAL. Thanks https://codereview.chromium.org/73923003/diff/1390001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/base/switches.cc#new... cc/base/switches.cc:8: #include "base/logging.h" On 2014/01/06 07:13:12, David Reveman wrote: > no need to add this include Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/picture_pi... cc/resources/picture_pile.cc:12: #include "cc/base/switches.h" On 2014/01/06 07:13:12, David Reveman wrote: > no need to add this include Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:471: int RasterWorkerPool::num_raster_threads_ = kDefaultNumRasterThreads; On 2014/01/06 07:13:12, David Reveman wrote: > move this to anonymous namespace above. Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:472: void RasterWorkerPool::SetNumRasterThreads() { On 2014/01/06 07:13:12, David Reveman wrote: > the logic below should not be here. the idea is to not have any command line > parsing in cc/. this function should take a "num_thread" value instead and be > called from content/renderer/render_thread_impl.cc where command line parsing > can be done. Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:286: static int num_raster_threads_; On 2014/01/06 07:13:12, David Reveman wrote: > no need for this to be in the header. put it in an anonymous namespace in the > .cc file instead. Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... cc/resources/worker_pool.cc:20: #include "cc/resources/raster_worker_pool.h" On 2014/01/06 07:13:12, David Reveman wrote: > please avoid to include raster_worker_pool.h here. I have a follow up patch that > will fix this circular dependency put please do something like add a "static int > WorkerPool::GetNumWorkerThreads()" method that is implemented in > raster_worker_pool.cc by just calling RasterWorkerPool::GetNumRasterThreads() > for now instead of including this header here. Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... cc/resources/worker_pool.cc:419: ready_to_run_namespaces_.end(), On 2014/01/06 07:13:12, David Reveman wrote: > nit: indent should vertically align with "ready_to_run_namesp.." Done. https://codereview.chromium.org/73923003/diff/1390001/cc/resources/worker_poo... cc/resources/worker_pool.cc:420: task_namespace) == ready_to_run_namespaces_.end()); On 2014/01/06 07:13:12, David Reveman wrote: > nit: indent should vertically align with "ready_to_run_namespaces_.begin()". if > you need to break the line. break it just after "==" and > "ready_to_run_namespaces_.end()" should in that case vertically align with > "std::find". Done.
https://codereview.chromium.org/73923003/diff/1520001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/picture_pi... cc/resources/picture_pile.cc:235: size_t num_raster_threads = RasterWorkerPool::GetNumRasterThreads(); GetNumRasterThreads() returns an "int" not size_t. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:322: const int kDefaultNumRasterThreads = 1; nit: I'd add a blank line after this https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:323: static int num_raster_threads = kDefaultNumRasterThreads; s/num_raster_threads/g_num_raster_threads/ if you initialize this to 0 then you can DCHECK that it's not set twice. nit: 'static' is not really necessary here. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:467: DCHECK(num_threads); I prefer DCHECK_LT(0, num_threads) also add DCHECK_EQ(0, g_num_raster_threads) https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:473: int RasterWorkerPool::GetNumRasterThreads() { Let's just use WorkerPool::GetNumRasterThreads() for now until I clean this up in a follow up. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:478: int WorkerPool::GetNumRasterThreads() { if you make this: { if (!g_num_raster_threads) g_num_raster_threads = kDefaultNumRasterThreads; return g_num_raster_threads; } DCHECK will also fail if someone decides to call SetNumRasterThreads after GetNumRasterThreads has been called. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:197: static int GetNumRasterThreads(); Let's just declare this in WorkerPool for now. I'll move it here in my follow up. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.h:112: static int GetNumRasterThreads(); Use the comment from RasterWorkerPool here instead: // Gets the number of threads to use for running raster tasks. and add this TODO: // TODO(reveman): Move this to RasterWorkerPool: crbug.com/331844 https://codereview.chromium.org/73923003/diff/1520001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1520001/content/renderer/render... content/renderer/render_thread_impl.cc:145: we don't need this blank line https://codereview.chromium.org/73923003/diff/1520001/content/renderer/render... content/renderer/render_thread_impl.cc:427: cc::switches::kNumRasterThreads << ": " << string_value; you're always printing this warning. even when the value is valid.
https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:77: DCHECK(!a->ready_to_run_tasks.empty()); typo: b->... https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:78: return CompareTaskPriority(a->ready_to_run_tasks.back(), According to make_heap docs, the max element in a heap is in a->ready_to_run_tasks.front(). Only after pop_heap is it moved to .back(). On a related note, should we rename ready_to_run_tasks to something indicating that it is a heap? Ie ready_to_run_tasks_heap or something? That would make it easier to keep track of what's being maintained by make_heap/pop_heap/etc https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:178: linked_ptr<TaskNamespace> task_set = make_linked_ptr(new TaskNamespace()); Sorry if this is a silly question, but why linked_ptr here? Why not shared_ptr? I don't really know what linked_ptr does but from reading the source it seems to be pretty much the same? https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:422: std::push_heap(ready_to_run_namespaces_.begin(), I think this has the same problem as before with the priority queue, no? Ie, an existing namespace that has its priority changed by adding more ready to run tasks now invalidates the heap. I think there's just an "else" that's required here that says "heap_is_now_dirty = true" and, after the loop, does "if (heap_is_now_dirty) std::make_heap(...)". With number of namespaces being fairly small, I think that's OK. Wdyt?
https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:78: return CompareTaskPriority(a->ready_to_run_tasks.back(), On 2014/01/06 20:46:31, vmpstr wrote: > According to make_heap docs, the max element in a heap is in > a->ready_to_run_tasks.front(). Only after pop_heap is it moved to .back(). > > On a related note, should we rename ready_to_run_tasks to something indicating > that it is a heap? Ie ready_to_run_tasks_heap or something? That would make it > easier to keep track of what's being maintained by make_heap/pop_heap/etc I'm usually not a fan of putting the type in the variable name but I could maybe be convinced in this case. How about we just add a comment here instead? https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:178: linked_ptr<TaskNamespace> task_set = make_linked_ptr(new TaskNamespace()); On 2014/01/06 20:46:31, vmpstr wrote: > Sorry if this is a silly question, but why linked_ptr here? Why not shared_ptr? > I don't really know what linked_ptr does but from reading the source it seems to > be pretty much the same? linked_ptr is preferred in this case as we'll never have more than 2 references to an object. this could be ScopedPtrHashMap as we don't actually need reference counting.. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:398: if (node) { how about this here: bool ready_to_run_namespaces_has_heap_properties = true; https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:422: std::push_heap(ready_to_run_namespaces_.begin(), On 2014/01/06 20:46:31, vmpstr wrote: > I think this has the same problem as before with the priority queue, no? Ie, an > existing namespace that has its priority changed by adding more ready to run > tasks now invalidates the heap. > > I think there's just an "else" that's required here that says "heap_is_now_dirty > = true" and, after the loop, does "if (heap_is_now_dirty) std::make_heap(...)". > With number of namespaces being fairly small, I think that's OK. > > Wdyt? Oh, I completely missed that this was never fixed. I think it's a premature optimization to avoid make_heap and use push_heap when possible. it's also awkward to do push_heap when ready_to_run_namespaces_ doesn't have heap properties so more logic than an "else" clause is needed to do this properly. I think we should just remove std::push_heap from here and always set ready_to_run_namespaces_has_heap_properties to false after changing a task namespace. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:427: } and this here: if (!ready_to_run_namespaces_has_heap_properties) std::make_heap(...)
https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:78: return CompareTaskPriority(a->ready_to_run_tasks.back(), On 2014/01/06 21:59:39, David Reveman wrote: > On 2014/01/06 20:46:31, vmpstr wrote: > > According to make_heap docs, the max element in a heap is in > > a->ready_to_run_tasks.front(). Only after pop_heap is it moved to .back(). > > > > On a related note, should we rename ready_to_run_tasks to something indicating > > that it is a heap? Ie ready_to_run_tasks_heap or something? That would make it > > easier to keep track of what's being maintained by make_heap/pop_heap/etc > > I'm usually not a fan of putting the type in the variable name but I could maybe > be convinced in this case. How about we just add a comment here instead? I think a comment here is fine. (and possibly anywhere else we access the heap but not call std::*heap functions; I think this is the only spot for now) https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:422: std::push_heap(ready_to_run_namespaces_.begin(), On 2014/01/06 21:59:39, David Reveman wrote: > On 2014/01/06 20:46:31, vmpstr wrote: > > I think this has the same problem as before with the priority queue, no? Ie, > an > > existing namespace that has its priority changed by adding more ready to run > > tasks now invalidates the heap. > > > > I think there's just an "else" that's required here that says > "heap_is_now_dirty > > = true" and, after the loop, does "if (heap_is_now_dirty) > std::make_heap(...)". > > With number of namespaces being fairly small, I think that's OK. > > > > Wdyt? > > Oh, I completely missed that this was never fixed. > > I think it's a premature optimization to avoid make_heap and use push_heap when > possible. it's also awkward to do push_heap when ready_to_run_namespaces_ > doesn't have heap properties so more logic than an "else" clause is needed to do > this properly. > > I think we should just remove std::push_heap from here and always set > ready_to_run_namespaces_has_heap_properties to false after changing a task > namespace. Sounds good to me.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/picture_pi... File cc/resources/picture_pile.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/picture_pi... cc/resources/picture_pile.cc:235: size_t num_raster_threads = RasterWorkerPool::GetNumRasterThreads(); On 2014/01/06 20:01:35, David Reveman wrote: > GetNumRasterThreads() returns an "int" not size_t. Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:322: const int kDefaultNumRasterThreads = 1; On 2014/01/06 20:01:35, David Reveman wrote: > nit: I'd add a blank line after this Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:323: static int num_raster_threads = kDefaultNumRasterThreads; On 2014/01/06 20:01:35, David Reveman wrote: > s/num_raster_threads/g_num_raster_threads/ > > if you initialize this to 0 then you can DCHECK that it's not set twice. > > nit: 'static' is not really necessary here. Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:467: DCHECK(num_threads); On 2014/01/06 20:01:35, David Reveman wrote: > I prefer DCHECK_LT(0, num_threads) > > also add DCHECK_EQ(0, g_num_raster_threads) Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:473: int RasterWorkerPool::GetNumRasterThreads() { On 2014/01/06 20:01:35, David Reveman wrote: > Let's just use WorkerPool::GetNumRasterThreads() for now until I clean this up > in a follow up. Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/raster_wor... cc/resources/raster_worker_pool.cc:478: int WorkerPool::GetNumRasterThreads() { On 2014/01/06 20:01:35, David Reveman wrote: > if you make this: > > { > if (!g_num_raster_threads) > g_num_raster_threads = kDefaultNumRasterThreads; > > return g_num_raster_threads; > } > > DCHECK will also fail if someone decides to call SetNumRasterThreads after > GetNumRasterThreads has been called. Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:78: return CompareTaskPriority(a->ready_to_run_tasks.back(), On 2014/01/06 22:07:20, vmpstr wrote: > On 2014/01/06 21:59:39, David Reveman wrote: > > On 2014/01/06 20:46:31, vmpstr wrote: > > > According to make_heap docs, the max element in a heap is in > > > a->ready_to_run_tasks.front(). Only after pop_heap is it moved to .back(). > > > > > > On a related note, should we rename ready_to_run_tasks to something > indicating > > > that it is a heap? Ie ready_to_run_tasks_heap or something? That would make > it > > > easier to keep track of what's being maintained by make_heap/pop_heap/etc > > > > I'm usually not a fan of putting the type in the variable name but I could > maybe > > be convinced in this case. How about we just add a comment here instead? > > I think a comment here is fine. (and possibly anywhere else we access the heap > but not call std::*heap functions; I think this is the only spot for now) Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:178: linked_ptr<TaskNamespace> task_set = make_linked_ptr(new TaskNamespace()); On 2014/01/06 21:59:39, David Reveman wrote: > On 2014/01/06 20:46:31, vmpstr wrote: > > Sorry if this is a silly question, but why linked_ptr here? Why not > shared_ptr? > > I don't really know what linked_ptr does but from reading the source it seems > to > > be pretty much the same? > > linked_ptr is preferred in this case as we'll never have more than 2 references > to an object. this could be ScopedPtrHashMap as we don't actually need reference > counting.. We used linked_ptr instead of ScopedPtrHashMap because it cannot be passed by value in std::map. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:398: if (node) { On 2014/01/06 21:59:39, David Reveman wrote: > how about this here: > bool ready_to_run_namespaces_has_heap_properties = true; Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:422: std::push_heap(ready_to_run_namespaces_.begin(), On 2014/01/06 22:07:20, vmpstr wrote: > On 2014/01/06 21:59:39, David Reveman wrote: > > On 2014/01/06 20:46:31, vmpstr wrote: > > > I think this has the same problem as before with the priority queue, no? Ie, > > an > > > existing namespace that has its priority changed by adding more ready to run > > > tasks now invalidates the heap. > > > > > > I think there's just an "else" that's required here that says > > "heap_is_now_dirty > > > = true" and, after the loop, does "if (heap_is_now_dirty) > > std::make_heap(...)". > > > With number of namespaces being fairly small, I think that's OK. > > > > > > Wdyt? > > > > Oh, I completely missed that this was never fixed. > > > > I think it's a premature optimization to avoid make_heap and use push_heap > when > > possible. it's also awkward to do push_heap when ready_to_run_namespaces_ > > doesn't have heap properties so more logic than an "else" clause is needed to > do > > this properly. > > > > I think we should just remove std::push_heap from here and always set > > ready_to_run_namespaces_has_heap_properties to false after changing a task > > namespace. > > Sounds good to me. Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.cc:427: } On 2014/01/06 21:59:39, David Reveman wrote: > and this here: > if (!ready_to_run_namespaces_has_heap_properties) > std::make_heap(...) Done. https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1520001/cc/resources/worker_poo... cc/resources/worker_pool.h:112: static int GetNumRasterThreads(); On 2014/01/06 20:01:35, David Reveman wrote: > Use the comment from RasterWorkerPool here instead: > // Gets the number of threads to use for running raster tasks. > > and add this TODO: > // TODO(reveman): Move this to RasterWorkerPool: crbug.com/331844 Done. https://codereview.chromium.org/73923003/diff/1520001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1520001/content/renderer/render... content/renderer/render_thread_impl.cc:145: On 2014/01/06 20:01:35, David Reveman wrote: > we don't need this blank line Done. https://codereview.chromium.org/73923003/diff/1520001/content/renderer/render... content/renderer/render_thread_impl.cc:427: cc::switches::kNumRasterThreads << ": " << string_value; On 2014/01/06 20:01:35, David Reveman wrote: > you're always printing this warning. even when the value is valid. Done.
https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... File cc/resources/picture_pile_base.cc (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... cc/resources/picture_pile_base.cc:47: clear_canvas_with_debug_color_(kDefaultClearCanvasSetting) { nit: should only be one space between ")" and "{" https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... cc/resources/picture_pile_base.cc:79: clear_canvas_with_debug_color_(other->clear_canvas_with_debug_color_) { nit: should only be one space between ")" and "{" https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... cc/resources/picture_pile_impl.cc:10: #include "cc/base/switches.h" no need to include this here https://codereview.chromium.org/73923003/diff/1630001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:197: static int GetNumRasterThreads(); GetNumRasterThreads is a WorkerPool function. Should not be defined here until we move it to RasterWorkerPool interface. Please remove this from here. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:81: b->ready_to_run_tasks.back()); you should be using .front() here as Vlad pointed out. And the comment should make it clear why that is. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:401: bool ready_to_run_namespaces_has_heap_properties = true; nit: I'd prefer add a blank line after this https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:425: ready_to_run_namespaces_has_heap_properties = false; this needs to be set to false even when "was_empty" is false as you're still changing the task namespace https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:429: if (!ready_to_run_namespaces_has_heap_properties) nit: blank line before this please. add this comment above the line: // Rearrange the task namespaces in |ready_to_run_namespaces_| // in such a way that they yet again form a heap. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:432: CompareTaskNamespacePriority); nit: please use "{" "}" as this is a multiple line https://codereview.chromium.org/73923003/diff/1630001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1630001/content/renderer/render... content/renderer/render_thread_impl.cc:426: cc::switches::kNumRasterThreads << ": " << string_value; nit: multiple line "else". please use "{" "}" for both "if" and "else" clause
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... File cc/resources/picture_pile_base.cc (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... cc/resources/picture_pile_base.cc:47: clear_canvas_with_debug_color_(kDefaultClearCanvasSetting) { On 2014/01/07 17:24:06, David Reveman wrote: > nit: should only be one space between ")" and "{" Done. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... cc/resources/picture_pile_base.cc:79: clear_canvas_with_debug_color_(other->clear_canvas_with_debug_color_) { On 2014/01/07 17:24:06, David Reveman wrote: > nit: should only be one space between ")" and "{" Done. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/picture_pi... cc/resources/picture_pile_impl.cc:10: #include "cc/base/switches.h" On 2014/01/07 17:24:06, David Reveman wrote: > no need to include this here Done. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/raster_wor... File cc/resources/raster_worker_pool.h (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/raster_wor... cc/resources/raster_worker_pool.h:197: static int GetNumRasterThreads(); On 2014/01/07 17:24:06, David Reveman wrote: > GetNumRasterThreads is a WorkerPool function. Should not be defined here until > we move it to RasterWorkerPool interface. Please remove this from here. Done. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:81: b->ready_to_run_tasks.back()); On 2014/01/07 17:24:06, David Reveman wrote: > you should be using .front() here as Vlad pointed out. And the comment should > make it clear why that is. Done. I missed that. https://codereview.chromium.org/73923003/diff/1630001/cc/resources/worker_poo... cc/resources/worker_pool.cc:401: bool ready_to_run_namespaces_has_heap_properties = true; On 2014/01/07 17:24:06, David Reveman wrote: > nit: I'd prefer add a blank line after this Done. https://codereview.chromium.org/73923003/diff/1630001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1630001/content/renderer/render... content/renderer/render_thread_impl.cc:426: cc::switches::kNumRasterThreads << ": " << string_value; On 2014/01/07 17:24:06, David Reveman wrote: > nit: multiple line "else". please use "{" "}" for both "if" and "else" clause Done.
This looks good after fixing these last few nits. However, I'd like to see you also add some new worker pool unit tests and I think you also need to update the unit test code for cc_unittests and cc_perftests to build. https://codereview.chromium.org/73923003/diff/1700001/cc/resources/picture_pi... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/73923003/diff/1700001/cc/resources/picture_pi... cc/resources/picture_pile_impl.cc:12: #include "cc/resources/raster_worker_pool.h" nit: cc/resources/worker_pool.h is enough here https://codereview.chromium.org/73923003/diff/1700001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1700001/cc/resources/worker_poo... cc/resources/worker_pool.cc:19: #include "cc/base/switches.h" nit: no need to add this include. https://codereview.chromium.org/73923003/diff/1700001/cc/resources/worker_poo... cc/resources/worker_pool.cc:121: // Ordered set of tasks namespaces that have ready to run tasks. nit: s/tasks namespaces/task namespaces/ https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... content/renderer/render_thread_impl.cc:425: else { nit: move "else {" to previous line
I agree that this might need a few new extra unittests, but other than that it looks good. https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... content/renderer/render_thread_impl.cc:165: const int kDefaultNumRasterThreads = 1; This appears here and in cc/resources/raster_worker_pool.cc. I would prefer if we only had one of these constants: either have it here and always set the value, or have it in RWP and only set the value if it was parsed properly.
https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... content/renderer/render_thread_impl.cc:165: const int kDefaultNumRasterThreads = 1; On 2014/01/08 17:47:25, vmpstr wrote: > This appears here and in cc/resources/raster_worker_pool.cc. I would prefer if > we only had one of these constants: either have it here and always set the > value, or have it in RWP and only set the value if it was parsed properly. I don't think cc should depend on this being always set externally. Only setting it when parsed properly sgtm.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1700001/cc/resources/picture_pi... File cc/resources/picture_pile_impl.cc (right): https://codereview.chromium.org/73923003/diff/1700001/cc/resources/picture_pi... cc/resources/picture_pile_impl.cc:12: #include "cc/resources/raster_worker_pool.h" On 2014/01/08 07:06:40, David Reveman wrote: > nit: cc/resources/worker_pool.h is enough here Done. https://codereview.chromium.org/73923003/diff/1700001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/1700001/cc/resources/worker_poo... cc/resources/worker_pool.cc:19: #include "cc/base/switches.h" On 2014/01/08 07:06:40, David Reveman wrote: > nit: no need to add this include. Done. https://codereview.chromium.org/73923003/diff/1700001/cc/resources/worker_poo... cc/resources/worker_pool.cc:121: // Ordered set of tasks namespaces that have ready to run tasks. On 2014/01/08 07:06:40, David Reveman wrote: > nit: s/tasks namespaces/task namespaces/ Done. https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... content/renderer/render_thread_impl.cc:165: const int kDefaultNumRasterThreads = 1; On 2014/01/08 18:07:56, David Reveman wrote: > On 2014/01/08 17:47:25, vmpstr wrote: > > This appears here and in cc/resources/raster_worker_pool.cc. I would prefer if > > we only had one of these constants: either have it here and always set the > > value, or have it in RWP and only set the value if it was parsed properly. > > I don't think cc should depend on this being always set externally. Only setting > it when parsed properly sgtm. Done. https://codereview.chromium.org/73923003/diff/1700001/content/renderer/render... content/renderer/render_thread_impl.cc:425: else { On 2014/01/08 07:06:40, David Reveman wrote: > nit: move "else {" to previous line Done.
https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (left): https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:111: no need to remove this blank line https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:113: no need to remove this blank line https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:84: no need to add these 2 blank lines https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:614: } I'd like to see the tests run tasks from multiple worker pool instances at the same time instead of waiting for all tasks to finish before using another worker pool instance as that's not a realistic use case. Using the existing test logic for multiple worker pool instances is a good idea but let's instead adjust the existing test code so it can run with 1-N worker pool instances instead of duplicating all this logic. Always using 3 worker pool instances is fine for now but let's make sure it's easy to parameterize this later. https://codereview.chromium.org/73923003/diff/1790001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1790001/content/renderer/render... content/renderer/render_thread_impl.cc:165: const int kDefaultNumRasterThreads = 1; kDefaultNumRasterThreads is now unused. Please remove.
I'm happy with the way this turned out, no more comments from me :)
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (left): https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:111: On 2014/01/09 16:04:26, David Reveman wrote: > no need to remove this blank line Done. https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:113: On 2014/01/09 16:04:26, David Reveman wrote: > no need to remove this blank line Done. https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1790001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:84: On 2014/01/09 16:04:26, David Reveman wrote: > no need to add these 2 blank lines Done. https://codereview.chromium.org/73923003/diff/1790001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/1790001/content/renderer/render... content/renderer/render_thread_impl.cc:165: const int kDefaultNumRasterThreads = 1; On 2014/01/09 16:04:26, David Reveman wrote: > kDefaultNumRasterThreads is now unused. Please remove. Done.
https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:191: } not sure why you need this but it looks like you're better of using std::find instead if necessary. https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:194: scoped_ptr<FakeWorkerPool> worker_pool_; why not make this a ScopedVector and add a "const int kWorkerPoolCount = 3" constant that is used to determine the size? instead of having to manually create worker pools in each test.. https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:196: std::vector<unsigned> on_task_completed_ids_; and maybe turn these into arrays of vectors. existing functions would need a new index parameter. ie. void RunTask(int worker_pool_index, unsigned id) { run_task_ids_[worker_pool_index].push_back(id); } const std::vector<unsigned>& run_task_ids(int worker_pool_index) { return run_task_ids_[worker_pool_index]; } https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:492: } The way you're modifying these tests make it really hard see if the logic is changing or not. Please try to keep the code and logic as intact as possible. That makes the changes much easier for me to review. I think adding a vector of WorkerPool instances to WorkerPoolTest as I suggested above is a good way to accomplish this.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:191: } On 2014/01/10 21:22:03, David Reveman wrote: > not sure why you need this but it looks like you're better of using std::find > instead if necessary. Done. https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:194: scoped_ptr<FakeWorkerPool> worker_pool_; On 2014/01/10 21:22:03, David Reveman wrote: > why not make this a ScopedVector and add a "const int kWorkerPoolCount = 3" > constant that is used to determine the size? instead of having to manually > create worker pools in each test.. Done. https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:196: std::vector<unsigned> on_task_completed_ids_; On 2014/01/10 21:22:03, David Reveman wrote: > and maybe turn these into arrays of vectors. existing functions would need a new > index parameter. ie. > > void RunTask(int worker_pool_index, unsigned id) { > run_task_ids_[worker_pool_index].push_back(id); > } > const std::vector<unsigned>& run_task_ids(int worker_pool_index) { > return run_task_ids_[worker_pool_index]; > } Done. https://codereview.chromium.org/73923003/diff/1850001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:492: } On 2014/01/10 21:22:03, David Reveman wrote: > The way you're modifying these tests make it really hard see if the logic is > changing or not. Please try to keep the code and logic as intact as possible. > That makes the changes much easier for me to review. > > I think adding a vector of WorkerPool instances to WorkerPoolTest as I suggested > above is a good way to accomplish this. Done.
https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:16: const int kWorkerPoolCount = 3; nit: blank line before and no indent for this line. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:149: for (int i = 0; i < kWorkerPoolCount; i++) nit: ++i instead for consistency https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:154: worker_pools_.begin(); i != worker_pools_.end(); ++i) { There are some style problems here but maybe better to just do "for (int i = 0; i < kWorkerPoolCount; ++i)" here instead? https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:161: for (int i = 0; i < kWorkerPoolCount; i++) { nit: ++i instead https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:198: TEST_F(WorkerPoolTest, Basic) { Please avoid changing the logic and use a number of "for (int i = 0; i < kWorkerPoolCount; ++i)" loops here instead so that all that is needed to test with more worker pool instances is to change kWorkerPoolCount. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:267: TEST_F(WorkerPoolTest, Dependencies) { Same thing with this test. Use "for" loops and don't change the logic of the test. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:320: TEST_F(WorkerPoolTest, Priority) { And same thing with this test. Use "for" loops and don't change the logic of the test.
PTAL. Thanks. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:16: const int kWorkerPoolCount = 3; On 2014/01/13 18:37:21, David Reveman wrote: > nit: blank line before and no indent for this line. Done. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:154: worker_pools_.begin(); i != worker_pools_.end(); ++i) { On 2014/01/13 18:37:21, David Reveman wrote: > There are some style problems here but maybe better to just do "for (int i = 0; > i < kWorkerPoolCount; ++i)" here instead? Done. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:161: for (int i = 0; i < kWorkerPoolCount; i++) { On 2014/01/13 18:37:21, David Reveman wrote: > nit: ++i instead Done. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:198: TEST_F(WorkerPoolTest, Basic) { On 2014/01/13 18:37:21, David Reveman wrote: > Please avoid changing the logic and use a number of "for (int i = 0; i < > kWorkerPoolCount; ++i)" loops here instead so that all that is needed to test > with more worker pool instances is to change kWorkerPoolCount. Done. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:267: TEST_F(WorkerPoolTest, Dependencies) { On 2014/01/13 18:37:21, David Reveman wrote: > Same thing with this test. Use "for" loops and don't change the logic of the > test. Done. https://codereview.chromium.org/73923003/diff/1900001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:320: TEST_F(WorkerPoolTest, Priority) { On 2014/01/13 18:37:21, David Reveman wrote: > And same thing with this test. Use "for" loops and don't change the logic of the > test. Done.
Great! This looks good. Just fix this small set of nits and I think this is ready to land. https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:151: worker_pools_.push_back(FakeWorkerPool::Create().release()); nit: push_back(Create().release()) is awkward. Could we make worker_pools_ a "std::vector<linked_ptr<FakeWorkerPool> >" instead and have ::Create() return a linked_ptr? https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:160: void ResetIAllWorkerPoolds() { Typo in function name: s/ResetIAllWorkerPoolds/ResetAllWorkerPoolIds/ Maybe this should also take an index parameter like the functions below for consistency? You can keep it like this if you prefer though. https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:217: } nit: think blank line after this would look better https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:243: } nit: and blank line here https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:268: } nit: and blank line here https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:295: } nit: and blank line here
PTAL. Thanks https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:151: worker_pools_.push_back(FakeWorkerPool::Create().release()); On 2014/01/14 07:35:44, David Reveman wrote: > nit: push_back(Create().release()) is awkward. Could we make worker_pools_ a > "std::vector<linked_ptr<FakeWorkerPool> >" instead and have ::Create() return a > linked_ptr? Done. https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:160: void ResetIAllWorkerPoolds() { On 2014/01/14 07:35:44, David Reveman wrote: > Typo in function name: s/ResetIAllWorkerPoolds/ResetAllWorkerPoolIds/ > > Maybe this should also take an index parameter like the functions below for > consistency? You can keep it like this if you prefer though. Done. https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:217: } On 2014/01/14 07:35:44, David Reveman wrote: > nit: think blank line after this would look better Done. https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:243: } On 2014/01/14 07:35:44, David Reveman wrote: > nit: and blank line here Done. https://codereview.chromium.org/73923003/diff/1940001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:295: } On 2014/01/14 07:35:44, David Reveman wrote: > nit: and blank line here Done.
I noticed that your patch is changing the mode of all files from 644 to 755. If you download the raw diff you'll see a bunch of: old mode 100644 new mode 100755 That is making all these files executable and we don't want that so you'll need to remove all those before this can land. This is also the reason why cc/base/switches.* files are still in your patch even though no lines are changed. You probably need to "chmod 644 file_changed.cc" all files in this patch locally and commit and upload a new patch. https://codereview.chromium.org/73923003/diff/1980001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1980001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:191: std::vector<linked_ptr<FakeWorkerPool> > worker_pools_; Sorry that I forgot to ask this before but did you try making this "scoped_ptr<FakeWorkerPool> worker_pool_[kWorkerPoolCount]"? That would probably be cleanest.
PTAL Thanks. https://codereview.chromium.org/73923003/diff/1980001/cc/resources/worker_poo... File cc/resources/worker_pool_unittest.cc (right): https://codereview.chromium.org/73923003/diff/1980001/cc/resources/worker_poo... cc/resources/worker_pool_unittest.cc:191: std::vector<linked_ptr<FakeWorkerPool> > worker_pools_; On 2014/01/14 08:44:24, David Reveman wrote: > Sorry that I forgot to ask this before but did you try making this > "scoped_ptr<FakeWorkerPool> worker_pool_[kWorkerPoolCount]"? That would probably > be cleanest. Done. I tried earlier at each test level, but not here.
thanks for all the hard work, lgtm +jamesr for content/renderer/
lgtm https://codereview.chromium.org/73923003/diff/2040001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/73923003/diff/2040001/content/renderer/render... content/renderer/render_thread_impl.cc:415: if (command_line.HasSwitch(cc::switches::kNumRasterThreads)) { if this flag is only going to be used in content/ (as it appears to be the case) then it should be defined in content's switches list, not cc::switches. The flag will have to be different for browser and renderer compositors anyway. otherwise patch lgtm. It'd be fine to move this switch to the right place in a follow-up.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/73923003/2040001
Failed to apply patch for cc/resources/image_raster_worker_pool.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file cc/resources/image_raster_worker_pool.cc
Hunk #1 FAILED at 58.
1 out of 1 hunk FAILED -- saving rejects to file
cc/resources/image_raster_worker_pool.cc.rej
Patch: cc/resources/image_raster_worker_pool.cc
Index: cc/resources/image_raster_worker_pool.cc
diff --git a/cc/resources/image_raster_worker_pool.cc
b/cc/resources/image_raster_worker_pool.cc
index
90572ab417dedc02c00d3d082ce2ca94643e3449..de722b55ed7d673b6810e69633d9f7614b592b77
100644
--- a/cc/resources/image_raster_worker_pool.cc
+++ b/cc/resources/image_raster_worker_pool.cc
@@ -58,9 +58,8 @@ class ImageWorkerPoolTaskImpl : public
internal::WorkerPoolTask {
ImageRasterWorkerPool::ImageRasterWorkerPool(
ResourceProvider* resource_provider,
- size_t num_threads,
GLenum texture_target)
- : RasterWorkerPool(resource_provider, num_threads),
+ : RasterWorkerPool(resource_provider),
texture_target_(texture_target),
raster_tasks_pending_(false),
raster_tasks_required_for_activation_pending_(false) {
you can cq+ this after fixing these 2 issues https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_poo... cc/resources/worker_pool.cc:573: void WorkerPool::SetTaskGraph(TaskGraph* graph) { not sure why you move this to the bottom. please keep functions in the same order as the header. https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_poo... cc/resources/worker_pool.h:109: virtual void CheckForCompletedWorkerTasks(); This function is not virtual anymore and it's already declared below as protected. Remove this line.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/73923003/2160001
Done. CQ-ing. https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_poo... File cc/resources/worker_pool.cc (right): https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_poo... cc/resources/worker_pool.cc:573: void WorkerPool::SetTaskGraph(TaskGraph* graph) { On 2014/01/16 08:00:44, David Reveman wrote: > not sure why you move this to the bottom. please keep functions in the same > order as the header. Done. https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_pool.h File cc/resources/worker_pool.h (right): https://codereview.chromium.org/73923003/diff/2110001/cc/resources/worker_poo... cc/resources/worker_pool.h:109: virtual void CheckForCompletedWorkerTasks(); On 2014/01/16 08:00:44, David Reveman wrote: > This function is not virtual anymore and it's already declared below as > protected. Remove this line. Done.
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/73923003/2400001
Retried try job too often on mac_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/73923003/2400001
Retried try job too often on mac_rel for step(s) app_list_unittests, base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chromedriver_unittests, components_unittests, content_browsertests, content_unittests, crypto_unittests, google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, jingle_unittests, media_unittests, message_center_unittests, nacl_integration, net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
On 2014/01/16 14:41:25, I haz the power (commit-bot) wrote: > Retried try job too often on mac_rel for step(s) app_list_unittests, > base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, > check_deps, check_deps2git, chromedriver_unittests, components_unittests, > content_browsertests, content_unittests, crypto_unittests, > google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, > jingle_unittests, media_unittests, message_center_unittests, nacl_integration, > net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, > sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, > unit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... For *mac* build, there seem to some conflict while resolving GL_XX macros, because render_thread_impl include npapi.h and also raster_worker_pool.h, which includes GLES2/gl2.h. I have removed GLES2/gl2.h from raster_worker_pool.h, because resource/resource.h now also includes it. Will it be correct ?
On 2014/01/16 16:14:38, sohanjg wrote: > On 2014/01/16 14:41:25, I haz the power (commit-bot) wrote: > > Retried try job too often on mac_rel for step(s) app_list_unittests, > > base_unittests, browser_tests, cacheinvalidation_unittests, cc_unittests, > > check_deps, check_deps2git, chromedriver_unittests, components_unittests, > > content_browsertests, content_unittests, crypto_unittests, > > google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests, > > jingle_unittests, media_unittests, message_center_unittests, nacl_integration, > > net_unittests, ppapi_unittests, printing_unittests, remoting_unittests, > > sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_unittests, > > unit_tests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu... > > For *mac* build, there seem to some conflict while resolving GL_XX macros, > because render_thread_impl include npapi.h and also raster_worker_pool.h, which > includes GLES2/gl2.h. > I have removed GLES2/gl2.h from raster_worker_pool.h, because > resource/resource.h now also includes it. > Will it be correct ? I think that will be fixed by https://codereview.chromium.org/140923003/.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/73923003/2730002
On 2014/01/17 05:53:47, I haz the power (commit-bot) wrote: > CQ is trying da patch. Follow status at > https://chromium-status.appspot.com/cq/sohan.jyoti%40samsung.com/73923003/273... gl2.h is included not only from raster_worker_pool.h but also from resource.h as well as resource_provider.h, which are included in raster_worker_pool.h, i think it will be cumbersome to change GLenum (there are other GLXX macros as well) for everything. lets keep the setter/getter temporarily in WP, and then move as follow up patch, crbug.com/331844.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sohan.jyoti@samsung.com/73923003/2130005
Message was sent while issue was closed.
Change committed as 245526
Message was sent while issue was closed.
Description was changed from ========== Shared Raster Worker Threads This CL will share worker pool threads across LTHI for a single process. Idea is to reduce the number of threads used per process. For this, we implement WorkerPool::Inner as singleton, and LTHI/WorkerPool would share the same instance per process. Reviewer: reveman@chromium.org BUG=239423 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245526 ========== to ========== Shared Raster Worker Threads This CL will share worker pool threads across LTHI for a single process. Idea is to reduce the number of threads used per process. For this, we implement WorkerPool::Inner as singleton, and LTHI/WorkerPool would share the same instance per process. Reviewer: reveman@chromium.org BUG=239423 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245526 ========== |
