Fully inplement TaskRunner for UWP (flutter/flutter#70890) #28013

moko256 · 2021-08-11T08:20:46Z

This PR adds an implementation to process tasks' target time to TaskRunner for UWP.

No changes in flutter/tests.

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

flutter-dashboard · 2021-08-11T08:20:48Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

chinmaygarde · 2021-08-26T20:19:05Z

I am not familiar with Windows APIs but creating a timer object per task seems extremely heavyweight. cc @iskakaushik for a detailed review.

iskakaushik

I have to agree with Chinmay here. Creating a timer for each task seems heavy-weight. Can we do something similar to what is being done on other platforms? We only need to keep track of the wake-up time per task, and wake up at the next-top-time. See: task_runner_win32.cc for some ideas.

moko256 · 2021-09-08T01:23:34Z

Sorry for the delay. I found that by using DispatcherQueue, I am able to reuse timer, and make almost all the code for TaskRunner the same between win32 and winuwp, so I moved task queueing behavior from TaskRunnerWin32 to TaskRunner, and create abstract TaskRunnerTimer for win32/winuwp.

chinmaygarde · 2021-09-16T20:15:24Z

cc @iskakaushik Are you familiar with these APIs? If not, I am comfortable reviewing the embedder API bits but it will take me longer to lookup the Windows stuff.

iskakaushik · 2021-09-20T11:10:08Z

Sorry, I was out of the office last week. This is in my review queue, will be able to get to this in the next couple of days.

iskakaushik · 2021-10-02T15:37:49Z

The implementation itself looks solid. Other than the 1 comment I left, Would it be possible to add some unit tests for these task runner implementations to ensure the tasks are executed in the right order? These could be in task_runner_unittests.cc (which needs to be created.)

Thanks

iskakaushik · 2021-10-02T15:10:56Z

shell/platform/windows/task_runner.h

+ public:
+  class Delegate {
+   public:
+    virtual std::chrono::nanoseconds ProcessTasks() = 0;


Please add some docs for this method and the other interface methods below. Also any reason for creating this base class rather than have these virtual methods exist on TaskRunner itself? As it stands right now, TaskRunnerTimer (which reads like a timer), has methods which aren't typically on timers, e.g. WakeUp and RunsOnCurrentThread. If these methods were to be on TaskRunner base class itself, and have TaskRunnerWin32 and TaskRunnerWinUwp implement this, it would make more sense IMO.

I'll add the docs soon.
I wanted to clarify the callback in TaskRunnerTimer, but I also agree that it seems not like timer.

moko256 · 2021-10-02T17:00:23Z

Thanks for your reviewing. I'll add the test.

moko256 · 2021-10-12T23:48:19Z

I made TaskRunner the base class because of easier mocking, and wrote the test to check the executed task order, and to check the task won't execute before deadline.

iskakaushik · 2021-10-15T19:22:08Z

Could you please rebase on top of master and push?

moko256 · 2021-10-15T19:33:39Z

I rebased on ToT and fixed the conflicts.

iskakaushik · 2021-10-15T19:39:29Z

could you also fix the licenses? :)

moko256 · 2021-10-15T20:58:51Z

I fixed them.

iskakaushik

LGTM cc: @cbracken

cbracken · 2021-10-18T22:07:49Z

Thanks -- will give a quick pass through today!

chinmaygarde · 2021-10-21T20:07:43Z

@cbracken: Should we land this right away? It looks like this has Kaushik's LGTM.

clarkezone

LGTM

…ngine#28013)

…8013)

flutter-dashboard bot added the platform-windows label Aug 11, 2021

flutter-dashboard bot added the needs tests label Aug 11, 2021

google-cla bot added the cla: yes label Aug 11, 2021

iskakaushik self-requested a review August 26, 2021 21:22

iskakaushik suggested changes Aug 31, 2021

View reviewed changes

moko256 force-pushed the moko256_uwp_task_runner branch from dce758a to 6c2ca13 Compare September 8, 2021 01:08

chinmaygarde requested a review from iskakaushik September 16, 2021 20:14

iskakaushik reviewed Oct 2, 2021

View reviewed changes

moko256 added 9 commits October 16, 2021 04:31

Fully inplement TaskRunner for UWP

88489bc

implement TaskRunner with DispatcherQueue for UWP

bed8757

add and update documenets

3d85b13

use extending insead of delegating

50f441e

add test for task_runner

e17de5a

remove redundant semicolons

1ccb1a3

meaningful names in task_runner_unittests

b4e4cdc

