Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
966813003:
[content]: Ensure TaskQueueManager AFTER_WAKEUP autopump policy wakes only on newer tasks. (Closed)
|
Created:
5 years, 9 months ago by rmcilroy Modified:
5 years, 9 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, scheduler-bugs_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[content]: Ensure TaskQueueManager AFTER_WAKEUP autopump policy wakes only on newer tasks.
The AFTER_WAKEUP autopump policy should only pump a task queue if the task
which was executed is newer (i.e., posted after) the oldest task in the
task queue's incoming queue. Otherwise the queue will be woken by the task
which actually posts the task.
This will be required for implemention of long idle task - design doc:
https://docs.google.com/a/chromium.org/document/d/1yBlUdYW8VTIfB-DqhvQqUeP0kf-Ap1W4cao2yQq58Do/edit
BUG=455713
Committed: https://crrev.com/0ca7a117f26b8e5c1a2fb55647d08e8997dd866d
Cr-Commit-Position: refs/heads/master@{#318752}
Patch Set 1 #Patch Set 2 : Minor whitespace fix #Patch Set 3 : Add check for null tasks #
Total comments: 15
Patch Set 4 : Address Sami's comments #
Total comments: 12
Patch Set 5 : Review comments #
Total comments: 4
Patch Set 6 : Review Comments #
Messages
Total messages: 16 (3 generated)
rmcilroy@chromium.org changed reviewers: + skyostil@chromium.org
PTAL, thanks!
https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:163: return true; I realized that the previous patch set might incorrectly schedule the queue to be pumped when the sequence number wrapped to negative, and we were passing a NULL task (i.e., the NULL task passed to UpdateWorkQueues before running the first task), so I've added this check. This might cause the queue not to be pumped for the 1 in 2^32 times the sequence number actually is validly '0', but I think this is OK - if you disagree I can rework it to be more explicit about when the task is actually a null task.
Looks great! https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:163: return true; On 2015/03/02 11:44:13, rmcilroy wrote: > I realized that the previous patch set might incorrectly schedule the queue to > be pumped when the sequence number wrapped to negative, and we were passing a > NULL task (i.e., the NULL task passed to UpdateWorkQueues before running the > first task), so I've added this check. This might cause the queue not to be > pumped for the 1 in 2^32 times the sequence number actually is validly '0', but > I think this is OK - if you disagree I can rework it to be more explicit about > when the task is actually a null task. Note that the sequence number starts at zero so the first task will always be considered a null task. Not pumping in this case would be fine if AFTER_WAKEUP work is always considered to be optional work, but I don't think we should make that assumption at the TQM level. I guess if you just made |task| a possibly null pointer then this would work correctly in all cases. WDYT? https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:170: return oldest_queued_task < task; We should really go and swap that operator around :) I was trying to decide if it makes sense to look at the delayed_run_time here or not. If we ever wanted to support delayed post-wakeup tasks, they delay should really be computed from the time the wakeup happens and not the posting time. It would be probably simpler to disallow them completely. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:485: EXPECT_EQ(1, run_count); Not sure why you made this more strict? Seems like a good idea anyway. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:672: EXPECT_EQ(2u, selector_->work_queues().size()); nit: These EXPECT_EQs here are a bit redundant. We could move them into the base class. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:752: // Check that if multiple task reposts themselves onto a auto pump after s/a/an/
picksi@google.com changed reviewers: + picksi@google.com
I have some non-functional comments on comments. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:159: if (task == NULL) Is this to protect against null-pointer deferencing, or is there an actual non-broken use case here? If so can the comment explain it a bit more (at the moment the comment adds nothing that I couldn't deduce from the code!) https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:450: if (!UpdateWorkQueues(&next_pending_delayed_task, NULL)) Can you add a comment/const to make it clear what this NULL is. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:742: // Check that if multiple task reposts themselves onto an auto pump after nits: 'tasks repost' 'pump-after-wakeup' 'eventually will stop'
https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:159: if (task == NULL) On 2015/03/02 14:30:06, picksi wrote: > Is this to protect against null-pointer deferencing, or is there an actual > non-broken use case here? If so can the comment explain it a bit more (at the > moment the comment adds nothing that I couldn't deduce from the code!) Also, I think we're using nullptr now, although "(!task)" probably fits best here. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:450: if (!UpdateWorkQueues(&next_pending_delayed_task, NULL)) On 2015/03/02 14:30:06, picksi wrote: > Can you add a comment/const to make it clear what this NULL is. nullptr here too. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:61: EXPECT_EQ(num_queues, selector_->work_queues().size()); Thanks!
https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:163: return true; On 2015/03/02 12:28:56, Sami wrote: > On 2015/03/02 11:44:13, rmcilroy wrote: > > I realized that the previous patch set might incorrectly schedule the queue to > > be pumped when the sequence number wrapped to negative, and we were passing a > > NULL task (i.e., the NULL task passed to UpdateWorkQueues before running the > > first task), so I've added this check. This might cause the queue not to be > > pumped for the 1 in 2^32 times the sequence number actually is validly '0', > but > > I think this is OK - if you disagree I can rework it to be more explicit about > > when the task is actually a null task. > > Note that the sequence number starts at zero so the first task will always be > considered a null task. Not pumping in this case would be fine if AFTER_WAKEUP > work is always considered to be optional work, but I don't think we should make > that assumption at the TQM level. > > I guess if you just made |task| a possibly null pointer then this would work > correctly in all cases. WDYT? Yeah that would probably be better - done. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:170: return oldest_queued_task < task; On 2015/03/02 12:28:56, Sami wrote: > We should really go and swap that operator around :) This would break the use of priority queues - we should probably just add an IsOlder method to pending_tasks though, WDYT? > I was trying to decide if > it makes sense to look at the delayed_run_time here or not. If we ever wanted to > support delayed post-wakeup tasks, they delay should really be computed from the > time the wakeup happens and not the posting time. It would be probably simpler > to disallow them completely. The '<' operator in pending_tasks already looks at the delayed_run_time. Are you saying we should avoid using it and should rely on just the sequence number? https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:485: EXPECT_EQ(1, run_count); On 2015/03/02 12:28:56, Sami wrote: > Not sure why you made this more strict? Seems like a good idea anyway. I added the run_count arg to RePostingTestTask for the tests below and then thought it would be worth checking here too. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:672: EXPECT_EQ(2u, selector_->work_queues().size()); On 2015/03/02 12:28:56, Sami wrote: > nit: These EXPECT_EQs here are a bit redundant. We could move them into the base > class. Done. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:752: // Check that if multiple task reposts themselves onto a auto pump after On 2015/03/02 12:28:56, Sami wrote: > s/a/an/ Done. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:159: if (task == NULL) On 2015/03/02 14:30:06, picksi wrote: > Is this to protect against null-pointer deferencing, or is there an actual > non-broken use case here? If so can the comment explain it a bit more (at the > moment the comment adds nothing that I couldn't deduce from the code!) Very true (the comment added more before I explicitly made the task NULL ;)). Done. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:159: if (task == NULL) On 2015/03/02 14:48:46, Sami wrote: > On 2015/03/02 14:30:06, picksi wrote: > > Is this to protect against null-pointer deferencing, or is there an actual > > non-broken use case here? If so can the comment explain it a bit more (at the > > moment the comment adds nothing that I couldn't deduce from the code!) > > Also, I think we're using nullptr now, although "(!task)" probably fits best > here. Done. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:450: if (!UpdateWorkQueues(&next_pending_delayed_task, NULL)) On 2015/03/02 14:48:46, Sami wrote: > On 2015/03/02 14:30:06, picksi wrote: > > Can you add a comment/const to make it clear what this NULL is. > > nullptr here too. Done. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:450: if (!UpdateWorkQueues(&next_pending_delayed_task, NULL)) On 2015/03/02 14:30:06, picksi wrote: > Can you add a comment/const to make it clear what this NULL is. Done. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:61: EXPECT_EQ(num_queues, selector_->work_queues().size()); On 2015/03/02 14:48:47, Sami wrote: > Thanks! Acknowledged. https://codereview.chromium.org/966813003/diff/60001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:742: // Check that if multiple task reposts themselves onto an auto pump after On 2015/03/02 14:30:06, picksi wrote: > nits: > 'tasks repost' > 'pump-after-wakeup' > 'eventually will stop' Done.
Thanks, nice and clear now! I think you lost a 'tasks' when editing one of the comments though. https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:744: // execute. nit:missing word? 'no other *tasks* execute.'
https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:170: return oldest_queued_task < task; On 2015/03/02 15:34:51, rmcilroy wrote: > On 2015/03/02 12:28:56, Sami wrote: > > We should really go and swap that operator around :) > > This would break the use of priority queues - we should probably just add an > IsOlder method to pending_tasks though, WDYT? We could just define the priority queue type with the correct ordering like here: https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/s... > > I was trying to decide if > > it makes sense to look at the delayed_run_time here or not. If we ever wanted > to > > support delayed post-wakeup tasks, they delay should really be computed from > the > > time the wakeup happens and not the posting time. It would be probably simpler > > to disallow them completely. > > The '<' operator in pending_tasks already looks at the delayed_run_time. Are you > saying we should avoid using it and should rely on just the sequence number? > Right, '<' already looks at the time and I was just wondering out loud if it should do that in this case. Maybe we just add a DCHECK somewhere that prevents delayed work for after-wakeup queues. WDYT? https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:485: EXPECT_EQ(1, run_count); On 2015/03/02 15:34:51, rmcilroy wrote: > On 2015/03/02 12:28:56, Sami wrote: > > Not sure why you made this more strict? Seems like a good idea anyway. > > I added the run_count arg to RePostingTestTask for the tests below and then > thought it would be worth checking here too. Ah, makes sense, thanks. https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:449: base::PendingTask previous_task((tracked_objects::Location()), nit: no need to move this here now.
https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:170: return oldest_queued_task < task; On 2015/03/02 15:55:57, Sami wrote: > On 2015/03/02 15:34:51, rmcilroy wrote: > > On 2015/03/02 12:28:56, Sami wrote: > > > We should really go and swap that operator around :) > > > > This would break the use of priority queues - we should probably just add an > > IsOlder method to pending_tasks though, WDYT? > > We could just define the priority queue type with the correct ordering like > here: > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/s... Hmm, good point :). I'll do this as a followup change. > > > I was trying to decide if > > > it makes sense to look at the delayed_run_time here or not. If we ever > wanted > > to > > > support delayed post-wakeup tasks, they delay should really be computed from > > the > > > time the wakeup happens and not the posting time. It would be probably > simpler > > > to disallow them completely. > > > > The '<' operator in pending_tasks already looks at the delayed_run_time. Are > you > > saying we should avoid using it and should rely on just the sequence number? > > > > Right, '<' already looks at the time and I was just wondering out loud if it > should do that in this case. Maybe we just add a DCHECK somewhere that prevents > delayed work for after-wakeup queues. WDYT? I need to use delayed task for the after-wakeup queue for the long idle implementation (see https://codereview.chromium.org/968073003/), but by the time the task get's into the incoming queue it's delayed_run_time should already have been set to null, so I added a DCHECK that this is always the case. https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:485: EXPECT_EQ(1, run_count); On 2015/03/02 15:55:58, Sami wrote: > On 2015/03/02 15:34:51, rmcilroy wrote: > > On 2015/03/02 12:28:56, Sami wrote: > > > Not sure why you made this more strict? Seems like a good idea anyway. > > > > I added the run_count arg to RePostingTestTask for the tests below and then > > thought it would be worth checking here too. > > Ah, makes sense, thanks. Acknowledged. https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:449: base::PendingTask previous_task((tracked_objects::Location()), On 2015/03/02 15:55:58, Sami wrote: > nit: no need to move this here now. Done. https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/966813003/diff/80001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager_unittest.cc:744: // execute. On 2015/03/02 15:39:40, picksi wrote: > nit:missing word? 'no other *tasks* execute.' Done.
Thanks Ross, lgtm. Doing the operator< fix as a follow up sounds fine.
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/966813003/100001
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/0ca7a117f26b8e5c1a2fb55647d08e8997dd866d Cr-Commit-Position: refs/heads/master@{#318752} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
