Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
994833003:
Prevent multiple pending UpdatePolicy tasks. (Closed)
|
Created:
5 years, 9 months ago by alex clarke (OOO till 29th) 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. |
DescriptionPrevent multiple pending UpdatePolicy tasks.
Allow only one pending delayed UpdatePolicy task, and seperatly
one pending non-delayed UpdatePolicy task.
BUG=465686
Committed: https://crrev.com/c106eb5cac46149507aca52278d5685c3d9f221d
Cr-Commit-Position: refs/heads/master@{#320488}
Patch Set 1 #Patch Set 2 : Tweak #
Total comments: 8
Patch Set 3 : Responding to feedback #Patch Set 4 : Refactored #
Total comments: 22
Patch Set 5 : Pull out a DeadlineTaskRunner #Patch Set 6 : Some more tweaks #Patch Set 7 : tweak 2 #
Total comments: 8
Patch Set 8 : Changed DeadlineTaskRunner::SetDeadline to take a delay #Patch Set 9 : Slight simplification #Patch Set 10 : Change for Ross #
Total comments: 16
Patch Set 11 : Polish #Patch Set 12 : Missed a comment and fixed all the typos this time #
Total comments: 8
Patch Set 13 : Add content export and a few nits #Patch Set 14 : Forgot to change the friend declaration #Messages
Total messages: 42 (10 generated)
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
skyostil@chromium.org changed reviewers: + rmcilroy@chromium.org
+Ross since he's also stared at this code a lot.
https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:303: pending_non_delayed_update_policy_ = false; I don't think the pending_non_delayed_update_policy_ changes are really required. I don't think we have a big issue with multiple non-delayed UpdatePolicy tasks (they typically check other fields and only repost if they really want to update the policy). Maybe we could just make PostUpdatePolicyOnControlRunner not take a delay (only post now), and have the UpdatePolicy function always cancel any pending delayed updatepolicy tasks and repost a new delayed one. I think this would end up being a fair bit simpler to understand, WDYT?
https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:303: pending_non_delayed_update_policy_ = false; On 2015/03/10 16:08:39, rmcilroy wrote: > I don't think the pending_non_delayed_update_policy_ changes are really > required. I don't think we have a big issue with multiple non-delayed > UpdatePolicy tasks (they typically check other fields and only repost if they > really want to update the policy). Maybe we could just make > PostUpdatePolicyOnControlRunner not take a delay (only post now), and have the > UpdatePolicy function always cancel any pending delayed updatepolicy tasks and > repost a new delayed one. I think this would end up being a fair bit simpler to > understand, WDYT? Yeah, I'm all for simplicity too. I was thinking we could just keep track of when the next policy update is due to run and act accordingly here, but I think that's equivalent to what you are suggesting. https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:124: void PostUpdatePolicyOnControlRunnerLocked(base::TimeDelta delay); Should we call this MaybePost... to match the other functions that do similar things? https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:877: PostUpdatePolicyOnControlRunner(base::TimeDelta()); Do we know what triggered the bug in the first place? It would be good to have a unit test that replicates the those circumstances. https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:913: PostUpdatePolicyOnControlRunner(base::TimeDelta::FromMilliseconds(1)); Could you also try this the other way around to make sure the immediate policy update doesn't cancel the delayed one?
https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:303: pending_non_delayed_update_policy_ = false; On 2015/03/10 16:08:39, rmcilroy wrote: > I don't think the pending_non_delayed_update_policy_ changes are really > required. I don't think we have a big issue with multiple non-delayed > UpdatePolicy tasks Currently they only get sent when the input_stream_state_ changes (i.e. you're right they're not a problem at the moment). Certinly your suggestion would simplify things a bit, however we've had a number of bugs when DoWork and now UpdatePolicy where posted more frequently than anticipated. I would argue that in light of that defensive programming is a good idea.
On 2015/03/10 16:19:38, Sami wrote: > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > File content/renderer/scheduler/renderer_scheduler_impl.cc (right): > > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > content/renderer/scheduler/renderer_scheduler_impl.cc:303: > pending_non_delayed_update_policy_ = false; > On 2015/03/10 16:08:39, rmcilroy wrote: > > I don't think the pending_non_delayed_update_policy_ changes are really > > required. I don't think we have a big issue with multiple non-delayed > > UpdatePolicy tasks (they typically check other fields and only repost if they > > really want to update the policy). Maybe we could just make > > PostUpdatePolicyOnControlRunner not take a delay (only post now), and have the > > UpdatePolicy function always cancel any pending delayed updatepolicy tasks and > > repost a new delayed one. I think this would end up being a fair bit simpler > to > > understand, WDYT? > > Yeah, I'm all for simplicity too. I was thinking we could just keep track of > when the next policy update is due to run and act accordingly here, but I think > that's equivalent to what you are suggesting. Yes, this would be equivalent to what I'm suggesting. > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > File content/renderer/scheduler/renderer_scheduler_impl.h (right): > > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > content/renderer/scheduler/renderer_scheduler_impl.h:124: void > PostUpdatePolicyOnControlRunnerLocked(base::TimeDelta delay); > Should we call this MaybePost... to match the other functions that do similar > things? > > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): > > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:877: > PostUpdatePolicyOnControlRunner(base::TimeDelta()); > Do we know what triggered the bug in the first place? It would be good to have a > unit test that replicates the those circumstances. > > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:913: > PostUpdatePolicyOnControlRunner(base::TimeDelta::FromMilliseconds(1)); > Could you also try this the other way around to make sure the immediate policy > update doesn't cancel the delayed one?
On 2015/03/10 16:21:42, alexclarke1 wrote: > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > File content/renderer/scheduler/renderer_scheduler_impl.cc (right): > > https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... > content/renderer/scheduler/renderer_scheduler_impl.cc:303: > pending_non_delayed_update_policy_ = false; > On 2015/03/10 16:08:39, rmcilroy wrote: > > I don't think the pending_non_delayed_update_policy_ changes are really > > required. I don't think we have a big issue with multiple non-delayed > > UpdatePolicy tasks > > Currently they only get sent when the input_stream_state_ changes (i.e. you're > right they're not a problem at the moment). > > Certinly your suggestion would simplify things a bit, however we've had a number > of bugs when DoWork and now UpdatePolicy where posted more frequently than > anticipated. I would argue that in light of that defensive programming is a good > idea. I'd still prefer to see the split between posting of the delayed tasks (which only happens through a cancelable callback within the UpdatePolicy method), and posting of immediate callbacks. One other though - could we re-use the PolicyNeedsUpdate flag? If that flag is set then there should be a task already posted, so maybe we could just avoid posting in those situations.
On 2015/03/10 18:17:38, rmcilroy wrote: > I'd still prefer to see the split between posting of the delayed tasks (which > only happens through a cancelable callback within the UpdatePolicy method), and > posting of immediate callbacks. One other though - could we re-use the > PolicyNeedsUpdate flag? If that flag is set then there should be a task already > posted, so maybe we could just avoid posting in those situations. We still need to make sure delayed policy updates happen at the right time. I wonder if there's some way to refactor all of this out to some kind of helper that deals with the complexity?
On 2015/03/10 18:27:16, Sami wrote: > On 2015/03/10 18:17:38, rmcilroy wrote: > > I'd still prefer to see the split between posting of the delayed tasks (which > > only happens through a cancelable callback within the UpdatePolicy method), > and > > posting of immediate callbacks. One other though - could we re-use the > > PolicyNeedsUpdate flag? If that flag is set then there should be a task > already > > posted, so maybe we could just avoid posting in those situations. > > We still need to make sure delayed policy updates happen at the right time. I > wonder if there's some way to refactor all of this out to some kind of helper > that deals with the complexity? The delayed policy updates only ever come from the updatepolicy function, and I think it makes sense that we explicitly enforce this. Since the UpdatePolicy function recalculates the time for the next policy update whenever it is run, it should be fine to simply cancel any outstanding delayed policy updates and post the new one. All the other update policy post-tasks are just to get it on the right thread. I chatted with Alex in-person yesterday about my thoughts and I think we can create a PostUpdatePolicyFromDifferentThread type API which both does the post and updates the updatePolicyFlag (and using the flag to ensure only one of these is in-flight at a time), and a seperate cancelable PostDelayedTask from within UpdatePolicy to deal with delayed policy updates. Happy to chat more about this in person if you like.
I've implemented something along the lines Ross suggested. PTAL https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:877: PostUpdatePolicyOnControlRunner(base::TimeDelta()); On 2015/03/10 16:19:38, Sami wrote: > Do we know what triggered the bug in the first place? It would be good to have a > unit test that replicates the those circumstances. Anything that calls MaybeUpdatePolicy can potentially cause a delayed task to get posted if we are prioritizing input. These are the current vectors: * UpdateForInputEvent -- but only if the input_stream_state_ changes * IsHighPriorityWorkAnticipated * ShouldYieldForHighPriorityWork I've added a test which exercises these. https://codereview.chromium.org/994833003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:913: PostUpdatePolicyOnControlRunner(base::TimeDelta::FromMilliseconds(1)); On 2015/03/10 16:19:38, Sami wrote: > Could you also try this the other way around to make sure the immediate policy > update doesn't cancel the delayed one? It's now hard to test this directly. If you're keen still I could probably add a helper.
Hmm, looks like I had more comments than I thought :) I think the high level approach is okay but I'm just being paranoid. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:214: UpdatePolicyLocked(); Could you add a DCHECK that this function is only called on the main thread? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:276: incoming_signals_lock_.AssertAcquired(); Maybe add a DCHECK() that this is never called on the main thread, because there we should just do the update immediately. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:286: incoming_signals_lock_.AssertAcquired(); I think this is probably flexible enough for now, but to provide some insurance in the future, could we change pending_delayed_policy_update_ to a timestamp and DCHECK here that we're not trying to schedule a policy update sooner than that? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:136: void UpdatePolicy(); Maybe move this next to UpdatePolicyLocked()? The difference between the functions is pretty obvious from the naming so we don't really need to explain what this does IMO. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:140: void DelayedPolicyUpdateTask(); nit: Name this with a verb instead a noun to match the other functions here. The comment might also be a little unnecessary if this is called something like PerformDelayedPolicyUpdate() Should we also allow this function only on the main thread, since we really only expect it to be triggered from within the policy update (maybe we could assert for that too)? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:62: clock_->AdvanceNow(base::TimeDelta::FromMilliseconds(100)); Thanks for adding this. By the way, would SetAutoAdvanceNowToPendingTasks accomplish the same thing? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:868: void UpdatePolicyLocked() override { The way we have to reach into the internals to test this things makes me think we should just pull the policy update mechanism out of the scheduler proper. I don't have a better suggestion of how to test this without doing that. I guess a little less invasive option would be to give the scheduler an closure/interface which it would call whenever the policy updates. WDYT? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:903: EXPECT_EQ(2, mock_scheduler->update_policy_count_); Could you add a comment saying which two updates these are?
Looking more in the direction I was thinking of, a couple of comments. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:274: void RendererSchedulerImpl::ScheduleUrgentPolicyUpdate( nit on naming - how about PostUpdatePolicyToMainThread or similar to identify that this should only really be used when you are not guaranteed to be on the main thread? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:276: incoming_signals_lock_.AssertAcquired(); On 2015/03/11 16:58:25, Sami wrote: > Maybe add a DCHECK() that this is never called on the main thread, because there > we should just do the update immediately. It feels like this would make it easy to introduce bugs where you accidentally prevent posting a more recent policy update if there was already a pending update policy task with a long deadline - I don't think there is any code in UpdatePolicy which would prevent us from doing this. As with my comment above, I think it would be safer to use a CancellableCallback to ensure that we always do the update at the most up-to-date time. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:177: base::Closure delayed_update_policy_closure_; I really think this should be a CancelableClosureHolder - the we wouldn't need to worry about the pending_delayed_policy_update_ and wouldn't need to add the DCHECK Sami is suggesting (since we would always be posting a update for the soonest policy update). Is there a reason we can't do this?
https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:177: base::Closure delayed_update_policy_closure_; On 2015/03/11 17:15:07, rmcilroy wrote: > I really think this should be a CancelableClosureHolder - the we wouldn't need > to worry about the pending_delayed_policy_update_ and wouldn't need to add the > DCHECK Sami is suggesting (since we would always be posting a update for the > soonest policy update). Is there a reason we can't do this? I think a good reason for avoiding cancellable closures is that they give you a false sense of safety: the underlying task will still run even if it's cancelled, adding invisible overhead. I think this is critical enough for battery life that we should try an be explicit about it.
On 2015/03/11 17:18:48, Sami wrote: > https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... > File content/renderer/scheduler/renderer_scheduler_impl.h (right): > > https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... > content/renderer/scheduler/renderer_scheduler_impl.h:177: base::Closure > delayed_update_policy_closure_; > On 2015/03/11 17:15:07, rmcilroy wrote: > > I really think this should be a CancelableClosureHolder - the we wouldn't need > > to worry about the pending_delayed_policy_update_ and wouldn't need to add the > > DCHECK Sami is suggesting (since we would always be posting a update for the > > soonest policy update). Is there a reason we can't do this? > > I think a good reason for avoiding cancellable closures is that they give you a > false sense of safety: the underlying task will still run even if it's > cancelled, adding invisible overhead. I think this is critical enough for > battery life that we should try an be explicit about it. Yes I see your point (although I think the fact that we don't run the updatePolicy task will mean we don't post a new delayed task and the impact will be much less than it is currently) but as I mentioned, I don't think it's safe to avoid posting new policy updates if there is already one pending. Let's chat about this in-person - I think it should be possible to add more support to Cancellable closures to get the best of both worlds and allow us to do this cleanly
PTAL https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:214: UpdatePolicyLocked(); On 2015/03/11 16:58:25, Sami wrote: > Could you add a DCHECK that this function is only called on the main thread? Done. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:274: void RendererSchedulerImpl::ScheduleUrgentPolicyUpdate( On 2015/03/11 17:15:07, rmcilroy wrote: > nit on naming - how about PostUpdatePolicyToMainThread or similar to identify > that this should only really be used when you are not guaranteed to be on the > main thread? I like the OnMainThread, but it doesn't always post so I prefer ScheduleUrgentPolicyUpdateOnMainThread. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:276: incoming_signals_lock_.AssertAcquired(); On 2015/03/11 17:15:07, rmcilroy wrote: > On 2015/03/11 16:58:25, Sami wrote: > > Maybe add a DCHECK() that this is never called on the main thread, because > there > > we should just do the update immediately. > > It feels like this would make it easy to introduce bugs where you accidentally > prevent posting a more recent policy update if there was already a pending > update policy task with a long deadline - I don't think there is any code in > UpdatePolicy which would prevent us from doing this. As with my comment above, > I think it would be safer to use a CancellableCallback to ensure that we always > do the update at the most up-to-date time. The de-duping for ScheduleDelayedPolicyUpdate is separate from ScheduleUrgentPolicyUpdate. So we don't have a problem if there's a UpdatePolicy posted for 100ms time but the compositor thread wants to post an UpdatePolicy with no delay. Hopefully the DeadlineTaskRunner addresses your other concerns. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:276: incoming_signals_lock_.AssertAcquired(); On 2015/03/11 16:58:25, Sami wrote: > Maybe add a DCHECK() that this is never called on the main thread, because there > we should just do the update immediately. In principle this is a good idea, but it makes testing tricky. We've discussed a bit of a testing refactor, lets fix this at the same time. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:286: incoming_signals_lock_.AssertAcquired(); On 2015/03/11 16:58:25, Sami wrote: > I think this is probably flexible enough for now, but to provide some insurance > in the future, could we change pending_delayed_policy_update_ to a timestamp and > DCHECK here that we're not trying to schedule a policy update sooner than that? I ended up adding a DeadlineTaskRunner that should be future proof. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:136: void UpdatePolicy(); On 2015/03/11 16:58:25, Sami wrote: > Maybe move this next to UpdatePolicyLocked()? The difference between the > functions is pretty obvious from the naming so we don't really need to explain > what this does IMO. Done. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:140: void DelayedPolicyUpdateTask(); On 2015/03/11 16:58:25, Sami wrote: > nit: Name this with a verb instead a noun to match the other functions here. The > comment might also be a little unnecessary if this is called something like > PerformDelayedPolicyUpdate() > > Should we also allow this function only on the main thread, since we really only > expect it to be triggered from within the policy update (maybe we could assert > for that too)? This function is gone. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:62: clock_->AdvanceNow(base::TimeDelta::FromMilliseconds(100)); On 2015/03/11 16:58:25, Sami wrote: > Thanks for adding this. By the way, would SetAutoAdvanceNowToPendingTasks > accomplish the same thing? Seems so. https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:868: void UpdatePolicyLocked() override { On 2015/03/11 16:58:25, Sami wrote: > The way we have to reach into the internals to test this things makes me think > we should just pull the policy update mechanism out of the scheduler proper. I > don't have a better suggestion of how to test this without doing that. I guess a > little less invasive option would be to give the scheduler an closure/interface > which it would call whenever the policy updates. WDYT? I think we should pick one style and stick to it. But perhaps we should do that after? https://codereview.chromium.org/994833003/diff/60001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:903: EXPECT_EQ(2, mock_scheduler->update_policy_count_); On 2015/03/11 16:58:25, Sami wrote: > Could you add a comment saying which two updates these are? Done.
This looks great - pretty much exactly along the lines of what I was thinking. I've not looked at the tests in great detail, but a couple of suggestions. https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.cc (right): https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:27: return; This might be clearer with a non-nested early out and use of a CancelableClosureHolder (see my comment below). I think you could do this as: if (deadline >= deadline_) return; if (deadline_ != base::TimeTicks) cancelable_closure_.Cancel() deadline_ = deadline task_runner_->PostDelayedTask(from_here, cancelable_closure_, delay); https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:47: base::Bind(&DeadlineTaskRunner::RunInternal, weak_factory_.GetWeakPtr()); Just use a CancelableClosureHolder for the internal closure - then you can create it once in the constructor and don't need to recreate it again and don't need a weak_factory_ - just call cancelable_closure.Cancel() and cancelable_closure.callback() in SetDeadline() https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:278: // thread. Can you do something like DCHECK(!main_thread_checker_.CalledOnValidThread()); (probably wrapping this in a separate static function since it looks a bit confusing)? https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:304: now + new_policy_duration, now); nit - how about passing in a delay instead of the actual deadline (since it can be calculated by 'now' which you pass as welll).
https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.cc (right): https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:27: return; On 2015/03/12 15:45:30, rmcilroy wrote: > This might be clearer with a non-nested early out and use of a > CancelableClosureHolder (see my comment below). I think you could do this as: > > if (deadline >= deadline_) > return; Unfortunately that will always early out if we use deadline_ = base::TimeTicks() to signal that no timer has been posted. But I think I can simplify this another way. > > if (deadline_ != base::TimeTicks) > cancelable_closure_.Cancel() > > deadline_ = deadline > task_runner_->PostDelayedTask(from_here, cancelable_closure_, delay); https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:47: base::Bind(&DeadlineTaskRunner::RunInternal, weak_factory_.GetWeakPtr()); On 2015/03/12 15:45:30, rmcilroy wrote: > Just use a CancelableClosureHolder for the internal closure - then you can > create it once in the constructor and don't need to recreate it again and don't > need a weak_factory_ - just call cancelable_closure.Cancel() and > cancelable_closure.callback() in SetDeadline() I thought about doing it that way but I think we'd end up with three nested closures. This method has two. https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:278: // thread. On 2015/03/12 15:45:30, rmcilroy wrote: > Can you do something like DCHECK(!main_thread_checker_.CalledOnValidThread()); > (probably wrapping this in a separate static function since it looks a bit > confusing)? I tried that (without the extra function), it makes testing very awkward. Sami suggested offline that we add the extra method as a virtual and override it in the unit test. I argued that we should do that as part of a larger refactor of testing. https://codereview.chromium.org/994833003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:304: now + new_policy_duration, now); On 2015/03/12 15:45:30, rmcilroy wrote: > nit - how about passing in a delay instead of the actual deadline (since it can > be calculated by 'now' which you pass as welll). Done.
Followed Ross's suggestion.
Thanks Alex, I think this lgtm and the pulling out the deadline task logic makes things much neater. Handful of nits below. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:20: } We don't need to cancel the callback here, right? https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:27: if (deadline_ == base::TimeTicks() || deadline < deadline_) { nit: deadline_.is_null() https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.h (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.h:17: class DeadlineTaskRunner { Maybe add a short comment here describing what this is for: // Runs a posted task at latest by a given deadline, but possibly sooner. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner_unittest.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. year++ https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:303: delayed_update_policy_runner_.SetDeadline(FROM_HERE, new_policy_duration, nit: add {} for multiline if. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:125: void EnsureUgentPolicyUpdatePostedOnMainThread( s/Ugent/Urgent/ https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:141: Policy ComputeNewPolicy(base::TimeDelta* new_policy_duration, nit: I'd swap the arguments so that the out argument is last. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:978: // Subsequent calls should not call UpdateTask. nit: UpdatePolicy
https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:20: } On 2015/03/12 18:15:22, Sami wrote: > We don't need to cancel the callback here, right? No that's done implicitly. There is a test for this: DeleteDeadlineTaskRunnerAfterPosting https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.cc:27: if (deadline_ == base::TimeTicks() || deadline < deadline_) { On 2015/03/12 18:15:22, Sami wrote: > nit: deadline_.is_null() Done. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner_unittest.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner_unittest.cc:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2015/03/12 18:15:22, Sami wrote: > year++ Done. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:303: delayed_update_policy_runner_.SetDeadline(FROM_HERE, new_policy_duration, On 2015/03/12 18:15:22, Sami wrote: > nit: add {} for multiline if. Done. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:125: void EnsureUgentPolicyUpdatePostedOnMainThread( On 2015/03/12 18:15:22, Sami wrote: > s/Ugent/Urgent/ Done. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:141: Policy ComputeNewPolicy(base::TimeDelta* new_policy_duration, On 2015/03/12 18:15:22, Sami wrote: > nit: I'd swap the arguments so that the out argument is last. Good point, that's actually in the style guide. https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:978: // Subsequent calls should not call UpdateTask. On 2015/03/12 18:15:22, Sami wrote: > nit: UpdatePolicy Done.
https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.h (right): https://codereview.chromium.org/994833003/diff/180001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.h:17: class DeadlineTaskRunner { On 2015/03/12 18:15:22, Sami wrote: > Maybe add a short comment here describing what this is for: > > // Runs a posted task at latest by a given deadline, but possibly sooner. Done.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/994833003/#ps220001 (title: "Missed a comment and fixed all the typos this time")
Still lgtm.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/994833003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks for addressing the feedback, looks great. lgtm. https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:63: ; nit - this would be clearer as an empty '{ }' rather than a trailing ';' IMO https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:867: class MockRendererSchedulerImpl : public RendererSchedulerImpl { naming nit - this isn't really a mock, RenderSchedulerImplForTest would be more approprate name I think. Happy for this to wait for the testing refactor CL. https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:993: } Nice tests, thanks!
https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.h (right): https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.h:18: class DeadlineTaskRunner { looks like you will need to add CONTENT_EXPORT for component builds of the tests
https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... File content/renderer/scheduler/deadline_task_runner.h (right): https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/deadline_task_runner.h:18: class DeadlineTaskRunner { On 2015/03/12 22:11:30, rmcilroy wrote: > looks like you will need to add CONTENT_EXPORT for component builds of the tests Done. https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:63: ; On 2015/03/12 22:09:34, rmcilroy wrote: > nit - this would be clearer as an empty '{ }' rather than a trailing ';' IMO Done. https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:867: class MockRendererSchedulerImpl : public RendererSchedulerImpl { On 2015/03/12 22:09:34, rmcilroy wrote: > naming nit - this isn't really a mock, RenderSchedulerImplForTest would be more > approprate name I think. Happy for this to wait for the testing refactor CL. Done. https://codereview.chromium.org/994833003/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:993: } On 2015/03/12 22:09:34, rmcilroy wrote: > Nice tests, thanks! Acknowledged.
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/994833003/#ps240001 (title: "Add content export and a few nits")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/994833003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/994833003/#ps260001 (title: "Forgot to change the friend declaration")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/994833003/260001
Message was sent while issue was closed.
Committed patchset #14 (id:260001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c106eb5cac46149507aca52278d5685c3d9f221d Cr-Commit-Position: refs/heads/master@{#320488} |