apply filename for the name for include guard in task_runner_winuwp.h

dad8d75

convert CRLF to LF in task_runner_winuwp

3cd8493

moko256 force-pushed the moko256_uwp_task_runner branch from 0498428 to 3cd8493 Compare October 15, 2021 19:31

moko256 force-pushed the moko256_uwp_task_runner branch from af355f6 to aadfef5 Compare October 15, 2021 20:17

add files in task_runner to licenses

665b8f7

moko256 force-pushed the moko256_uwp_task_runner branch from aadfef5 to 665b8f7 Compare October 15, 2021 20:17

iskakaushik approved these changes Oct 18, 2021

View reviewed changes

cbracken requested review from cbracken and clarkezone October 18, 2021 22:07

clarkezone approved these changes Oct 21, 2021

View reviewed changes

iskakaushik added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Oct 21, 2021

fluttergithubbot merged commit 9f20885 into flutter:master Oct 21, 2021

engine-flutter-autoroll mentioned this pull request Oct 22, 2021

Roll Engine from d8a54333e57b to e898106f5306 (10 revisions) flutter/flutter#92308

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2021

9f20885 Fully inplement TaskRunner for UWP (flutter#70890) (flutter/e…

a51e67d

…ngine#28013)

engine-flutter-autoroll mentioned this pull request Oct 22, 2021

Roll Engine from d8a54333e57b to e898106f5306 (10 revisions) flutter/flutter#92311

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2021

9f20885 Fully inplement TaskRunner for UWP (flutter#70890) (flutter/e…

3be56e0

…ngine#28013)

kylinchen pushed a commit to XianyuTech/engine that referenced this pull request Oct 22, 2021

Fully inplement TaskRunner for UWP (flutter/flutter#70890) (flutter#2…

e5e85ed

…8013)

moko256 added a commit to moko256/flutter_engine that referenced this pull request Nov 3, 2021

cleanup flutter#28013

bb7c30b

moko256 mentioned this pull request Nov 3, 2021

Windows: Make task_runner_unittests.cc more deterministic. #29500

Merged

8 tasks

bdero pushed a commit that referenced this pull request Jan 19, 2022

cleanup #28013 (#29500)

31b1a1d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully inplement TaskRunner for UWP (flutter/flutter#70890) #28013

Fully inplement TaskRunner for UWP (flutter/flutter#70890) #28013

moko256 commented Aug 11, 2021 •

edited

Loading

flutter-dashboard bot commented Aug 11, 2021

chinmaygarde commented Aug 26, 2021

iskakaushik left a comment

moko256 commented Sep 8, 2021

chinmaygarde commented Sep 16, 2021

iskakaushik commented Sep 20, 2021

iskakaushik commented Oct 2, 2021

iskakaushik Oct 2, 2021

moko256 Oct 2, 2021 •

edited

Loading

moko256 Oct 12, 2021

moko256 commented Oct 2, 2021

moko256 commented Oct 12, 2021

iskakaushik commented Oct 15, 2021

moko256 commented Oct 15, 2021

iskakaushik commented Oct 15, 2021

moko256 commented Oct 15, 2021

iskakaushik left a comment

cbracken commented Oct 18, 2021

chinmaygarde commented Oct 21, 2021

clarkezone left a comment

Fully inplement TaskRunner for UWP (flutter/flutter#70890) #28013

Fully inplement TaskRunner for UWP (flutter/flutter#70890) #28013

Conversation

moko256 commented Aug 11, 2021 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Aug 11, 2021

chinmaygarde commented Aug 26, 2021

iskakaushik left a comment

Choose a reason for hiding this comment

moko256 commented Sep 8, 2021

chinmaygarde commented Sep 16, 2021

iskakaushik commented Sep 20, 2021

iskakaushik commented Oct 2, 2021

iskakaushik Oct 2, 2021

Choose a reason for hiding this comment

moko256 Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

moko256 Oct 12, 2021

Choose a reason for hiding this comment

moko256 commented Oct 2, 2021

moko256 commented Oct 12, 2021

iskakaushik commented Oct 15, 2021

moko256 commented Oct 15, 2021

iskakaushik commented Oct 15, 2021

moko256 commented Oct 15, 2021

iskakaushik left a comment

Choose a reason for hiding this comment

cbracken commented Oct 18, 2021

chinmaygarde commented Oct 21, 2021

clarkezone left a comment

Choose a reason for hiding this comment

moko256 commented Aug 11, 2021 •

edited

Loading

moko256 Oct 2, 2021 •

edited

Loading