Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
174453003:
cc: Replace RasterTaskStateMap with a vector and brute force search instead of hash map lookups. (Closed)
|
Created:
6 years, 10 months ago by reveman Modified:
6 years, 9 months ago Reviewers:
vmpstr CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptioncc: Replace RasterTaskStateMap with a vector and brute force search instead of hash map lookups.
Also avoids unnecessary heap allocations by recycling old state vector.
Result is ~100% performance improvement to ScheduleTasks.
BUG=269841
TEST=cc_perftests --gtest_filter=RasterWorkerPoolPerfTests/RasterWorkerPoolPerfTest.ScheduleTasks/0
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255615
Patch Set 1 #Patch Set 2 : rebase #
Total comments: 12
Patch Set 3 : address review feedback #
Total comments: 4
Patch Set 4 : add comment #
Total comments: 2
Patch Set 5 : move comment #Patch Set 6 : rebase #Patch Set 7 : android build fix #
Messages
Total messages: 27 (0 generated)
https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:115: RasterTaskStateMap::iterator state_it = raster_task_states_.find(task); Out of curiousity... The current approach will have to eventually perform worse (ie, the more we schedule the more the overhead of a hash map will not matter as much as a linear search through the vector). Do you know what that number is? What I mean is how many items do I have to schedule at once before the vector becomes slower than the hash map? https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:138: std::swap(*state_it, raster_task_states_.back()); nit: s/\*state_it/state/ https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:179: raster_task_states_.swap(recycled_raster_task_states_); Is there a better name for this vector? Recycled makes it seem that you're going to reuse the states themselves, but it seems that you are only reusing the heap memory. Maybe something like pending_raster_task_state_ or even new_raster_task_states_ https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:715: continue; no need for continue https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.h:42: class TaskComparator { I know this is consistent with other classes in task graph runner / worker pool system, but is there a better name for this class? TaskComparator to me means something that compares two arbitrary tasks to determine which one is "better" or something.. This is more like a TaskFinder where it only returns true if the passed argument matches the constructor argument. I don't know, this is mostly a big nit but feel free to leave it as is, since I don't have any good suggestions.
https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:115: RasterTaskStateMap::iterator state_it = raster_task_states_.find(task); On 2014/02/21 18:39:41, vmpstr wrote: > Out of curiousity... The current approach will have to eventually perform worse > (ie, the more we schedule the more the overhead of a hash map will not matter as > much as a linear search through the vector). Do you know what that number is? > What I mean is how many items do I have to schedule at once before the vector > becomes slower than the hash map? We get about the same performance when reaching ~1000 raster tasks with latest patch. https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:138: std::swap(*state_it, raster_task_states_.back()); On 2014/02/21 18:39:41, vmpstr wrote: > nit: s/\*state_it/state/ Done. https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:179: raster_task_states_.swap(recycled_raster_task_states_); On 2014/02/21 18:39:41, vmpstr wrote: > Is there a better name for this vector? Recycled makes it seem that you're going > to reuse the states themselves, but it seems that you are only reusing the heap > memory. Maybe something like pending_raster_task_state_ or even > new_raster_task_states_ I figured out a faster way to do this that avoids the need for a second vector. This gave another ~25% performance improvement and allows me to remove the set_ functions from RasterTaskState. PTAL. https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:715: continue; On 2014/02/21 18:39:41, vmpstr wrote: > no need for continue Oh, this is a bug! This continue statement was supposed to continue the outermost loop and that obviously doesn't work. Fixed in https://codereview.chromium.org/170783007 that introduced this problem. Thanks! :) https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.h:42: class TaskComparator { On 2014/02/21 18:39:41, vmpstr wrote: > I know this is consistent with other classes in task graph runner / worker pool > system, but is there a better name for this class? TaskComparator to me means > something that compares two arbitrary tasks to determine which one is "better" > or something.. > > This is more like a TaskFinder where it only returns true if the passed argument > matches the constructor argument. I don't know, this is mostly a big nit but > feel free to leave it as is, since I don't have any good suggestions. Maybe IsTask or IsTaskComparator would be better as the result is code such as: find_if(states.begin(), states.end(), IsTask(task)) I would have to change this everywhere though so better as a follow up.
https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.cc (left): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.cc:115: RasterTaskStateMap::iterator state_it = raster_task_states_.find(task); On 2014/02/22 11:32:01, reveman wrote: > On 2014/02/21 18:39:41, vmpstr wrote: > > Out of curiousity... The current approach will have to eventually perform > worse > > (ie, the more we schedule the more the overhead of a hash map will not matter > as > > much as a linear search through the vector). Do you know what that number is? > > What I mean is how many items do I have to schedule at once before the vector > > becomes slower than the hash map? > > We get about the same performance when reaching ~1000 raster tasks with latest > patch. Nice. Let's hope we don't schedule more than that too often (or ever :P) https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... File cc/resources/pixel_buffer_raster_worker_pool.h (right): https://codereview.chromium.org/174453003/diff/40001/cc/resources/pixel_buffe... cc/resources/pixel_buffer_raster_worker_pool.h:42: class TaskComparator { On 2014/02/22 11:32:01, reveman wrote: > On 2014/02/21 18:39:41, vmpstr wrote: > > I know this is consistent with other classes in task graph runner / worker > pool > > system, but is there a better name for this class? TaskComparator to me means > > something that compares two arbitrary tasks to determine which one is "better" > > or something.. > > > > This is more like a TaskFinder where it only returns true if the passed > argument > > matches the constructor argument. I don't know, this is mostly a big nit but > > feel free to leave it as is, since I don't have any good suggestions. > > Maybe IsTask or IsTaskComparator would be better as the result is code such as: > > find_if(states.begin(), states.end(), IsTask(task)) > > I would have to change this everywhere though so better as a follow up. IsTask or IsTaskComparator or IsTaskClosure or something all sound good to me. As a follow up sounds good as well. https://codereview.chromium.org/174453003/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/174453003/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:162: if (state_it == raster_task_states_.end()) What does it mean for the previously scheduled raster task item not to have a raster task state? Does this mean it was completed? If so, could you add a comment? https://codereview.chromium.org/174453003/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:178: state.required_for_activation = false; Do you need to subtract raster_tasks_required_for_activation_count_ here? Also, why is this marked as not required for activation? I mean I understand that a cancelled task that is required for activation is kind of weird, but it can still happen, no?
https://codereview.chromium.org/174453003/diff/100001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/174453003/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:162: if (state_it == raster_task_states_.end()) On 2014/02/24 17:37:59, vmpstr wrote: > What does it mean for the previously scheduled raster task item not to have a > raster task state? Does this mean it was completed? If so, could you add a > comment? Yes, it means we've already processed completion of task. Added a comment. https://codereview.chromium.org/174453003/diff/100001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:178: state.required_for_activation = false; On 2014/02/24 17:37:59, vmpstr wrote: > Do you need to subtract raster_tasks_required_for_activation_count_ here? Also, > why is this marked as not required for activation? I mean I understand that a > cancelled task that is required for activation is kind of weird, but it can > still happen, no? Can't happen. The required-for-activate state is not sticky. Only those tasks that were marked as required-for-activation in the last call to ScheduleTasks affect the required for activation signal. I think this will be more clear after we generalize the concept of this signal like we talked about last week. We shouldn't adjust raster_tasks_required_for_activation_count_ here as it was initialized above to the new set of required-for-activation tasks.
lgtm https://codereview.chromium.org/174453003/diff/150001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/174453003/diff/150001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:163: continue; // Already processed completion of task. nit: Can you put the comment before the if instead?
https://codereview.chromium.org/174453003/diff/150001/cc/resources/pixel_buff... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/174453003/diff/150001/cc/resources/pixel_buff... cc/resources/pixel_buffer_raster_worker_pool.cc:163: continue; // Already processed completion of task. On 2014/02/24 19:22:28, vmpstr wrote: > nit: Can you put the comment before the if instead? Done.
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/174453003/230001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on win_rel for step(s) app_list_unittests, ash_unittests, aura_unittests, cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git, chrome_elf_unittests, chromedriver_unittests, components_unittests, compositor_unittests, content_browsertests, content_unittests, crypto_unittests, events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests, jingle_unittests, media_unittests, mini_installer_test, nacl_integration, ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests, sync_integration_tests, sync_unit_tests, telemetry_perf_unittests, telemetry_unittests, views_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/174453003/230001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/174453003/230001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_dbg for step(s) slave_steps http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/174453003/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg, mac_chromium_rel
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/174453003/250001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg
The CQ bit was checked by reveman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/reveman@chromium.org/174453003/250001
Message was sent while issue was closed.
Change committed as 255615 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
