[task] Use TaskEnvironment in blink platform tests [chromium/src : main]
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 10 comments![]( "Open in Gerrit")
PTAL
The CL is quite big, but mostly mechanical (I highlighted the subtleties).
All tests are independent, but let me know if I should break it down in multiple CLs.
task_environment_.AdvanceClock(
now.SnappedToNextTick(base::TimeTicks(), base::Hours(1)) - now);Whereas we previously set expectation relative to TimeTicks(), MOCK_TIME will give us a random start time.
This is needed to align time on (at least) 1h boundary, and expectations below are updated to be relative to start_time_.
void SimulateMainThreadInputHandlingCompositorTask(Unused
void SimulateMainThreadCompositor(base::TimeTicks frame_time,(this is the trickiest bit)
Previously: RunUntilIdle() (from caller) relies on TestMockTimeTaskRunner that would pump all immediate tasks. SimulateMainThreadCompositorTask does AdvanceClock(), but the TestMockTimeTaskRunner doesn't re-evaluate time.
TaskEnvironment will re-evaluate time between each task and run tasks that become ready after SimulateMainThreadCompositorTask's AdvanceClock.
Solution: all relates tests want to interleave MainThreadCompositor with other tasks and see what gets starve. Instead of relying on RunUntilIdle() subtleties, I've made this a PostDelayedTask with the next frame time, so that the scheduler gets a chance to run tasks in between.
base::RunLoop run_loop;
compositor_task_runner_->PostTask(
FROM_HERE, base::BindLambdaForTesting([&]() {
DoMainFrame();
SimulateMainThreadCompositorTask(base::Milliseconds(1000));
run_loop.Quit();
}));
run_loop.Run();This would previously rely on the fact that nothing runs between DoMainFrame() (from caller) and SimulateMainThreadCompositorTask(); this isn't always true, so I moved DoMainFrame() inside the task to make sure the scheduler always considers this task "main frame"
EXPECT_EQ(2u, count) << "i = " << i;This seems a consequence of RunUntilIdle() subtlety, but 1u is the desired behavior AFAICT.
EXPECT_EQ(279u, run_order.size());This seems a consequence of RunUntilIdle() subtlety, but 0u is the desired behavior AFAICT.
I couldn't find an explanation in history for 279.
base::test::TaskEnvironment::TimeSource::SYSTEM_TIME) {}SequenceManager doesn't support nesting TimeDomain (which both TaskEnvironment MOCK_TIME and EnableVirtualTime rely on).
But these tests don't actually need MOCK_TIME.
base::test::TaskEnvironment::ThreadingMode::DEFAULT) {}Ideally this would be ThreadingMode::MAIN_THREAD_ONLY, but Android uses the thread pool in SelfCompactionManager::OnRunningCompact() (this is true ToT)
EXPECT_THAT(tasks, ElementsAre(base::TimeTicks() + base::Seconds(1),The test fixture starts with TimeTicks() + 5ms, which shifts everything by 1s when aligning timers.
For simplicity I used aligned start_time_.
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
AI Code Reviewer (Gerrit)
AI Code Reviewer added 2 comments![]( "Open in Gerrit")
scheduler::NonMainThreadSchedulerHelper* non_main_thread_scheduler() {nit: Blink Style Guide: Naming - Use 'CamelCase' for all function names. Consider renaming this getter to 'NonMainThreadScheduler()' to adhere to Blink's getter naming conventions.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
MainThreadScheduler* main_thread_scheduler() const {nit: Blink Style Guide: Naming - Use 'CamelCase' for all function names. Since the bare word 'MainThreadScheduler' collides with the template type name, consider renaming this getter to 'GetMainThreadScheduler()'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 16 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
16 comments
Sweet! I thought this was coming some day. Just a few small things, but overall LGTM.
void SimulateMainThreadCompositor(base::TimeTicks frame_time,(this is the trickiest bit)
Previously: RunUntilIdle() (from caller) relies on TestMockTimeTaskRunner that would pump all immediate tasks. SimulateMainThreadCompositorTask does AdvanceClock(), but the TestMockTimeTaskRunner doesn't re-evaluate time.TaskEnvironment will re-evaluate time between each task and run tasks that become ready after SimulateMainThreadCompositorTask's AdvanceClock.
Solution: all relates tests want to interleave MainThreadCompositor with other tasks and see what gets starve. Instead of relying on RunUntilIdle() subtleties, I've made this a PostDelayedTask with the next frame time, so that the scheduler gets a chance to run tasks in between.
(opening for visibility, but I think this LGTM)
Yeah quite tricky! Some of the tests are explicitly testing starvation via priority, such that the compositor TQ doesn't get prioritized if rendering takes too long, in which case adding delay could be problematic -- but I think as long as those tests are immediately reposting (because next frame is expected immediately), this should be fine.
---
Aside: Some of these tests might be based on old, removed policies (e.g. IIRC we used to throttle timers based on policy, but now only pause certain queues if touchstart is pending -- everything else is priority), so I'm not sure they _all_ make sense anymore -- but some do.
Aside 2: I don't know why the related tests call WillBeginMainFrame and DidCommitFrameToCompositor in a separate task -- these are expected to happen in the same task.
Anyway, thanks for getting these working -- I may audit and clean these up further at some point, some day...
DoMainFrame();It's a little unfortunate that `DoMainFrame()` and `SimulateMainThreadCompositorTask()` both call `DidCommitFrameToCompositor()`. You could consider making `DoMainFrame()` take a duration (Default=0) and insert the delay between WillBeginFrame() and Commit() ?
std::optional<TaskEnvironmentForMainThreadSchedulerTest> task_environment_;Same question as other file: does this need to be optional<>, or can it be initialized in the c'tor initializer list?
EXPECT_EQ(279u, run_order.size());This seems a consequence of RunUntilIdle() subtlety, but 0u is the desired behavior AFAICT.
I couldn't find an explanation in history for 279.
Yeah that makes sense. Probably control tasks / policy updates or something.
std::optional<blink::test::TaskEnvironmentWithMainThreadScheduler>Why optional? It's set in both c'tors. Can it be instantiated in the c'tor initializer list?
base::test::TaskEnvironment::ThreadingMode::DEFAULT) {}Ideally this would be ThreadingMode::MAIN_THREAD_ONLY, but Android uses the thread pool in SelfCompactionManager::OnRunningCompact() (this is true ToT)
Worth adding a comment here?
EXPECT_LT(real_time - initial_real_time, TestTimeouts::tiny_timeout());TIL!
// The global tick clock has not moved, yet we ran a large number of "delayed"Is this somewhat outdated now? This still uses MOCK_TIME, but the IIUC the expectation below is now comparing actual elapsed wall time, so I'm not sure how much this comment applies anymore.
scheduler::NonMainThreadSchedulerHelper* non_main_thread_scheduler() {Maybe GetNonMainThreadSchedulerHelper() to match the type? That could make some call sites more readable since this isn't actually returning a NonMainThreadSchedulerBase.
// These classes instantiate specific scheduling environments that's not```suggestion
// These classes instantiate specific scheduling environments that are not
```
(or remove "that's")
class TaskEnvironmentWithNonMainThreadSchedulerShould this be `TaskEnvironmentWithNonMainThreadSchedulerHelper`? This isn't really a (full) NonMainThreadScheduler -- it's just part of one for tests that test/need that bit directly.
Alternatively, and maybe this is overkill, this could create an actual non-main thread scheduler and tests could get the Helper from it?
(Aside: FWIW I'd love to get rid of SchedulerHelper and merge it with `ThreadSchedulerBase` (crbug.com/40246056))
MainThreadScheduler* main_thread_scheduler() const {nit: Blink Style Guide: Naming - Use 'CamelCase' for all function names. Since the bare word 'MainThreadScheduler' collides with the template type name, consider renaming this getter to 'GetMainThreadScheduler()'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Could you change this? Blink actually uses CamelCase for everything, even simple accessors https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#Use-for-all-function-names
There are a bunch of violations (main_thread_only() etc.) of this in blink/scheduler because the code used to live in //components and was never fully cleaned up, but I'm hoping to get it to conform slowly and not introduce more.
WorkerThreadSchedulerForTest* thread_scheduler() { return scheduler_.get(); }style nit: (Get)ThreadScheduler()
EXPECT_THAT(tasks, ElementsAre(base::TimeTicks() + base::Seconds(1),The test fixture starts with TimeTicks() + 5ms, which shifts everything by 1s when aligning timers.
For simplicity I used aligned start_time_.
Nice.
WorkerThreadSchedulerForTest* thread_scheduler() { return scheduler_.get(); }style nit: (Get)ThreadScheduler()
Related details
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Etienne Pierre-Doray (Gerrit)
Etienne Pierre-Doray added 13 comments![]( "Open in Gerrit")
void SimulateMainThreadCompositor(base::TimeTicks frame_time,Scott Haseley(this is the trickiest bit)
Previously: RunUntilIdle() (from caller) relies on TestMockTimeTaskRunner that would pump all immediate tasks. SimulateMainThreadCompositorTask does AdvanceClock(), but the TestMockTimeTaskRunner doesn't re-evaluate time.TaskEnvironment will re-evaluate time between each task and run tasks that become ready after SimulateMainThreadCompositorTask's AdvanceClock.
Solution: all relates tests want to interleave MainThreadCompositor with other tasks and see what gets starve. Instead of relying on RunUntilIdle() subtleties, I've made this a PostDelayedTask with the next frame time, so that the scheduler gets a chance to run tasks in between.
(opening for visibility, but I think this LGTM)
Yeah quite tricky! Some of the tests are explicitly testing starvation via priority, such that the compositor TQ doesn't get prioritized if rendering takes too long, in which case adding delay could be problematic -- but I think as long as those tests are immediately reposting (because next frame is expected immediately), this should be fine.
---
Aside: Some of these tests might be based on old, removed policies (e.g. IIRC we used to throttle timers based on policy, but now only pause certain queues if touchstart is pending -- everything else is priority), so I'm not sure they _all_ make sense anymore -- but some do.
Aside 2: I don't know why the related tests call WillBeginMainFrame and DidCommitFrameToCompositor in a separate task -- these are expected to happen in the same task.
Anyway, thanks for getting these working -- I may audit and clean these up further at some point, some day...
in which case adding delay could be problematic
To be more precise, what SimulateMainThreadCompositor() now does: it ensures that the next simulated frame is no sooner than delay after the current frame.
- If the frame interval is 16ms, and the task takes a (simulated) 20ms, then the next frame will be immediate.
- If the frame interval is 16ms, and the task takes 10ms, then the next frame is posted 6ms later, giving the chance for something else to run in between.
I don't know why the related tests call WillBeginMainFrame and DidCommitFrameToCompositor in a separate task
Yeah this is all very fake (and relies on the fact that no task runs in between).
I could fold WillBeginFrame() into the same task, with some extra plumbing; I suspect DidHandleInputEventOnCompositorThread() should also go into the same task so I'd plumb its arguments as well. Thoughts?
DoMainFrame();It's a little unfortunate that `DoMainFrame()` and `SimulateMainThreadCompositorTask()` both call `DidCommitFrameToCompositor()`. You could consider making `DoMainFrame()` take a duration (Default=0) and insert the delay between WillBeginFrame() and Commit() ?
Done
std::optional<blink::test::TaskEnvironmentWithMainThreadScheduler>Why optional? It's set in both c'tors. Can it be instantiated in the c'tor initializer list?
This is the only way to initialize task_environment_ after feature list for now, but I sent https://crrev.com/c/7723407, if that works I'll remove optional here.
base::test::TaskEnvironment::ThreadingMode::DEFAULT) {}Scott HaseleyIdeally this would be ThreadingMode::MAIN_THREAD_ONLY, but Android uses the thread pool in SelfCompactionManager::OnRunningCompact() (this is true ToT)
Worth adding a comment here?
Done
// The global tick clock has not moved, yet we ran a large number of "delayed"Is this somewhat outdated now? This still uses MOCK_TIME, but the IIUC the expectation below is now comparing actual elapsed wall time, so I'm not sure how much this comment applies anymore.
Right, updated comment.
scheduler::NonMainThreadSchedulerHelper* non_main_thread_scheduler() {Maybe GetNonMainThreadSchedulerHelper() to match the type? That could make some call sites more readable since this isn't actually returning a NonMainThreadSchedulerBase.
Done
// These classes instantiate specific scheduling environments that's not```suggestion
// These classes instantiate specific scheduling environments that are not
```(or remove "that's")
Done
scheduler::NonMainThreadSchedulerHelper* non_main_thread_scheduler() {nit: Blink Style Guide: Naming - Use 'CamelCase' for all function names. Consider renaming this getter to 'NonMainThreadScheduler()' to adhere to Blink's getter naming conventions.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Done
class TaskEnvironmentWithNonMainThreadSchedulerShould this be `TaskEnvironmentWithNonMainThreadSchedulerHelper`? This isn't really a (full) NonMainThreadScheduler -- it's just part of one for tests that test/need that bit directly.
Alternatively, and maybe this is overkill, this could create an actual non-main thread scheduler and tests could get the Helper from it?
(Aside: FWIW I'd love to get rid of SchedulerHelper and merge it with `ThreadSchedulerBase` (crbug.com/40246056))
Done for TaskEnvironmentWithNonMainThreadSchedulerHelper.
I'd rather not switch to NonMainThreadScheduler though, since that would unnecessarily introduce more setup in tests.
MainThreadScheduler* main_thread_scheduler() const {Scott Haseleynit: Blink Style Guide: Naming - Use 'CamelCase' for all function names. Since the bare word 'MainThreadScheduler' collides with the template type name, consider renaming this getter to 'GetMainThreadScheduler()'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your 🙏 feedback 🙏 to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Could you change this? Blink actually uses CamelCase for everything, even simple accessors https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/blink-c++.md#Use-for-all-function-names
There are a bunch of violations (main_thread_only() etc.) of this in blink/scheduler because the code used to live in //components and was never fully cleaned up, but I'm hoping to get it to conform slowly and not introduce more.
Done
WorkerThreadSchedulerForTest* thread_scheduler() { return scheduler_.get(); }Etienne Pierre-Doraystyle nit: (Get)ThreadScheduler()
Done
WorkerThreadSchedulerForTest* thread_scheduler() { return scheduler_.get(); }Etienne Pierre-Doraystyle nit: (Get)ThreadScheduler()
Done
Related details
- Scott Haseley
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Scott Haseley (Gerrit)
Scott Haseley voted and added 3 comments![]( "Open in Gerrit")
Votes added by Scott Haseley
| Code-Review | +1 |
3 comments
LGTM
void SimulateMainThreadCompositor(base::TimeTicks frame_time,Scott Haseley(this is the trickiest bit)
Previously: RunUntilIdle() (from caller) relies on TestMockTimeTaskRunner that would pump all immediate tasks. SimulateMainThreadCompositorTask does AdvanceClock(), but the TestMockTimeTaskRunner doesn't re-evaluate time.TaskEnvironment will re-evaluate time between each task and run tasks that become ready after SimulateMainThreadCompositorTask's AdvanceClock.
Solution: all relates tests want to interleave MainThreadCompositor with other tasks and see what gets starve. Instead of relying on RunUntilIdle() subtleties, I've made this a PostDelayedTask with the next frame time, so that the scheduler gets a chance to run tasks in between.
Etienne Pierre-Doray(opening for visibility, but I think this LGTM)
Yeah quite tricky! Some of the tests are explicitly testing starvation via priority, such that the compositor TQ doesn't get prioritized if rendering takes too long, in which case adding delay could be problematic -- but I think as long as those tests are immediately reposting (because next frame is expected immediately), this should be fine.
---
Aside: Some of these tests might be based on old, removed policies (e.g. IIRC we used to throttle timers based on policy, but now only pause certain queues if touchstart is pending -- everything else is priority), so I'm not sure they _all_ make sense anymore -- but some do.
Aside 2: I don't know why the related tests call WillBeginMainFrame and DidCommitFrameToCompositor in a separate task -- these are expected to happen in the same task.
Anyway, thanks for getting these working -- I may audit and clean these up further at some point, some day...
in which case adding delay could be problematic
To be more precise, what SimulateMainThreadCompositor() now does: it ensures that the next simulated frame is no sooner than delay after the current frame.
- If the frame interval is 16ms, and the task takes a (simulated) 20ms, then the next frame will be immediate.
- If the frame interval is 16ms, and the task takes 10ms, then the next frame is posted 6ms later, giving the chance for something else to run in between.
I don't know why the related tests call WillBeginMainFrame and DidCommitFrameToCompositor in a separate task
Yeah this is all very fake (and relies on the fact that no task runs in between).
I could fold WillBeginFrame() into the same task, with some extra plumbing; I suspect DidHandleInputEventOnCompositorThread() should also go into the same task so I'd plumb its arguments as well. Thoughts?
If the frame interval is 16ms, and the task takes 10ms, then the next frame is posted 6ms later, giving the chance for something else to run in between.
This is the one that I was a bit concerned about because if the test is based purely on priority (i.e. testing that the compositor TQ stays at normal priority, which matches the throttleable queue), then the tests could pass even if the priority does get increased because the high priority task is delayed.
But looking at the couple tests that use 10ms, this looks fine.
I suspect DidHandleInputEventOnCompositorThread() should also go into the same task so I'd plumb its arguments as well
Typically DidHandleInputEventOnCompositorThread would be a different task because in reality this happens on the CC thread.
---
I think just getting these to work any way for now is fine, and this approach LGTM.
If I have some spare cycles soon I might audit some of these tests. Some other things I don't like is how, e.g. SYNCHRONIZED_GESTURE_ThrottleableTaskThrottling_task_not_expensive, mixes the main frame bits with pausing the scheduler. I think this is a remnant of an older approach where we did more throttling based on use case, and then stuck the Pauser in there -- which is totally different (pause during nested run loops, etc.).
std::optional<blink::test::TaskEnvironmentWithMainThreadScheduler>Etienne Pierre-DorayWhy optional? It's set in both c'tors. Can it be instantiated in the c'tor initializer list?
This is the only way to initialize task_environment_ after feature list for now, but I sent https://crrev.com/c/7723407, if that works I'll remove optional here.
- Etienne Pierre-Doray
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |