[go: nahoru, domu]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 10 commits into from
Oct 21, 2021
Merged

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

merged 10 commits into from
Oct 21, 2021

Conversation

moko256
Copy link
Member
@moko256 moko256 commented Aug 11, 2021 •
edited
Loading

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

Fixes: flutter/flutter#70890

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
Copy link

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.

@google-cla google-cla bot added the cla: yes label Aug 11, 2021
@chinmaygarde
Copy link
Member

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

@iskakaushik iskakaushik self-requested a review August 26, 2021 21:22
iskakaushik
iskakaushik suggested changes Aug 31, 2021
View reviewed changes
Copy link
Contributor
@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author
moko256 commented Sep 8, 2021

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
iskakaushik reviewed Oct 2, 2021
View reviewed changes
shell/platform/windows/task_runner.h Outdated
public:
class Delegate {
public:
virtual std::chrono::nanoseconds ProcessTasks() = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author
@moko256 moko256 Oct 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@moko256
Copy link
Member Author
moko256 commented Oct 2, 2021

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

@moko256
Copy link
Member Author
moko256 commented Oct 12, 2021

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
Copy link
Contributor

Could you please rebase on top of master and push?

@moko256
Copy link
Member Author
moko256 commented Oct 15, 2021

I rebased on ToT and fixed the conflicts.

@iskakaushik
Copy link
Contributor

could you also fix the licenses? :)

@moko256
Copy link
Member Author
moko256 commented Oct 15, 2021

I fixed them.

iskakaushik
iskakaushik approved these changes Oct 18, 2021
View reviewed changes
Copy link
Contributor
@iskakaushik iskakaushik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM cc: @cbracken

@cbracken
Copy link
Member

Thanks -- will give a quick pass through today!

@chinmaygarde
Copy link
Member

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

clarkezone
clarkezone approved these changes Oct 21, 2021
View reviewed changes
Copy link
Contributor
@clarkezone clarkezone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@iskakaushik 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 fluttergithubbot merged commit 9f20885 into flutter:master Oct 21, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 22, 2021
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2021
@engine-flutter-autoroll
9f20885 Fully inplement TaskRunner for UWP (flutter#70890) (flutter/e…
a51e67d 
…ngine#28013)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Oct 22, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Oct 22, 2021
@engine-flutter-autoroll
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
@moko256 @kylinchen
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
@moko256
cleanup flutter#28013
bb7c30b
@moko256 moko256 mentioned this pull request Nov 3, 2021
Merged
8 tasks
bdero pushed a commit that referenced this pull request Jan 19, 2022
@moko256
cleanup #28013 (#29500)
31b1a1d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iskakaushik iskakaushik iskakaushik approved these changes

@clarkezone clarkezone clarkezone approved these changes

@cbracken cbracken Awaiting requested review from cbracken

Assignees
No one assigned
Labels
cla: yes needs tests platform-windows waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Milestone
No milestone
6 participants
@moko256 @chinmaygarde @iskakaushik @cbracken @clarkezone @fluttergithubbot