Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2742383005:
[scheduler] Untangle BudgetPool from TaskQueueThrottler. (Closed)
Description[scheduler] Untangle BudgetPool from TaskQueueThrottler.
This is the refactoring needed for new wakeup-based throttling
mechanism.
Remove friendship between BudgetPools and TaskQueueThrottler.
R=skyostil@chromium.org,alexclarke@chromium.org
BUG=699541
Review-Url: https://codereview.chromium.org/2742383005
Cr-Commit-Position: refs/heads/master@{#457395}
Committed: https://chromium.googlesource.com/chromium/src/+/dbd112157be505b23f60116a3636870aee7ade32
Patch Set 1 #
Total comments: 6
Patch Set 2 : Addressed comments from skyostil@ #
Total comments: 2
Patch Set 3 : Fix nit #
Messages
Total messages: 26 (17 generated)
PTAL
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm Over all this looks fine. Just a design bikeshed for follow up patches. https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:116: void AddQueueToBudgetPool(TaskQueue* queue, BudgetPool* budget_pool); bike-shed: Should we try to decouple even further? E.g. put an interface between the BudgetPool and TaskQueueThrottler. The eventual goal would be to be able to reuse the BudgetPool elsewhere, and potentially the throttling with lucky luke?
https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:116: void AddQueueToBudgetPool(TaskQueue* queue, BudgetPool* budget_pool); On 2017/03/14 18:22:39, alex clarke wrote: > bike-shed: Should we try to decouple even further? E.g. put an interface > between the BudgetPool and TaskQueueThrottler. The eventual goal would be to be > able to reuse the BudgetPool elsewhere, and potentially the throttling with > lucky luke? I like the idea in general, but that I think that's premature at the moment. Lucky Luke integration will require some effort and direct copy-paste will be impossible, so let's wait and do the refactoring then the need arises.
https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:64: // Call TaskQueueThrottler::BlockThrottledQueue for all relevant queues. This is an abstract interface with no obvious ties to TaskQueueThrottler -- can we describe what this method does on a higher level without referring to TQT? https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:116: void AddQueueToBudgetPool(TaskQueue* queue, BudgetPool* budget_pool); On 2017/03/14 18:26:44, altimin wrote: > On 2017/03/14 18:22:39, alex clarke wrote: > > bike-shed: Should we try to decouple even further? E.g. put an interface > > between the BudgetPool and TaskQueueThrottler. The eventual goal would be to > be > > able to reuse the BudgetPool elsewhere, and potentially the throttling with > > lucky luke? > > I like the idea in general, but that I think that's premature at the moment. > Lucky Luke integration will require some effort and direct copy-paste will be > impossible, so let's wait and do the refactoring then the need arises. I think I'd also like to suggest turning these methods into an explicit abstract interface. It doesn't need to be perfect, but will help testing the budget pools in isolation and makes it clear what the dependencies and responsibilities between the two class hierarchies are (and would avoid a circular dependency too). Doing that later will be much harder as more dependencies creep in.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
PTAL https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h (right): https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/budget_pool.h:64: // Call TaskQueueThrottler::BlockThrottledQueue for all relevant queues. On 2017/03/14 18:43:00, Sami wrote: > This is an abstract interface with no obvious ties to TaskQueueThrottler -- can > we describe what this method does on a higher level without referring to TQT? Done. https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2742383005/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:116: void AddQueueToBudgetPool(TaskQueue* queue, BudgetPool* budget_pool); On 2017/03/14 18:43:01, Sami wrote: > On 2017/03/14 18:26:44, altimin wrote: > > On 2017/03/14 18:22:39, alex clarke wrote: > > > bike-shed: Should we try to decouple even further? E.g. put an interface > > > between the BudgetPool and TaskQueueThrottler. The eventual goal would be > to > > be > > > able to reuse the BudgetPool elsewhere, and potentially the throttling with > > > lucky luke? > > > > I like the idea in general, but that I think that's premature at the moment. > > Lucky Luke integration will require some effort and direct copy-paste will be > > impossible, so let's wait and do the refactoring then the need arises. > > I think I'd also like to suggest turning these methods into an explicit abstract > interface. It doesn't need to be perfect, but will help testing the budget pools > in isolation and makes it clear what the dependencies and responsibilities > between the two class hierarchies are (and would avoid a circular dependency > too). Doing that later will be much harder as more dependencies creep in. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, thanks! https://codereview.chromium.org/2742383005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2742383005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:84: BudgetPoolController { nit: public inheritance plz
Description was changed from ========== [scheduler] Untangle BudgetPool from TaskQueueThrottler. This is the refactoring needed for new wakeup-based throttling mechanism. Remove friendship between BudgetPools and TaskQueueThrottler. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 [scheduler] Separate TaskQueueThrottler and BudgetPool R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 ========== to ========== [scheduler] Untangle BudgetPool from TaskQueueThrottler. This is the refactoring needed for new wakeup-based throttling mechanism. Remove friendship between BudgetPools and TaskQueueThrottler. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 ==========
The CQ bit was checked by altimin@chromium.org to run a CQ dry run
https://codereview.chromium.org/2742383005/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h (right): https://codereview.chromium.org/2742383005/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/scheduler/renderer/task_queue_throttler.h:84: BudgetPoolController { On 2017/03/15 15:08:08, Sami wrote: > nit: public inheritance plz Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by altimin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexclarke@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/2742383005/#ps40001 (title: "Fix nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489664926552810,
"parent_rev": "e9c1c98d51c7b2bb74f58da335aa75bdb128cd89", "commit_rev":
"dbd112157be505b23f60116a3636870aee7ade32"}
Message was sent while issue was closed.
Description was changed from ========== [scheduler] Untangle BudgetPool from TaskQueueThrottler. This is the refactoring needed for new wakeup-based throttling mechanism. Remove friendship between BudgetPools and TaskQueueThrottler. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 ========== to ========== [scheduler] Untangle BudgetPool from TaskQueueThrottler. This is the refactoring needed for new wakeup-based throttling mechanism. Remove friendship between BudgetPools and TaskQueueThrottler. R=skyostil@chromium.org,alexclarke@chromium.org BUG=699541 Review-Url: https://codereview.chromium.org/2742383005 Cr-Commit-Position: refs/heads/master@{#457395} Committed: https://chromium.googlesource.com/chromium/src/+/dbd112157be505b23f60116a3636... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/dbd112157be505b23f60116a3636... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
