Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1576133002:
Add category handling to RasterWorkerPool (Closed)
|
Created:
4 years, 11 months ago by ericrk Modified:
4 years, 11 months ago CC:
blink-worker-reviews_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, kinuko+watch, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@hihi Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd category handling to RasterWorkerPool
This change causes RasterWorkerPool to handle task
categories. To do this RasterWorkerPool assigns
specific task categories to each of its threads.
As I've been having trouble showing a clear win/loss
for using a single thread for background tasks, this
is behind a flag
"--use-single-thread-for-background-raster-tasks",
allowing us to test this more.
BUG=564260
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect
Committed: https://crrev.com/7795c700d046679b159ca6c7f0b2e479355c9fb2
Cr-Commit-Position: refs/heads/master@{#370812}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : rebase #Patch Set 5 : improve comments #Patch Set 6 : comments #Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #
Total comments: 7
Patch Set 13 : feedback #
Total comments: 2
Patch Set 14 : nit #
Messages
Total messages: 26 (14 generated)
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: GPU Thread - a single thread which handles all GPU tasks. High Priority Threads - N threads which handle high priority software tasks. Low Priority Thread - A single thread which handles low priority software tasks. The low priority thread only runs if the GPU and high priority threads are idle. BUG= ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: GPU Thread - a single thread which handles all GPU tasks. High Priority Threads - N threads which handle high priority software tasks. Low Priority Thread - A single thread which handles low priority software tasks. The low priority thread only runs if the GPU and high priority threads are idle. BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: GPU Thread - a single thread which handles all GPU tasks. High Priority Threads - N threads which handle high priority software tasks. Low Priority Thread - A single thread which handles low priority software tasks. The low priority thread only runs if the GPU and high priority threads are idle. BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: NONCONCURRENT_FOREGROUND thread - a single thread which handles any tasks which shouldn't be run in parallel - currently just GPU raster tasks. FOREGROUND threads - N threads which handle high priority tasks (anything needed for draw or activation). BACKGROUND thread - A single thread which handles low priority tasks (anything that doesn't meet the high priority qualifications). The BACKGROUND thread only runs if the FOREGROUND threads are idle. BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ==========
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: NONCONCURRENT_FOREGROUND thread - a single thread which handles any tasks which shouldn't be run in parallel - currently just GPU raster tasks. FOREGROUND threads - N threads which handle high priority tasks (anything needed for draw or activation). BACKGROUND thread - A single thread which handles low priority tasks (anything that doesn't meet the high priority qualifications). The BACKGROUND thread only runs if the FOREGROUND threads are idle. BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: NONCONCURRENT_FOREGROUND thread - a single thread which handles any tasks which shouldn't be run in parallel - currently just GPU raster tasks. FOREGROUND threads - N threads which handle high priority tasks (anything needed for draw or activation). BACKGROUND thread - A single thread which handles low priority tasks (anything that doesn't meet the high priority qualifications). The BACKGROUND thread only runs if the FOREGROUND threads are idle. BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ==========
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: NONCONCURRENT_FOREGROUND thread - a single thread which handles any tasks which shouldn't be run in parallel - currently just GPU raster tasks. FOREGROUND threads - N threads which handle high priority tasks (anything needed for draw or activation). BACKGROUND thread - A single thread which handles low priority tasks (anything that doesn't meet the high priority qualifications). The BACKGROUND thread only runs if the FOREGROUND threads are idle. BUG= CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: NONCONCURRENT_FOREGROUND thread - a single thread which handles any tasks which shouldn't be run in parallel - currently just GPU raster tasks. FOREGROUND threads - N threads which handle high priority tasks (anything needed for draw or activation). BACKGROUND thread - A single thread which handles low priority tasks (anything that doesn't meet the high priority qualifications). The BACKGROUND thread only runs if the FOREGROUND threads are idle. As I've been having trouble showing a clear win/loss for the BACKGROUND thread in RasterWorkerPool, this is added behind a flag "--enable-background-raster-thread", allowing us to test this via a finch experiment. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ==========
ericrk@chromium.org changed reviewers: + reveman@chromium.org, vmpstr@chromium.org
Patchset #12 (id:210001) has been deleted
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool creates three sets of threads: NONCONCURRENT_FOREGROUND thread - a single thread which handles any tasks which shouldn't be run in parallel - currently just GPU raster tasks. FOREGROUND threads - N threads which handle high priority tasks (anything needed for draw or activation). BACKGROUND thread - A single thread which handles low priority tasks (anything that doesn't meet the high priority qualifications). The BACKGROUND thread only runs if the FOREGROUND threads are idle. As I've been having trouble showing a clear win/loss for the BACKGROUND thread in RasterWorkerPool, this is added behind a flag "--enable-background-raster-thread", allowing us to test this via a finch experiment. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool assigns specific task categories to each of its threads. As I've been having trouble showing a clear win/loss for using a single thread for background tasks, this is behind a flag "--use-single-thread-for-background-raster-tasks", allowing us to test this more. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ==========
https://codereview.chromium.org/1576133002/diff/230001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1576133002/diff/230001/content/public/common/... content/public/common/content_switches.h:235: CONTENT_EXPORT extern const char kUseSingleThreadForBackgroundRasterTasks[]; nit: Remove "Raster" as this technically not limited to raster tasks. However, I don't think we need a flag for this. Even if we haven't been able to demonstrate a huge benefit yet I think it makes sense to always use this. I'm expecting the benefit to come from increasing the number of raster threads without the regression in responsiveness that we've seen in the past. https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... File content/renderer/raster_worker_pool.cc (right): https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... content/renderer/raster_worker_pool.cc:143: if (threads_.size() == 0 || hm, should we avoid using the same thread as NONCONCURRENT_FOREGROUND? "threads_.size() == (num_threads - 1)" instead? https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... content/renderer/raster_worker_pool.cc:229: } nit: move this code into a new utility function that return false when no task was ready to run. Maybe reuse the RunTaskWithLockAcquired name and rename the RunTaskWithLockAcquired(cc::TaskCategory category) function below to RunTaskInCategoryWithLockAcquired instead.
https://codereview.chromium.org/1576133002/diff/230001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1576133002/diff/230001/content/public/common/... content/public/common/content_switches.h:235: CONTENT_EXPORT extern const char kUseSingleThreadForBackgroundRasterTasks[]; On 2016/01/13 23:09:30, reveman wrote: > nit: Remove "Raster" as this technically not limited to raster tasks. > > However, I don't think we need a flag for this. Even if we haven't been able to > demonstrate a huge benefit yet I think it makes sense to always use this. I'm > expecting the benefit to come from increasing the number of raster threads > without the regression in responsiveness that we've seen in the past. Fair enough - we can see what happens when we land this without the flag. https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... File content/renderer/raster_worker_pool.cc (right): https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... content/renderer/raster_worker_pool.cc:143: if (threads_.size() == 0 || On 2016/01/13 23:09:30, reveman wrote: > hm, should we avoid using the same thread as NONCONCURRENT_FOREGROUND? > "threads_.size() == (num_threads - 1)" instead? with this setup, low priority tasks will never run until all nonconcurrent and regular foreground tasks are started - if we moved it to the last thread, they might run when there are still pending nonconcurrent foreground tasks... although this might be fine, as nonconcurrent foreground tasks are, well, nonconcurrent, so I don't expect as much contention... I think your suggestion may make more sense (doesn't leave threads idle in more scenarios), so changing this for now... WDYT? https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... content/renderer/raster_worker_pool.cc:229: } On 2016/01/13 23:09:30, reveman wrote: > nit: move this code into a new utility function that return false when no task > was ready to run. Maybe reuse the RunTaskWithLockAcquired name and rename the > RunTaskWithLockAcquired(cc::TaskCategory category) function below to > RunTaskInCategoryWithLockAcquired instead. Good suggestion.
lgtm with nit Fyi, I'd like to give https://codereview.chromium.org/1616953003 a try after this lands. https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... File content/renderer/raster_worker_pool.cc (right): https://codereview.chromium.org/1576133002/diff/230001/content/renderer/raste... content/renderer/raster_worker_pool.cc:143: if (threads_.size() == 0 || On 2016/01/21 at 01:19:44, ericrk wrote: > On 2016/01/13 23:09:30, reveman wrote: > > hm, should we avoid using the same thread as NONCONCURRENT_FOREGROUND? > > "threads_.size() == (num_threads - 1)" instead? > > with this setup, low priority tasks will never run until all nonconcurrent and regular foreground tasks are started - if we moved it to the last thread, they might run when there are still pending nonconcurrent foreground tasks... although this might be fine, as nonconcurrent foreground tasks are, well, nonconcurrent, so I don't expect as much contention... > > I think your suggestion may make more sense (doesn't leave threads idle in more scenarios), so changing this for now... WDYT? Sgtm https://codereview.chromium.org/1576133002/diff/250001/content/renderer/raste... File content/renderer/raster_worker_pool.cc (right): https://codereview.chromium.org/1576133002/diff/250001/content/renderer/raste... content/renderer/raster_worker_pool.cc:141: // them. nit: Please update this comment. Simply "The last thread can run background tasks." now that the flag is gone?
The CQ bit was checked by ericrk@chromium.org
https://codereview.chromium.org/1576133002/diff/250001/content/renderer/raste... File content/renderer/raster_worker_pool.cc (right): https://codereview.chromium.org/1576133002/diff/250001/content/renderer/raste... content/renderer/raster_worker_pool.cc:141: // them. On 2016/01/21 at 17:12:24, reveman wrote: > nit: Please update this comment. Simply "The last thread can run background tasks." now that the flag is gone? yup
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1576133002/#ps270001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576133002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576133002/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ericrk@chromium.org changed reviewers: + sievers@chromium.org
+sievers for owners. Thanks!
lgtm
The CQ bit was checked by ericrk@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1576133002/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1576133002/270001
Message was sent while issue was closed.
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool assigns specific task categories to each of its threads. As I've been having trouble showing a clear win/loss for using a single thread for background tasks, this is behind a flag "--use-single-thread-for-background-raster-tasks", allowing us to test this more. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool assigns specific task categories to each of its threads. As I've been having trouble showing a clear win/loss for using a single thread for background tasks, this is behind a flag "--use-single-thread-for-background-raster-tasks", allowing us to test this more. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ==========
Message was sent while issue was closed.
Committed patchset #14 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool assigns specific task categories to each of its threads. As I've been having trouble showing a clear win/loss for using a single thread for background tasks, this is behind a flag "--use-single-thread-for-background-raster-tasks", allowing us to test this more. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect ========== to ========== Add category handling to RasterWorkerPool This change causes RasterWorkerPool to handle task categories. To do this RasterWorkerPool assigns specific task categories to each of its threads. As I've been having trouble showing a clear win/loss for using a single thread for background tasks, this is behind a flag "--use-single-thread-for-background-raster-tasks", allowing us to test this more. BUG=564260 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect Committed: https://crrev.com/7795c700d046679b159ca6c7f0b2e479355c9fb2 Cr-Commit-Position: refs/heads/master@{#370812} ==========
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/7795c700d046679b159ca6c7f0b2e479355c9fb2 Cr-Commit-Position: refs/heads/master@{#370812} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
