Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
489293002:
cc: Don't infinitely throttle large raster tasks (Closed)
|
Created:
6 years, 4 months ago by enne (OOO) Modified:
6 years, 4 months ago Reviewers:
reveman CC:
chromium-reviews, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Descriptioncc: Don't infinitely throttle large raster tasks
Previously, if a raster task had a resource size that was larger than
the throttling limit for the raster worker, that task would never get
scheduled. This leads to starvation for that task, preventing tree
activation, and causing freezes.
The fix is to always allow such large tasks if it is the first task.
R=reveman@chromium.org
BUG=403446
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291484
Patch Set 1 #
Total comments: 14
Patch Set 2 : reveman comments #
Total comments: 2
Patch Set 3 : Remove unused member OOPS #Patch Set 4 : Rebase #
Messages
Total messages: 19 (0 generated)
https://codereview.chromium.org/489293002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/489293002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:519: // but if it's the first task allow it to complete no matter what its size, nit: maybe "only" instead of "first" as this is effected by previously scheduled tasks and not only the tasks being scheduled at this time https://codereview.chromium.org/489293002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:524: bytes_pending_upload > 0) { nit: s/bytes_pending_upload > 0/bytes_pending_upload/ https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... File cc/resources/raster_worker_pool_unittest.cc (right): https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:207: void AppendTask(unsigned id, gfx::Size size) { nit: const gfx::Size& size https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:274: size_t throttle_bytes_; nit: max_transfer_buffer_usage_bytes_ as from this pov it's not explicit that PBRWP is throttling tasks based on this. Also, does this need to be a member? can it just be a constant? kMaxTransferBufferUsageBytes.. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:338: TEST_P(PixelBufferRasterWorkerPoolTest, BigTaskThrottling) { How about we call this LargeResources and run it for all RWP implementations? I don't think we have to leak the detail that some implementations do throttling to this code. Just need to make sure resources of this size will run without problem independent of the RWP implementation.. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:342: // Verify a resource of this size will get throttled. are we missing a "not" here? Maybe just "Verify a resource of this size is handled properly" https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:346: EXPECT_GE(resource->bytes(), throttle_bytes_); Could you instead compute |size| based on kMaxTransferBufferUsageBytes? maybe change kMaxTransferBufferUsageBytes to kMaxTransferBufferResourceDimension if that's cleaner..
https://codereview.chromium.org/489293002/diff/1/cc/resources/pixel_buffer_ra... File cc/resources/pixel_buffer_raster_worker_pool.cc (right): https://codereview.chromium.org/489293002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:519: // but if it's the first task allow it to complete no matter what its size, On 2014/08/21 01:15:47, reveman wrote: > nit: maybe "only" instead of "first" as this is effected by previously scheduled > tasks and not only the tasks being scheduled at this time Done. https://codereview.chromium.org/489293002/diff/1/cc/resources/pixel_buffer_ra... cc/resources/pixel_buffer_raster_worker_pool.cc:524: bytes_pending_upload > 0) { On 2014/08/21 01:15:47, reveman wrote: > nit: s/bytes_pending_upload > 0/bytes_pending_upload/ Done. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... File cc/resources/raster_worker_pool_unittest.cc (right): https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:207: void AppendTask(unsigned id, gfx::Size size) { On 2014/08/21 01:15:47, reveman wrote: > nit: const gfx::Size& size Done. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:274: size_t throttle_bytes_; On 2014/08/21 01:15:47, reveman wrote: > nit: max_transfer_buffer_usage_bytes_ as from this pov it's not explicit that > PBRWP is throttling tasks based on this. > > Also, does this need to be a member? can it just be a constant? > kMaxTransferBufferUsageBytes.. Done. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:338: TEST_P(PixelBufferRasterWorkerPoolTest, BigTaskThrottling) { On 2014/08/21 01:15:47, reveman wrote: > How about we call this LargeResources and run it for all RWP implementations? I > don't think we have to leak the detail that some implementations do throttling > to this code. Just need to make sure resources of this size will run without > problem independent of the RWP implementation.. Done. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:342: // Verify a resource of this size will get throttled. On 2014/08/21 01:15:47, reveman wrote: > are we missing a "not" here? Maybe just "Verify a resource of this size is > handled properly" No, but maybe it should be "could get throttled". It's trying to test the test. Rewrote this to be more clear. https://codereview.chromium.org/489293002/diff/1/cc/resources/raster_worker_p... cc/resources/raster_worker_pool_unittest.cc:346: EXPECT_GE(resource->bytes(), throttle_bytes_); On 2014/08/21 01:15:47, reveman wrote: > Could you instead compute |size| based on kMaxTransferBufferUsageBytes? maybe > change kMaxTransferBufferUsageBytes to kMaxTransferBufferResourceDimension if > that's cleaner.. That's why there's the block of code right here, to make sure that the size for this test is larger than the throttle size. I made kLargeResourceDimension a constant along with kMaxTransferBufferUsageBytes.
lgtm after removing unused member variable https://codereview.chromium.org/489293002/diff/20001/cc/resources/raster_work... File cc/resources/raster_worker_pool_unittest.cc (right): https://codereview.chromium.org/489293002/diff/20001/cc/resources/raster_work... cc/resources/raster_worker_pool_unittest.cc:278: size_t throttle_bytes_; unused, right?
https://codereview.chromium.org/489293002/diff/20001/cc/resources/raster_work... File cc/resources/raster_worker_pool_unittest.cc (right): https://codereview.chromium.org/489293002/diff/20001/cc/resources/raster_work... cc/resources/raster_worker_pool_unittest.cc:278: size_t throttle_bytes_; On 2014/08/21 02:07:00, reveman wrote: > unused, right? Done.
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/489293002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/489293002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/enne@chromium.org/489293002/60001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
Message was sent while issue was closed.
Committed patchset #4 (60001) as 291484 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
