Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
387493002:
Fixing and enhancing OrderedSimpleTaskRunner to allow 100% deterministic tests. (Closed)
|
Created:
6 years, 5 months ago by mithro-old Modified:
6 years, 3 months ago CC:
cc-bugs_chromium.org, chromium-reviews, simonhong Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionNew features include;
* Actually running the tasks in the ordered you asked!
* Allow running only pending tasks, all tasks until idle, to a given time or
for a given period.
* Allow stopping of running tasks on *any* arbitrary condition. No longer will
your tasks stop working when someone adds a new task or changes the task
order!
* Task runner intimately connected to time and controls Now().
Supports both automatic management and manual control.
This change makes it possible for the scheduler_unit tests to be 100%
deterministic. It also allows them to be more flexible and less brittle.
BUG=380889
Committed: https://crrev.com/0c0ac6a1616778e3852e3f8b3248688ca9d899b2
Cr-Commit-Position: refs/heads/master@{#294059}
Patch Set 1 #Patch Set 2 : Using iterator. #Patch Set 3 : Adding unittests into ordered_simple_task_runner and fixing compile error. #Patch Set 4 : sprintf is hard. #
Total comments: 12
Patch Set 5 : Rebase onto master. #Patch Set 6 : Fixing code review issues. #Patch Set 7 : Lots of changes. #
Total comments: 8
Patch Set 8 : Rebase onto master after 3 weeks. #Patch Set 9 : Lint fixes. #Patch Set 10 : Small fixes. #Patch Set 11 : Tests now pass. #Patch Set 12 : Using SizeTToString rather then c++11 to_string. #
Total comments: 42
Patch Set 13 : Refactoring the code so that the Now source is a seperate concept from the OrderedSimpleTaskRunner. #Patch Set 14 : Fixing the gn build. #
Total comments: 3
Patch Set 15 : Refactoring the code to allow arbitary conditions for stopping to process tasks. Reimplimented all … #Patch Set 16 : Rebase onto master. #
Total comments: 29
Patch Set 17 : Rebase onto master. #Patch Set 18 : Refactoring to use base::Closure rather then micro-classes. #Patch Set 19 : Trying to fix gcc. #Patch Set 20 : Fixing Windows compile (no idea why this worked on non-windows!) #
Total comments: 26
Patch Set 21 : Fixing android compile issue. #Patch Set 22 : Fixing for Sami's review. #
Total comments: 14
Patch Set 23 : Fixing for Brian's review. #
Total comments: 2
Patch Set 24 : Fixes for Brian's comments. #
Messages
Total messages: 40 (1 generated)
Hi Sami / Brian, This CL doesn't quite work yet, it triggers a bunch of failures but wanted to see what you thought before working on it to much longer. The idea here is to make things like retro frames, deadlines and other similar issues occur reliably because we have control over the time the scheduler is using / comparing too. What do you think? Tim
I like the idea. Quick thought: would it scale better to make it possible for tests to override gfx::FrameTime globally?
On 2014/07/10 14:02:11, Sami wrote: > I like the idea. Quick thought: would it scale better to make it possible for > tests to override gfx::FrameTime globally? I actually want to kill gfx::FrameTime, everything should be using an explicit render time otherwise it is impossible to do pipelining and other similar features. Once we get to that stage we can put in a presubmit to stop people adding in Now usage (including gfx::FrameTime::Now). Tim
On 2014/07/11 00:39:50, mithro wrote: > On 2014/07/10 14:02:11, Sami wrote: > > I like the idea. Quick thought: would it scale better to make it possible for > > tests to override gfx::FrameTime globally? > > I actually want to kill gfx::FrameTime, everything should be using an explicit > render time otherwise it is impossible to do pipelining and other similar > features. Once we get to that stage we can put in a presubmit to stop people > adding in Now usage (including gfx::FrameTime::Now). > > Tim This CL doesn't break any tests now. Tim
https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... cc/test/ordered_simple_task_runner.h:24: void SetAdvanceNow(bool advance_now) { advance_now_ = advance_now; } Looks like these should go into the .cc file. https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:75: virtual void SetupSyntheticBeginFrames() { Not needed? https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:86: void RunPendingTasks(bool advance_now) { AFAICT we never call this with |false| -- does it really need to be parameterized? In any case I'd either suggest an enum or a separate entrypoint to make the call site more readable.
Like where this is headed. Hopefully I can use this to fix some of the SyntheticBeginFrame test flakiness - or was that what you are trying to fix here? https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... cc/test/ordered_simple_task_runner.h:24: void SetAdvanceNow(bool advance_now) { advance_now_ = advance_now; } Can you make the method name more descriptive? It isn't very clear without looking at the implementation that time gets automatically advanced on task execution. https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:75: virtual void SetupSyntheticBeginFrames() { On 2014/07/11 17:22:46, Sami wrote: > Not needed? And if it's not needed can the virtual in the base class be removed again? https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:86: void RunPendingTasks(bool advance_now) { On 2014/07/11 17:22:46, Sami wrote: > AFAICT we never call this with |false| -- does it really need to be > parameterized? In any case I'd either suggest an enum or a separate entrypoint > to make the call site more readable. +1 to a separate entry point or enum.
On 2014/07/14 23:42:32, brianderson wrote: > Like where this is headed. Hopefully I can use this to fix some of the > SyntheticBeginFrame test flakiness - or was that what you are trying to fix > here? This is exactly where I'm going here :). I want to make us have reliable, deterministic tests for all the deadline expiring, retro frames, stuff :). The current kind of non-deterministic nature of the tests is what holding back my BeginFrameSource patches. > > https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... > File cc/test/ordered_simple_task_runner.h (right): > > https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... > cc/test/ordered_simple_task_runner.h:24: void SetAdvanceNow(bool advance_now) { > advance_now_ = advance_now; } > Can you make the method name more descriptive? It isn't very clear without > looking at the implementation that time gets automatically advanced on task > execution. > > https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... > File cc/test/scheduler_test_common.h (right): > > https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... > cc/test/scheduler_test_common.h:75: virtual void SetupSyntheticBeginFrames() { > On 2014/07/11 17:22:46, Sami wrote: > > Not needed? > > And if it's not needed can the virtual in the base class be removed again? > > https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... > cc/test/scheduler_test_common.h:86: void RunPendingTasks(bool advance_now) { > On 2014/07/11 17:22:46, Sami wrote: > > AFAICT we never call this with |false| -- does it really need to be > > parameterized? In any case I'd either suggest an enum or a separate entrypoint > > to make the call site more readable. > > +1 to a separate entry point or enum.
Hi Brian / Sami, This patch has changed quite a bit since you last looked at it. Can you please take another look? I found some weirdness with how OrderedSimpleTaskRunner operated before the addition of now. You can see this behaviour in the https://codereview.chromium.org/396063011/ I also hadn't uploaded the fix which did the delay_time_source overriding, which I have fixed. The original version which tried to use a virtual SetupSyntheticTimeSource failed as in a C++ constructor you always call *that* classes version of the function even for virtual functions. See http://www.parashift.com/c%2B%2B-faq-lite/calling-virtuals-from-ctors.html for more information, C++ is always so full of surprises :/ I also have hooked up the BeginFrameArgs creation using this concept of Now. It should be noted that setting auto-advance now to false lets you advance Now in much larger chunks. This lets you simulate things like getting descheduled, see the test in ordered_task_runner. I think this has an interesting interaction with retro frames and we should add some proper tests for that. I've also been working on converting the LayerTree unit tests to use the ordered_task_runner functionality. This makes the LayerTreeHostAnimationTestAddAnimationAfterAnimating work as using SetAdvanceNow(false) allows me to only run the current pending tasks and not continue to the next frame and thus I can check that animate got called a second time. Lastly I added a tracing to the OrderedSimpleTaskRunner, this makes it much easier to see how / why things are occurring (see below). It probably makes sense to move it into a "cc::test" or similar tracing namespace? ------------------------------------------------------ ------------------------------------------------------ [18030:18030:0717/001502:4431926098896:ERROR:trace_event_impl.cc(1875)] Main: Scheduler::DidCreateAndInitializeOutputSurface[cc] [18030:18030:0717/001502:4431926099012:ERROR:trace_event_impl.cc(2017)] Main: Scheduler::DidCreateAndInitializeOutputSurface[cc] (0.153 ms) [18030:18030:0717/001502:4431926099409:ERROR:trace_event_impl.cc(1875)] Main: DelayBasedTimeSource::SetActive[cc], {active:true} [18030:18030:0717/001502:4431926099553:ERROR:trace_event_impl.cc(1875)] Main: | OrderedSimpleTaskRunner::PostDelayedTask[cc], {RunAt:0, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926099700:ERROR:trace_event_impl.cc(2017)] Main: DelayBasedTimeSource::SetActive[cc], {active:true} (0.262 ms) [18030:18030:0717/001502:4431926099782:ERROR:trace_event_impl.cc(1875)] Main: Scheduler::NotifyBeginMainFrameStarted[cc] [18030:18030:0717/001502:4431926099873:ERROR:trace_event_impl.cc(2017)] Main: Scheduler::NotifyBeginMainFrameStarted[cc] (0.070 ms) [18030:18030:0717/001502:4431926099937:ERROR:trace_event_impl.cc(1875)] Main: Scheduler::NotifyReadyToCommit[cc] [18030:18030:0717/001502:4431926100374:ERROR:trace_event_impl.cc(2017)] Main: Scheduler::NotifyReadyToCommit[cc] (0.410 ms) [18030:18030:0717/001502:4431926100447:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks pendingTask[cc], {waiting:1} [18030:18030:0717/001502:4431926100563:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks pendingTask[cc], {RunAt:0, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926100700:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks running[cc], {RunAt:0, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926100804:ERROR:trace_event_impl.cc(1875)] Main: | OrderedSimpleTaskRunner::PostDelayedTask[cc], {RunAt:16666, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926101002:ERROR:trace_event_impl.cc(1875)] Main: | Scheduler::BeginFrame[cc], {args:{"deadline_us":16666.0,"frame_time_us":0.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} [18030:18030:0717/001502:4431926101144:ERROR:trace_event_impl.cc(1875)] Main: | | Scheduler::BeginImplFrame[cc], {args:{"deadline_us":16666.0,"frame_time_us":0.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} [18030:18030:0717/001502:4431926101879:ERROR:trace_event_impl.cc(1875)] Main: | | | OrderedSimpleTaskRunner::PostDelayedTask[cc], {RunAt:0, Function:"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:573"} [18030:18030:0717/001502:4431926102037:ERROR:trace_event_impl.cc(2017)] Main: | | Scheduler::BeginImplFrame[cc], {args:{"deadline_us":16666.0,"frame_time_us":0.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} (0.873 ms) [18030:18030:0717/001502:4431926102161:ERROR:trace_event_impl.cc(2017)] Main: | Scheduler::BeginFrame[cc], {args:{"deadline_us":16666.0,"frame_time_us":0.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} (1.193 ms) [18030:18030:0717/001502:4431926102264:ERROR:trace_event_impl.cc(2017)] Main: OrderedSimpleTaskRunner::RunPendingTasks running[cc], {RunAt:0, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} (1.558 ms) [18030:18030:0717/001502:4431926102348:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks pendingTask[cc], {waiting:2} [18030:18030:0717/001502:4431926102453:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks pendingTask[cc], {RunAt:0, Function:"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:573"} [18030:18030:0717/001502:4431926102565:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks pendingTask[cc], {RunAt:16666, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926102715:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks running[cc], {RunAt:0, Function:"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:573"} [18030:18030:0717/001502:4431926102796:ERROR:trace_event_impl.cc(1875)] Main: | Scheduler::OnBeginImplFrameDeadline[cc] [18030:18030:0717/001502:4431926103204:ERROR:trace_event_impl.cc(2017)] Main: | Scheduler::OnBeginImplFrameDeadline[cc] (0.401 ms) [18030:18030:0717/001502:4431926103322:ERROR:trace_event_impl.cc(2017)] Main: OrderedSimpleTaskRunner::RunPendingTasks running[cc], {RunAt:0, Function:"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:573"} (0.585 ms) [18030:18030:0717/001502:4431926103396:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::SetNow[cc], {current:16666, new:16666} [18030:18030:0717/001502:4431926103493:ERROR:trace_event_impl.cc(2017)] Main: OrderedSimpleTaskRunner::SetNow[cc], {current:16666, new:16666} (0.073 ms) [18030:18030:0717/001502:4431926103608:ERROR:trace_event_impl.cc(1875)] Main: OrderedSimpleTaskRunner::RunPendingTasks running[cc], {RunAt:16666, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926103718:ERROR:trace_event_impl.cc(1875)] Main: | OrderedSimpleTaskRunner::PostDelayedTask[cc], {RunAt:33332, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} [18030:18030:0717/001502:4431926103922:ERROR:trace_event_impl.cc(1875)] Main: | Scheduler::BeginFrame[cc], {args:{"deadline_us":33332.0,"frame_time_us":16666.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} [18030:18030:0717/001502:4431926104060:ERROR:trace_event_impl.cc(1875)] Main: | | Scheduler::BeginImplFrame[cc], {args:{"deadline_us":33332.0,"frame_time_us":16666.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} [18030:18030:0717/001502:4431926104486:ERROR:trace_event_impl.cc(1875)] Main: | | | OrderedSimpleTaskRunner::PostDelayedTask[cc], {RunAt:33332, Function:"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:573"} [18030:18030:0717/001502:4431926104634:ERROR:trace_event_impl.cc(2017)] Main: | | Scheduler::BeginImplFrame[cc], {args:{"deadline_us":33332.0,"frame_time_us":16666.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} (0.573 ms) [18030:18030:0717/001502:4431926104761:ERROR:trace_event_impl.cc(2017)] Main: | Scheduler::BeginFrame[cc], {args:{"deadline_us":33332.0,"frame_time_us":16666.0,"interval_us":16666.0,"type":"BeginFrameArgs"}} (0.854 ms) [18030:18030:0717/001502:4431926104878:ERROR:trace_event_impl.cc(2017)] Main: OrderedSimpleTaskRunner::RunPendingTasks running[cc], {RunAt:16666, Function:"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:275"} (1.259 ms) ------------------------------------------------------ ------------------------------------------------------ There are a bunch of syntax / formatting issues I need to fix up, I'll fix them up tomorrow. I currently need to go home, pack my bags and sleep :P Tim 'mithro' Ansell https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... cc/test/ordered_simple_task_runner.h:24: void SetAdvanceNow(bool advance_now) { advance_now_ = advance_now; } On 2014/07/11 17:22:46, Sami wrote: > Looks like these should go into the .cc file. Done. https://codereview.chromium.org/387493002/diff/60001/cc/test/ordered_simple_t... cc/test/ordered_simple_task_runner.h:24: void SetAdvanceNow(bool advance_now) { advance_now_ = advance_now; } On 2014/07/14 23:42:31, brianderson wrote: > Can you make the method name more descriptive? It isn't very clear without > looking at the implementation that time gets automatically advanced on task > execution. I changed it to SetAutoAdvanceNowToPendingTasks but I'm not sure that is the right name. Open to suggestions. https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:75: virtual void SetupSyntheticBeginFrames() { On 2014/07/14 23:42:31, brianderson wrote: > On 2014/07/11 17:22:46, Sami wrote: > > Not needed? > > And if it's not needed can the virtual in the base class be removed again? Done. https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:75: virtual void SetupSyntheticBeginFrames() { On 2014/07/14 23:42:31, brianderson wrote: > On 2014/07/11 17:22:46, Sami wrote: > > Not needed? > > And if it's not needed can the virtual in the base class be removed again? Acknowledged. https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:86: void RunPendingTasks(bool advance_now) { On 2014/07/14 23:42:31, brianderson wrote: > On 2014/07/11 17:22:46, Sami wrote: > > AFAICT we never call this with |false| -- does it really need to be > > parameterized? In any case I'd either suggest an enum or a separate entrypoint > > to make the call site more readable. > > +1 to a separate entry point or enum. Done. https://codereview.chromium.org/387493002/diff/60001/cc/test/scheduler_test_c... cc/test/scheduler_test_common.h:86: void RunPendingTasks(bool advance_now) { On 2014/07/11 17:22:46, Sami wrote: > AFAICT we never call this with |false| -- does it really need to be > parameterized? In any case I'd either suggest an enum or a separate entrypoint > to make the call site more readable. I refactored this code slightly to be more inline with the original source. This extra level of indirection was left over from my first attempt where Now was managed by the scheduler rather than the task runner. I was sure there were 2 tests which needed the non-advancing behaviour but they seem to have disappeared (or maybe I imagined them in the first place).
https://codereview.chromium.org/387493002/diff/140001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/387493002/diff/140001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:166: base::TimeTicks now = base::TimeTicks(); // gfx::FrameTime::Now(); Why did this change? https://codereview.chromium.org/387493002/diff/140001/cc/test/begin_frame_arg... File cc/test/begin_frame_args_test.h (right): https://codereview.chromium.org/387493002/diff/140001/cc/test/begin_frame_arg... cc/test/begin_frame_args_test.h:24: OrderedSimpleTaskRunner& task_runner); Please include a comment describing what these do with the task_runner. https://codereview.chromium.org/387493002/diff/140001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/140001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:40: TRACE_EVENT_INSTANT2("cc", What categories do other cc testing files use? https://codereview.chromium.org/387493002/diff/140001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:107: break; So this function doesn't run all pending tasks if new tasks are posted that sit in the middle of the existing tasks. Does that play well with test expectations? Should "RunPendingTasks" be called something else?
Hi Brian, Haven't had a chance to upload a new version but wanted to reply to a couple of the comments you made. Tim https://codereview.chromium.org/387493002/diff/140001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_state_machine.cc (right): https://codereview.chromium.org/387493002/diff/140001/cc/scheduler/scheduler_... cc/scheduler/scheduler_state_machine.cc:166: base::TimeTicks now = base::TimeTicks(); // gfx::FrameTime::Now(); On 2014/07/17 05:26:03, brianderson wrote: > Why did this change? Opps, this shouldn't be in this commit :) https://codereview.chromium.org/387493002/diff/140001/cc/test/begin_frame_arg... File cc/test/begin_frame_args_test.h (right): https://codereview.chromium.org/387493002/diff/140001/cc/test/begin_frame_arg... cc/test/begin_frame_args_test.h:24: OrderedSimpleTaskRunner& task_runner); On 2014/07/17 05:26:03, brianderson wrote: > Please include a comment describing what these do with the task_runner. Acknowledged. https://codereview.chromium.org/387493002/diff/140001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/140001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:107: break; On 2014/07/17 05:26:03, brianderson wrote: > So this function doesn't run all pending tasks if new tasks are posted that sit > in the middle of the existing tasks. Does that play well with test expectations? > Should "RunPendingTasks" be called something else? See the discussion on the https://codereview.chromium.org/396063011/ about this topic.
Hi Brian / Sami,
Can you please take another look at this change? I'm still looking into why it
causes the following tests to fail but would like a review while I work on that;
SchedulerTest.BeginRetroFrame_SwapThrottled
SchedulerTest.StopBeginFrameAfterDidLoseOutputSurfaceWithSyntheticBeginFrameSource
SchedulerTest.SyntheticBeginFrames_SwapThrottled
Thanks!
Tim
https://codereview.chromium.org/387493002/diff/140001/cc/test/begin_frame_arg...
File cc/test/begin_frame_args_test.h (right):
https://codereview.chromium.org/387493002/diff/140001/cc/test/begin_frame_arg...
cc/test/begin_frame_args_test.h:24: OrderedSimpleTaskRunner& task_runner);
On 2014/07/17 05:26:03, brianderson wrote:
> Please include a comment describing what these do with the task_runner.
Done.
Hi guys, The tests now all pass as far as I can see. Can you please take a look as there is a bunch more patches queue up behind this one. Thanks! Tim
Thanks Tim, I think this looks pretty good. Added some miscellaneous thoughts. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:21: #include "ui/gfx/frame_time.h" Not sure why you needed to add this include here? https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:190: virtual base::TimeTicks Now() const; bikeshed: I'd prefer a more descriptive name like CurrentFrameTime() or something to that effect. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1313: client.task_runner().SetNow(base::TimeTicks::FromInternalValue(1133332)); Does this value have any significance? Could we compute it like Now() + 1 second or something a little less magic? :) https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:69: // static https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:154: std::stable_sort( Can you call SortTasks here? https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:179: TRACE_EVENT_INSTANT2("cc", Random question: can we get traces out of unit tests nowadays or are you using the trace-to-console option? https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:23: explicit OrderedSimpleTaskRunner(bool advance_now = true); No default arguments in chromium please :P https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:24: // base::TestSimpleTaskRunner implementation: https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:28: virtual bool PostNonNestableDelayedTask( Just curious: do we ever post non-nestable tasks from the compositor? If not, we could probably simplify this a bit and just make this a NOTREACHED(). https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:76: virtual std::string TypeString() const OVERRIDE; Is this used anywhere? https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:90: // A bunch of tests require Now() to be > BeginFrameArgs::DefaultInterval() Oh, really? That seems broken :)
One major comment about the structure: it's confusing that OrderedSimpleTaskRunner is the owner of time and that there are many places to query for the time and set the time indirectly. I think it would be clearer if there was a separate "NowSource" class that owned the time and was injected into each of the test classes that need it. The "NowSource" class would be: 1) the only class you can use to change the current time. 2) the only test class with a public version of Now(). https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (left): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:701: (AnticipatedDrawTime() - base::TimeTicks::Now()).InMillisecondsF()); It was a bug to call base::TimeTicks::Now() rather than the gfx version. This patch fixes it - thanks. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (left): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:268: EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending()); Why was this removed? If this expectation is no longer true, then the comment above is wrong. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:278: EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending()); Why was this removed? https://codereview.chromium.org/387493002/diff/240001/cc/test/begin_frame_arg... File cc/test/begin_frame_args_test.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/begin_frame_arg... cc/test/begin_frame_args_test.h:27: const OrderedSimpleTaskRunner& task_runner); Instead of having such a strong dependency on the OrderedSimpleTask runner, it would be better to just take in a timestamp for Now. I don't think OrderedSimpleTaskRunner should be exposing a public version of its concept of Now() anyway. https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:34: base::TimeTicks Now() const; protected not public. https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:37: void SetNow(base::TimeTicks time); Remove this. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:63: virtual base::TimeTicks Now() const OVERRIDE; Please make this protected, not public. The DelayBasedTimeSource should make its Now() protected too. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:67: OrderedSimpleTaskRunner& task_runner() { return *test_task_runner_; } Please remove SetNow and task_runner from this class. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:90: // A bunch of tests require Now() to be > BeginFrameArgs::DefaultInterval() Yes, that seems broken. Is it because some operations cause the timeline to go negative and we don't handle that properly? Or is there another reason? https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:101: virtual base::TimeTicks Now() const OVERRIDE; protected, not public. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:112: void SetNow(base::TimeTicks time); SetNow should be removed.
Hi Brian, I've refactored the Now time source out into it's own TestNowSource class. OrderedSimpleTaskRunner is still pretty tied to the now source functionality as it moves the time around as needed by tasks. Can you please take another look? Thanks Tim https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.cc File cc/scheduler/scheduler.cc (left): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.... cc/scheduler/scheduler.cc:701: (AnticipatedDrawTime() - base::TimeTicks::Now()).InMillisecondsF()); On 2014/08/20 20:04:30, brianderson wrote: > It was a bug to call base::TimeTicks::Now() rather than the gfx version. This > patch fixes it - thanks. Acknowledged. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:21: #include "ui/gfx/frame_time.h" On 2014/08/19 at 18:45:04, Sami wrote: > Not sure why you needed to add this include here? Removed. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:190: virtual base::TimeTicks Now() const; On 2014/08/19 18:45:04, Sami wrote: > bikeshed: I'd prefer a more descriptive name like CurrentFrameTime() or > something to that effect. I'm worried it will be to easy to confuse CurrentFrameTime with the "CurrentFrameTimeToRenderFor". https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (left): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:268: EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending()); On 2014/08/20 20:04:30, brianderson wrote: > Why was this removed? If this expectation is no longer true, then the comment > above is wrong. There is a lot of complexity here that I'm finding hard to express via email. Can we organise a time to walk through the traces? Now that the test runs are deterministic (and hence repeatable) we can do meaningful diffs of the tests and see how they compare. I wrote a little script which took the trace output of the InitializeOutputSurfaceAndFirstCommit function from each test and compares them. It came up with ~3 distinctive groups where it behaves reasonably differently but describing them is difficult. Here is what I have so far; ----------------------------------------------------------------- I wrote a little script which took the trace output of the InitializeOutputSurfaceAndFirstCommit function from each test and compares them. This is what it came up with; {'382676c410a9cdfda58fa5a71c97ac2b.md5sum': ['SchedulerTest.SyntheticBeginFrames_And_VSyncThrottlingDisabled', 'SchedulerTest.SyntheticBeginFrames_And_VSyncThrottlingDisabled_SwapThrottled', 'SchedulerTest.VSyncThrottlingDisabled', 'SchedulerTest.VSyncThrottlingDisabled_SwapThrottled'], '7d3a414c65d1d09bb98426709aeab54e.md5sum': ['SchedulerTest.StopBeginFrameAfterDidLoseOutputSurfaceWithSyntheticBeginFrameSource', 'SchedulerTest.SyntheticBeginFrames_SwapThrottled'], 'a1e9f900e7c6f80ffc47f99a081cd344.md5sum': ['SchedulerTest.RequestCommitInsideFailedDraw', 'SchedulerTest.RequestRedrawInsideDraw', 'SchedulerTest.RequestRedrawInsideFailedDraw'], 'a3ee0cca7c9225d8d724d53b6fb9b0e7.md5sum': ['SchedulerTest.BeginRetroFrame', 'SchedulerTest.BeginRetroFrame_SwapThrottled', 'SchedulerTest.DidLoseOutputSurfaceAfterBeginFrameStarted', 'SchedulerTest.DidLoseOutputSurfaceAfterBeginFrameStartedWithHighLatency', 'SchedulerTest.DidLoseOutputSurfaceAfterBeginFrameStartedWithHighLatencyWithImplPaint', 'SchedulerTest.DidLoseOutputSurfaceAfterBeginRetroFramePosted', 'SchedulerTest.DidLoseOutputSurfaceAfterReadyToCommit', 'SchedulerTest.DidLoseOutputSurfaceAfterReadyToCommitWithImplPainting', 'SchedulerTest.DidLoseOutputSurfaceAfterSetNeedsManageTiles', 'SchedulerTest.DidLoseOutputSurfaceDuringBeginRetroFrameRunning', 'SchedulerTest.ManageTilesOncePerFrame', 'SchedulerTest.NoSwapWhenDrawFails', 'SchedulerTest.NotSkipMainFrameIfHighLatencyAndCanActivateTooLong', 'SchedulerTest.NotSkipMainFrameIfHighLatencyAndCanCommitTooLong', 'SchedulerTest.NotSkipMainFrameInPreferSmoothnessMode', 'SchedulerTest.RequestCommit', 'SchedulerTest.RequestCommitAfterBeginMainFrameSent', 'SchedulerTest.RequestCommitInsideDraw', 'SchedulerTest.ShouldUpdateVisibleTiles', 'SchedulerTest.SkipMainFrameIfHighLatencyAndCanCommitAndActivateBeforeDeadline'], 'aa4dbaeb7644e5fbc71db4a476b3a279.md5sum': ['SchedulerTest.ManageTiles', 'SchedulerTest.TriggerBeginFrameDeadlineEarly']} Groups a3ee0cca7c9225d8d724d53b6fb9b0e7 and aa4dbaeb7644e5fbc71db4a476b3a279 are identical except there is an extra "SetNeedsManageTiles call, so we can safely ignore aa4dbaeb7644e5fbc71db4a476b3a279. Group a1e9f900e7c6f80ffc47f99a081cd344 and a3ee0cca7c9225d8d724d53b6fb9b0e7 are both cases where external BeginFrames are posted. The difference is that in group a1e9f900e7c6f80ffc47f99a081cd344 ScheduleBeginImplFrameDeadline is posted with a zero second deadline. Group 382676c410a9cdfda58fa5a71c97ac2b goes through the SetupNextBeginFrameWhenVSyncThrottlingDisabled function. ---------------------------- | OrderedSimpleTaskRunner::PostDelayedTask[cc], {task:{"posted_from":"SetupNextBeginFrameWhenVSyncThrottlingDisabled@../../cc/scheduler/scheduler.cc:335", "run_at":100000}} ---------------------------- Group 7d3a414c65d1d09bb98426709aeab54e goes through the delay_based_time_source. ---------------------------- | | OrderedSimpleTaskRunner::PostDelayedTask[cc], {task:{"posted_from":"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:276", "run_at":116662}} ---------------------------- Just before line 224 we have the following situations; * If using throttling and an external begin frame source - there is nothing currently scheduled in the taskrunner; * If using throttling and an synthetic begin frame source - there is the following task scheduled; task:{"posted_from":"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:276", "run_at":116662} * If not using throttling - there is; task:{"posted_from":"SetupNextBeginFrameWhenVSyncThrottlingDisabled@../../cc/scheduler/scheduler.cc:335", "run_at":100000}} A line 230 we have the following situations; * If using throttling and an external begin frame source - there is nothing currently scheduled in the taskrunner; | OrderedSimpleTaskRunner::RunPendingTasks[cc], {this:{ "0":{"posted_from":"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:582","run_at":110000}, "now_in_microseconds":110000, "pending_tasks":1}} * If not using throttling - there is; | OrderedSimpleTaskRunner::RunPendingTasks[cc], {this:{ "0":{"posted_from":"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:582","run_at":110000}, "now_in_microseconds":110000, "pending_tasks":1}} * If using throttling and an synthetic begin frame source - there is the following task scheduled; | OrderedSimpleTaskRunner::RunPendingTasks[cc], {this:{ "0":{"posted_from":"ScheduleBeginImplFrameDeadline@../../cc/scheduler/scheduler.cc:582","run_at":110000}, "1":{"posted_from":"PostNextTickTask@../../cc/scheduler/delay_based_time_source.cc:276","run_at":116662}, "now_in_microseconds":110000, "pending_tasks":2}}[0;m This means the call at 232 is going to have some divergence. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:278: EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending()); On 2014/08/20 20:04:30, brianderson wrote: > Why was this removed? See above. https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:1313: client.task_runner().SetNow(base::TimeTicks::FromInternalValue(1133332)); On 2014/08/19 at 18:45:04, Sami wrote: > Does this value have any significance? Could we compute it like Now() + 1 second or something a little less magic? :) Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/begin_frame_arg... File cc/test/begin_frame_args_test.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/begin_frame_arg... cc/test/begin_frame_args_test.h:27: const OrderedSimpleTaskRunner& task_runner); On 2014/08/20 at 20:04:31, brianderson wrote: > Instead of having such a strong dependency on the OrderedSimpleTask runner, it would be better to just take in a timestamp for Now. > > I don't think OrderedSimpleTaskRunner should be exposing a public version of its concept of Now() anyway. I feel like as the system evolved it might want more information to create the begin frame args which won't be available just on a timestamp. With the refactor it's now only dependent on the TestNowSource. It is also a strong signal that you should be using *this* CreateBeginFrameArgsForTesting (which will get the right Now() value) rather than the other ones. https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:23: explicit OrderedSimpleTaskRunner(bool advance_now = true); On 2014/08/19 at 18:45:04, Sami wrote: > No default arguments in chromium please :P How does one do this otherwise? https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:24: On 2014/08/19 at 18:45:04, Sami wrote: > // base::TestSimpleTaskRunner implementation: Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:28: virtual bool PostNonNestableDelayedTask( On 2014/08/19 at 18:45:04, Sami wrote: > Just curious: do we ever post non-nestable tasks from the compositor? If not, we could probably simplify this a bit and just make this a NOTREACHED(). No idea. I do think this task runner could possibly be suitable to end up in base/ once we prove its usefulness? It would be bad for it to be reimplemented many times (because it's been surprisingly hard to actually get right). https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:34: base::TimeTicks Now() const; On 2014/08/20 at 20:04:31, brianderson wrote: > protected not public. Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:37: void SetNow(base::TimeTicks time); On 2014/08/20 at 20:04:31, brianderson wrote: > Remove this. Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:63: virtual base::TimeTicks Now() const OVERRIDE; On 2014/08/20 at 20:04:31, brianderson wrote: > Please make this protected, not public. The DelayBasedTimeSource should make its Now() protected too. Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:67: OrderedSimpleTaskRunner& task_runner() { return *test_task_runner_; } On 2014/08/20 at 20:04:31, brianderson wrote: > Please remove SetNow and task_runner from this class. Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:76: virtual std::string TypeString() const OVERRIDE; On 2014/08/19 at 18:45:04, Sami wrote: > Is this used anywhere? Yes, in the parent's AsValue function. It means traces include TestDelayBasedTimeSource rather than DelayBasedTimeSource which is really useful for making sure that the test is using the right time source :). https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:90: // A bunch of tests require Now() to be > BeginFrameArgs::DefaultInterval() On 2014/08/20 at 20:04:31, brianderson wrote: > Yes, that seems broken. Is it because some operations cause the timeline to go negative and we don't handle that properly? Or is there another reason? There are two things which go wrong; * base::TimeTicks with a value of zero is treated specially by a huge amount of code (it's assume to be null / invalid value). * As Brain suggested a bunch of stuff gets a bit grumpy when the time value goes negative. There are a couple of DCHECKS for negative values and one place where negative values are replace with the invalid value. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:101: virtual base::TimeTicks Now() const OVERRIDE; On 2014/08/20 at 20:04:31, brianderson wrote: > protected, not public. Done. https://codereview.chromium.org/387493002/diff/240001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:112: void SetNow(base::TimeTicks time); On 2014/08/20 at 20:04:31, brianderson wrote: > SetNow should be removed. Done.
https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (left): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:268: EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending()); In your description of the possible states at line 230, you indicate in the first state that there is nothing scheduled in the task runner, but also include output that makes it look like there is a deadline pending. Which one is true? I'm guessing the latter, because: On line 231, EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending()), should indicate that there is a deadline task posted. In which case, line 232 should run the posted deadline, and we should be able to keep the expectation that the deadline is no longer pending. https://codereview.chromium.org/387493002/diff/280001/cc/test/scheduler_test_... File cc/test/scheduler_test_common.h (right): https://codereview.chromium.org/387493002/diff/280001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:82: int layer_tree_host_id) { Should this body move to the cc file? https://codereview.chromium.org/387493002/diff/280001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:83: // A bunch of tests require Now() to be > BeginFrameArgs::DefaultInterval() Please describe why in the comment. https://codereview.chromium.org/387493002/diff/280001/cc/test/scheduler_test_... cc/test/scheduler_test_common.h:87: new OrderedSimpleTaskRunner(now_src, true); bool automatically_advance_now = true; ... new OrderedSimpleTaskRunner(now_src, automatically_advance_now);
Hi Brian, FYI Here is a screenshot of the different runs, dunno if it'll make it will through to reitveld or not... Tim On 22 August 2014 07:48, <brianderson@chromium.org> wrote: > > https://codereview.chromium.org/387493002/diff/240001/cc/ > scheduler/scheduler_unittest.cc > File cc/scheduler/scheduler_unittest.cc (left): > > https://codereview.chromium.org/387493002/diff/240001/cc/ > scheduler/scheduler_unittest.cc#oldcode268 > cc/scheduler/scheduler_unittest.cc:268: > EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending()); > In your description of the possible states at line 230, you indicate in > the first state that there is nothing scheduled in the task runner, but > also include output that makes it look like there is a deadline pending. > Which one is true? > > I'm guessing the latter, because: > > On line 231, EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending()), > should indicate that there is a deadline task posted. > > In which case, line 232 should run the posted deadline, and we should be > able to keep the expectation that the deadline is no longer pending. > > https://codereview.chromium.org/387493002/diff/280001/cc/ > test/scheduler_test_common.h > File cc/test/scheduler_test_common.h (right): > > https://codereview.chromium.org/387493002/diff/280001/cc/ > test/scheduler_test_common.h#newcode82 > cc/test/scheduler_test_common.h:82: int layer_tree_host_id) { > Should this body move to the cc file? > > https://codereview.chromium.org/387493002/diff/280001/cc/ > test/scheduler_test_common.h#newcode83 > cc/test/scheduler_test_common.h:83: // A bunch of tests require Now() to > be > BeginFrameArgs::DefaultInterval() > Please describe why in the comment. > > https://codereview.chromium.org/387493002/diff/280001/cc/ > test/scheduler_test_common.h#newcode87 > cc/test/scheduler_test_common.h:87: new OrderedSimpleTaskRunner(now_src, > true); > bool automatically_advance_now = true; > ... > new OrderedSimpleTaskRunner(now_src, automatically_advance_now); > > https://codereview.chromium.org/387493002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/240001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:232: client->task_runner().RunPendingTasks(); Thanks for attaching the diff, it made it very clear what's happening. In the synthetic source + throttled case, the deadline and next BeginFrame are pending here. So, even though we exit the pending deadline, we immediately re-enter the next BeginFrame and have a new deadline pending. Can you update the comments then? Or keep the expectations and add an OrderedSimpleTaskRunner::RunNextPendingTask() to avoid immediately re-entering the deadline? I imagine an OrderedSimpleTaskRunner::RunNextPendingTask() will be really useful elsewhere too.
Hi Brian / Sami (and Simon),
After looking into your idea of adding a
OrderedSimpleTaskRunner::RunNextPendingTask()
and finding that it make the test super brittle I came to the realisation
we wanted to actually "run all tasks until a condition was met".
I ended up refactoring the code to support running tasks until a callback
returns false. I then re-implemented all the other stop conditions on
top of this idea.
Once this was done, I was then able to do the following;
// Run the posted deadline task.
EXPECT_TRUE(scheduler->BeginImplFrameDeadlinePending());
client->task_runner().RunTasksUntil(
RunWhileImplFrameDeadlinePending(scheduler, false));
EXPECT_FALSE(scheduler->BeginImplFrameDeadlinePending());
Which is really what the test is *actually* trying to do. With these
changes the traces for InitializeOutputSurfaceAndFirstCommit on all
scheduler settings become very similar - and can be pretty much visually
verified as being equal. I've attached the output of diffuse tool which
shows how the differences in traces. It is clear that in *all* cases
exactly two frames are processed.
I think from the initial changes in this CL, it is clear that
scheduler_unittest can be substantially improved to make use of this new
ability which would both;
* Makes the test less brittle, IE If something gets split into two tasks
the test won't be affected.
* Makes the test actually express the actual intent of the author and
conditions they care about.
* Removes a lot of unnecessary code in areas the test doesn't care about.
I hope this task runner might be generally useful inside Chrome in the
future but want to prove how useful it is in cc/.
If you could please take another look it would be greatly appreciated.
Tim 'mithro' Ansell
On 23 August 2014 09:34, <brianderson@chromium.org> wrote:
> https://codereview.chromium.org/387493002/
>
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
I sort of like the idea of a run-until-condition-x message loop, but I worry that it might make our tests a little too loose in some cases. For instance we might start emitting entirely new actions in some cases and the tests would still pass, even though that could be a serious bug. Maybe it's a case of making the checks more strict by disallowing certain things from happening. You've got the 100 task limit here which is probably too permissive. Or am I being paranoid -- Brian, WDYT?
Sami brings up a good point. Using a condition to stop running tasks might make us run more tasks than expected. However, RunPendingTasks has the same problem and I don't think we'll get rid of that. Tim, can you explain why a OrderedSimpleTaskRunner::RunNextPendingTask solution ended up being flaky? If we have full control over Now() and our tests are deterministic, shouldn't the next pending task always be the same between runs at any given point in the test? I'd prefer a RunNextPendingTask solution if possible. However, if RunTasksUntil proves useful in other cases, I'm up for keeping it since it is no worse than RunPendingTasks.
On 28 August 2014 13:12, <brianderson@chromium.org> wrote: > Sami brings up a good point. Using a condition to stop running tasks might > make > us run more tasks than expected. The task runner supports stopping on multiple different conditions. If you want to prevent something from occurring you can also give it to RunTasksUntil also. > However, RunPendingTasks has the same problem > and I don't think we'll get rid of that. > Yes, infact that is already happening. > Tim, can you explain why a OrderedSimpleTaskRunner::RunNextPendingTask > solution > ended up being flaky? If we have full control over Now() and our tests are > deterministic, shouldn't the next pending task always be the same between > runs > at any given point in the test? > The problem isn't flakiness. The issue is that in the many different configurations of the scheduler which share common code paths determining when the next pending task is the actual one you want is extremely tricky. It also leads to the tests being very brittle about the task queue ordering. If the ordering of tasks which you don't care about change then you end up with the test failing. I'd prefer a RunNextPendingTask solution if possible. However, if > RunTasksUntil > proves useful in other cases, I'm up for keeping it since it is no worse > than > RunPendingTasks. > With this system you can get "RunNextPendingTask" via two different methods; task_runner->SetTimeout(1) task_runner->RunPendingTask() You can also do task_runner->RunTasksUntil(OrderedTaskRunner::RunNTasks(1)) In both cases, at most 1 new task will be run. Tim To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:75: base::Callback<bool(void)> AsCallback() const; Why is this needed? https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:140: class RunOnlyBefore : public RunCheckNeedsNextTaskTime { This class is 4 levels of inheritance deep! My head hurts =P. Please limit it to 1. Even though there will be more code duplication, I think it'll help remove a lot of the boilerplate and indirection of the inheritance.
https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:157: scoped_refptr<DelayBasedTimeSource> time_source); Could this be a scoped_ptr instead? Looks like this class could take ownership of the time source. https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:84: task_runner().SetTimeout(100); Please call this something that makes it clear 100 refers to the number of tasks and not a time duration, e.g., SetTaskCountLimit(). https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:111: external_begin_frame); Having |external_begin_frame| here is redundant because of the if-statement. Should this trace be a the top level? https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:121: scoped_refptr<TestNowSource> now_src() { return now_src_; } Could this be a raw pointer? Nobody needs to hang on to the returned reference, right? https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:246: scoped_refptr<TestNowSource> now_src_; scoped_ptr? https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:266: client->task_runner().RunTasksUntil( This reads a bit oddly -- "Run tasks until while impl frame deadline pending = false". How about: RunTasksUntil(CheckImplFrameDeadlinePending(scheduler, false)) (or HasImplFrameDeadlinePending) https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:33: // operators needed by std::set and comparision s/comparision/comparison/ https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:84: virtual bool Check() OVERRIDE; It might be worth adding a comment saying when Check() is called (so that RunNTasks is guaranteed to work). Also document the return value. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:140: class RunOnlyBefore : public RunCheckNeedsNextTaskTime { On 2014/08/28 22:15:16, brianderson wrote: > This class is 4 levels of inheritance deep! My head hurts =P. Please limit it to > 1. > > Even though there will be more code duplication, I think it'll help remove a lot > of the boilerplate and indirection of the inheritance. Yeah, I think we could just have one Check() that is given all the parameters any one of these needs. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:169: void SetTimeout(size_t max_tasks) { max_tasks_ = max_tasks; } Instead of adding this arbitrary limit, should we just rely on the overall test timing out like we do now? https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:177: bool RunTasksUntil(const RunCheck& check_callback); Do we need all of these entrypoints? How about adding the ones we need initially and introducing more as we go on? Otherwise I'm not sure we can still call this a simple task runner :) https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:200: scoped_refptr<TestNowSource> now_src_; scoped_ptr?
Hi Sami / Brian, Took on your feedback regarding the micro classes and refactored to use base::Closure instead. Can you PTAL? Tim https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler.h File cc/scheduler/scheduler.h (right): https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler.... cc/scheduler/scheduler.h:157: scoped_refptr<DelayBasedTimeSource> time_source); On 2014/08/29 13:13:51, Sami wrote: > Could this be a scoped_ptr instead? Looks like this class could take ownership > of the time source. DelayBasedTimeSource is ref counted, so have to use a scoped_refptr here. I don't want to change DelayBasedTimeSource in this CL, but definite candidate for fixes in the future. https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:84: task_runner().SetTimeout(100); On 2014/08/29 13:13:51, Sami wrote: > Please call this something that makes it clear 100 refers to the number of tasks > and not a time duration, e.g., SetTaskCountLimit(). Done. I went with "SetRunTaskLimit". https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:111: external_begin_frame); On 2014/08/29 at 13:13:51, Sami wrote: > Having |external_begin_frame| here is redundant because of the if-statement. Should this trace be a the top level? Removed. This trace only existed to make the indentation of the trace output match between the scheduler_->BeginFrame and task_runner() traces. https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:121: scoped_refptr<TestNowSource> now_src() { return now_src_; } On 2014/08/29 13:13:51, Sami wrote: > Could this be a raw pointer? Nobody needs to hang on to the returned reference, > right? Done. https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:246: scoped_refptr<TestNowSource> now_src_; On 2014/08/29 13:13:51, Sami wrote: > scoped_ptr? now_src is currently ref-counted. The fact that the now_src has to be given to many different locations (scheduler, task runner, anything else which is used) tends to point towards ref counting? https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:266: client->task_runner().RunTasksUntil( On 2014/08/29 13:13:51, Sami wrote: > This reads a bit oddly -- "Run tasks until while impl frame deadline pending = > false". How about: > > RunTasksUntil(CheckImplFrameDeadlinePending(scheduler, false)) > > (or HasImplFrameDeadlinePending) With the other changes this now reads as; RunTasks(WhileImplFrameDeadlinePending(false)); https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:33: // operators needed by std::set and comparision On 2014/08/29 at 13:13:52, Sami wrote: > s/comparision/comparison/ Done. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:75: base::Callback<bool(void)> AsCallback() const; On 2014/08/28 at 22:15:16, brianderson wrote: > Why is this needed? Removed now. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:84: virtual bool Check() OVERRIDE; On 2014/08/29 at 13:13:51, Sami wrote: > It might be worth adding a comment saying when Check() is called (so that RunNTasks is guaranteed to work). Also document the return value. Done. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:140: class RunOnlyBefore : public RunCheckNeedsNextTaskTime { On 2014/08/29 at 13:13:51, Sami wrote: > On 2014/08/28 22:15:16, brianderson wrote: > > This class is 4 levels of inheritance deep! My head hurts =P. Please limit it to > > 1. > > > > Even though there will be more code duplication, I think it'll help remove a lot > > of the boilerplate and indirection of the inheritance. > > Yeah, I think we could just have one Check() that is given all the parameters any one of these needs. After some thinking, I refactored this code to just use base::Callback/base::Closure directly rather than a micro-class type system. The micro classes where only really advantageous for making the "RunNTasks / RunOnlyExistingTasks" storage explicit. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:169: void SetTimeout(size_t max_tasks) { max_tasks_ = max_tasks; } On 2014/08/29 at 13:13:51, Sami wrote: > Instead of adding this arbitrary limit, should we just rely on the overall test timing out like we do now? This makes our tests fail much faster when things are broken, faster failing tests are good :) https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:177: bool RunTasksUntil(const RunCheck& check_callback); On 2014/08/29 at 13:13:51, Sami wrote: > Do we need all of these entrypoints? How about adding the ones we need initially and introducing more as we go on? Otherwise I'm not sure we can still call this a simple task runner :) Drastically simplified "RunTaskUntil" function with the base::Callback change. The convenience functions (RunPendingTasks, RunUntilIdle, RunUntilTime and RunForPeriod) are all used by the scheduler_unittest. It would be nice to see RunPendingTasks go away (because its usage leads to fragile tests and is almost always not what you want) but that requires more work on the scheduler_unittest side. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:200: scoped_refptr<TestNowSource> now_src_; On 2014/08/29 at 13:13:52, Sami wrote: > scoped_ptr? See above.
Thanks for simplifying. https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/320001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:246: scoped_refptr<TestNowSource> now_src_; On 2014/09/01 06:19:46, mithro wrote: > On 2014/08/29 13:13:51, Sami wrote: > > scoped_ptr? > > now_src is currently ref-counted. The fact that the now_src has to be given to > many different locations (scheduler, task runner, anything else which is used) > tends to point towards ref counting? Right, I was just wondering if the the scheduler's life time is enough to keep it valid for all clients automatically. If there's a risk that some of those things outlive the scheduler (which they really shouldn't as far as I can see), then we need ref counting. No need to change that in this patch though. https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/320001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:177: bool RunTasksUntil(const RunCheck& check_callback); On 2014/09/01 06:19:46, mithro wrote: > On 2014/08/29 at 13:13:51, Sami wrote: > > Do we need all of these entrypoints? How about adding the ones we need > initially and introducing more as we go on? Otherwise I'm not sure we can still > call this a simple task runner :) > > Drastically simplified "RunTaskUntil" function with the base::Callback change. > > The convenience functions (RunPendingTasks, RunUntilIdle, RunUntilTime and > RunForPeriod) are all used by the scheduler_unittest. Is that in a future patch or did I miss it? It feels like we should avoid adding code that we're not using especially since this patch is already pretty big. > It would be nice to see RunPendingTasks go away (because its usage leads to > fragile tests and is almost always not what you want) but that requires more > work on the scheduler_unittest side. https://codereview.chromium.org/387493002/diff/400001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/400001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:87: base::Callback<bool(void)> WhileImplFrameDeadlinePending(bool state) { I didn't see any calls to this with |state| = false, so I'd lean toward removing the parameter for now. https://codereview.chromium.org/387493002/diff/400001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:94: bool WhileImplFrameDeadlinePendingCallback(bool state) { Could you move this under protected: (or private:) to make it more obvious that it's an internal helper function? https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:130: if (pending_tasks_.size() <= 0) nit: Add braces https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:140: if (pending_tasks_.size() <= 0) nit: Add braces https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:176: modifiable_conditions.push_back(AdvanceNow()); It's clever but having AdvanceNow() as a condition feels wrong, since always returns true and is there just for the side-effect. How about inlining the code in the run loop instead? I think it would be easier to read that way. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:190: goto run_tasks_exit; I feel like I almost have to offer an alternative here :) How about wrapping inside_run_tasks_until_ in a base::AutoReset and just returning here? https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:273: bool OrderedSimpleTaskRunner::WhileTaskRunCountBelowCallback( nit: Add blank line. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:286: bool OrderedSimpleTaskRunner::WhileTaskExistedInitiallyCallback( nit: Add blank line. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:297: bool OrderedSimpleTaskRunner::WhileNowBeforeCallback(base::TimeTicks stop_at) { nit: add Blank line. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:68: static const size_t absolute_max_tasks; kAbsoluteMaxTasks https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:70: void ClearTimeout() { max_tasks_ = absolute_max_tasks; } ClearRunTaskLimit()? https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:88: bool RunTasks(const std::vector<base::Callback<bool(void)> > conditions); This should probably be a reference: const std::vector<base::Callback<bool(void)> >& conditions
Fixed Sami's comments. Can you see anything else major Sami? (Obviously will wait on Brian's comments before submitting...) Do you think we should rename it to OrderedTestRunner or something rather than "simple"? Tim https://codereview.chromium.org/387493002/diff/400001/cc/scheduler/scheduler_... File cc/scheduler/scheduler_unittest.cc (right): https://codereview.chromium.org/387493002/diff/400001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:87: base::Callback<bool(void)> WhileImplFrameDeadlinePending(bool state) { On 2014/09/01 11:35:32, Sami wrote: > I didn't see any calls to this with |state| = false, so I'd lean toward removing > the parameter for now. Line 107 uses false, most of the other calls use true. https://codereview.chromium.org/387493002/diff/400001/cc/scheduler/scheduler_... cc/scheduler/scheduler_unittest.cc:94: bool WhileImplFrameDeadlinePendingCallback(bool state) { On 2014/09/01 11:35:32, Sami wrote: > Could you move this under protected: (or private:) to make it more obvious that > it's an internal helper function? Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:130: if (pending_tasks_.size() <= 0) On 2014/09/01 11:35:33, Sami wrote: > nit: Add braces Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:140: if (pending_tasks_.size() <= 0) On 2014/09/01 11:35:33, Sami wrote: > nit: Add braces Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:176: modifiable_conditions.push_back(AdvanceNow()); On 2014/09/01 11:35:33, Sami wrote: > It's clever but having AdvanceNow() as a condition feels wrong, since always > returns true and is there just for the side-effect. How about inlining the code > in the run loop instead? I think it would be easier to read that way. Actually, come to think of it - there is actually a condition AdvanceNow should return false on, is kind of silly however? See the change... I do think conditions having side effects is probably a very useful thing to allow as it means you can use them to effect the running tasks? https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:190: goto run_tasks_exit; On 2014/09/01 11:35:32, Sami wrote: > I feel like I almost have to offer an alternative here :) How about wrapping > inside_run_tasks_until_ in a base::AutoReset and just returning here? Done. Never seen base::AutoReset before! LGTM. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:273: bool OrderedSimpleTaskRunner::WhileTaskRunCountBelowCallback( On 2014/09/01 11:35:33, Sami wrote: > nit: Add blank line. Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:286: bool OrderedSimpleTaskRunner::WhileTaskExistedInitiallyCallback( On 2014/09/01 11:35:33, Sami wrote: > nit: Add blank line. Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:297: bool OrderedSimpleTaskRunner::WhileNowBeforeCallback(base::TimeTicks stop_at) { On 2014/09/01 11:35:33, Sami wrote: > nit: add Blank line. Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:68: static const size_t absolute_max_tasks; On 2014/09/01 11:35:33, Sami wrote: > kAbsoluteMaxTasks Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:70: void ClearTimeout() { max_tasks_ = absolute_max_tasks; } On 2014/09/01 11:35:33, Sami wrote: > ClearRunTaskLimit()? Done. https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:88: bool RunTasks(const std::vector<base::Callback<bool(void)> > conditions); On 2014/09/01 11:35:33, Sami wrote: > This should probably be a reference: > > const std::vector<base::Callback<bool(void)> >& conditions Done.
https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:130: if (pending_tasks_.size() <= 0) On 2014/09/01 12:00:19, mithro wrote: > On 2014/09/01 11:35:33, Sami wrote: > > nit: Add braces > > Done. No need for them now but oh well :) https://codereview.chromium.org/387493002/diff/400001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:176: modifiable_conditions.push_back(AdvanceNow()); On 2014/09/01 12:00:19, mithro wrote: > On 2014/09/01 11:35:33, Sami wrote: > > It's clever but having AdvanceNow() as a condition feels wrong, since always > > returns true and is there just for the side-effect. How about inlining the > code > > in the run loop instead? I think it would be easier to read that way. > > Actually, come to think of it - there is actually a condition AdvanceNow should > return false on, is kind of silly however? See the change... > > I do think conditions having side effects is probably a very useful thing to > allow as it means you can use them to effect the running tasks? I'm fine with conditions having side effects, but the problem with AdvanceNow() is that it introduces an ordering constraint between conditions. It only really works if it's the last condition. That's why I still feel it would be better to just have three lines here: if (advance_now_ && now_src_->Now() < next_task_time) { now_src_->SetNow(next_task_time); } The special case you added could also be an assertion here.
https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:180: if (inside_run_tasks_until_) Should this early out and the AutoReset move up? https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:192: return pending_tasks_.size() > 0; Can you just return true here? https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:302: base::Callback<bool(void)> OrderedSimpleTaskRunner::AdvanceNow() { I would want to explictly disallow side-effects like this if we could, but I can't think of a way to enforce it either at compile time or run time - just review time. It's convenient that you can implement AdvanceNow like this, but it seems like an abuse of the condition argument, which I wouldn't expect to have any side effects when run. If we are going to allow side-effects, I would suggest renaming the condition argument to something different. https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:311: return false; This "return false" seems redundant with the existing pending_tasks_.size() > 0 check in RunTasks. Is there any reason to keep it? https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:82: bool RunTasks(base::Callback<bool(void)> condition); If we rename this to RunTasksWhile we could remove "While" from the functions that create the conditions and it will communicate better what this method does with the condition.
Will try and get to these ASAP. Is there anything else that needs to be done before this can get a LGTM? Tim 'mithro' Ansell On 5 Sep 2014 12:48, <brianderson@chromium.org> wrote: > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.cc > File cc/test/ordered_simple_task_runner.cc (right): > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.cc#newcode180 > cc/test/ordered_simple_task_runner.cc:180: if (inside_run_tasks_until_) > Should this early out and the AutoReset move up? > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.cc#newcode192 > cc/test/ordered_simple_task_runner.cc:192: return pending_tasks_.size() > >> 0; >> > Can you just return true here? > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.cc#newcode302 > cc/test/ordered_simple_task_runner.cc:302: base::Callback<bool(void)> > OrderedSimpleTaskRunner::AdvanceNow() { > I would want to explictly disallow side-effects like this if we could, > but I can't think of a way to enforce it either at compile time or run > time - just review time. > > It's convenient that you can implement AdvanceNow like this, but it > seems like an abuse of the condition argument, which I wouldn't expect > to have any side effects when run. If we are going to allow > side-effects, I would suggest renaming the condition argument to > something different. > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.cc#newcode311 > cc/test/ordered_simple_task_runner.cc:311: return false; > This "return false" seems redundant with the existing > pending_tasks_.size() > 0 check in RunTasks. Is there any reason to keep > it? > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.h > File cc/test/ordered_simple_task_runner.h (right): > > https://codereview.chromium.org/387493002/diff/440001/cc/ > test/ordered_simple_task_runner.h#newcode82 > cc/test/ordered_simple_task_runner.h:82: bool > RunTasks(base::Callback<bool(void)> condition); > If we rename this to RunTasksWhile we could remove "While" from the > functions that create the conditions and it will communicate better what > this method does with the condition. > > https://codereview.chromium.org/387493002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I had another look and I think looks good to go after the changes Brian suggested.
PTAL Tim https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:180: if (inside_run_tasks_until_) On 2014/09/05 02:48:03, brianderson wrote: > Should this early out and the AutoReset move up? Done. https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:192: return pending_tasks_.size() > 0; On 2014/09/05 02:48:03, brianderson wrote: > Can you just return true here? A condition could possibly modify the queue size and return false. Maybe a "FlushAndExit()" type condition? https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:302: base::Callback<bool(void)> OrderedSimpleTaskRunner::AdvanceNow() { On 2014/09/05 02:48:03, brianderson wrote: > I would want to explictly disallow side-effects like this if we could, but I > can't think of a way to enforce it either at compile time or run time - just > review time. We also need side-effects for the "WhileTaskRunCountBelow" and similar conditions. > > It's convenient that you can implement AdvanceNow like this, but it seems like > an abuse of the condition argument, which I wouldn't expect to have any side > effects when run. If we are going to allow side-effects, I would suggest > renaming the condition argument to something different. Very open to suggestions... https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:311: return false; On 2014/09/05 02:48:03, brianderson wrote: > This "return false" seems redundant with the existing pending_tasks_.size() > 0 > check in RunTasks. Is there any reason to keep it? Removed. https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.h (right): https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.h:82: bool RunTasks(base::Callback<bool(void)> condition); On 2014/09/05 02:48:03, brianderson wrote: > If we rename this to RunTasksWhile we could remove "While" from the functions > that create the conditions and it will communicate better what this method does > with the condition. Done.
Should be the last round of comments from me. https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:192: return pending_tasks_.size() > 0; On 2014/09/08 12:52:19, mithro wrote: > On 2014/09/05 02:48:03, brianderson wrote: > > Can you just return true here? > > A condition could possibly modify the queue size and return false. Maybe a > "FlushAndExit()" type condition? A comment explaining that a condition might modify the queue size is good enough. Is it possible that a callback empties the pending_tasks_? If so, would we need an additional check before running the first task, which might not exist? https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:302: base::Callback<bool(void)> OrderedSimpleTaskRunner::AdvanceNow() { On 2014/09/08 12:52:19, mithro wrote: > On 2014/09/05 02:48:03, brianderson wrote: > > I would want to explictly disallow side-effects like this if we could, but I > > can't think of a way to enforce it either at compile time or run time - just > > review time. > > We also need side-effects for the "WhileTaskRunCountBelow" and similar > conditions. > > > > > It's convenient that you can implement AdvanceNow like this, but it seems like > > an abuse of the condition argument, which I wouldn't expect to have any side > > effects when run. If we are going to allow side-effects, I would suggest > > renaming the condition argument to something different. > > Very open to suggestions... Argh, naming is hard. I can't think of any better name that isn't suuper long. You already make it clear in the RunTasksWhile comments that there's short-circuit evaluation; can you also put in a warning about side-effects? https://codereview.chromium.org/387493002/diff/460001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/460001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:279: bool OrderedSimpleTaskRunner::TaskRunCountBelowCallback(size_t max_tasks, Looks like this can be static.
https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:192: return pending_tasks_.size() > 0; On 2014/09/08 19:24:08, brianderson wrote: > On 2014/09/08 12:52:19, mithro wrote: > > On 2014/09/05 02:48:03, brianderson wrote: > > > Can you just return true here? > > > > A condition could possibly modify the queue size and return false. Maybe a > > "FlushAndExit()" type condition? > > A comment explaining that a condition might modify the queue size is good > enough. > > Is it possible that a callback empties the pending_tasks_? If so, would we need > an additional check before running the first task, which might not exist? Done. https://codereview.chromium.org/387493002/diff/440001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:302: base::Callback<bool(void)> OrderedSimpleTaskRunner::AdvanceNow() { On 2014/09/08 19:24:08, brianderson wrote: > On 2014/09/08 12:52:19, mithro wrote: > > On 2014/09/05 02:48:03, brianderson wrote: > > > I would want to explictly disallow side-effects like this if we could, but I > > > can't think of a way to enforce it either at compile time or run time - just > > > review time. > > > > We also need side-effects for the "WhileTaskRunCountBelow" and similar > > conditions. > > > > > > > > It's convenient that you can implement AdvanceNow like this, but it seems > like > > > an abuse of the condition argument, which I wouldn't expect to have any side > > > effects when run. If we are going to allow side-effects, I would suggest > > > renaming the condition argument to something different. > > > > Very open to suggestions... > > Argh, naming is hard. I can't think of any better name that isn't suuper long. > > You already make it clear in the RunTasksWhile comments that there's > short-circuit evaluation; can you also put in a warning about side-effects? Done. https://codereview.chromium.org/387493002/diff/460001/cc/test/ordered_simple_... File cc/test/ordered_simple_task_runner.cc (right): https://codereview.chromium.org/387493002/diff/460001/cc/test/ordered_simple_... cc/test/ordered_simple_task_runner.cc:279: bool OrderedSimpleTaskRunner::TaskRunCountBelowCallback(size_t max_tasks, On 2014/09/08 19:24:08, brianderson wrote: > Looks like this can be static. Done.
lgtm
The CQ bit was checked by mithro@mithis.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mithro@mithis.com/387493002/480001
Message was sent while issue was closed.
Committed patchset #24 (id:480001) as 2fa82c49e96927ff704f7c980d068a54418a508d
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/0c0ac6a1616778e3852e3f8b3248688ca9d899b2 Cr-Commit-Position: refs/heads/master@{#294059} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
