Introduce task_runner_checker, task_runner_weak_ptr by cyanglaz · Pull Request #17649 · flutter/engine

cyanglaz · 2020-04-10T23:19:54Z

Replace thread_checker with task_runner_checker, make the weakPtr to be accessed from different threads as long as they are on the same task_runner (thread merging)
flutter/flutter#52253

iskakaushik · 2020-04-27T17:40:38Z

fml/memory/task_runner_checker.h

+  }
+
+ private:
+  TaskQueueId self_;


I think self_ is an ambiguous name. Can this be called, initialized_queue_id_?

iskakaushik · 2020-04-27T17:41:11Z

fml/memory/task_runner_checker.h

+  ~TaskRunnerChecker() = default;
+
+  bool RunsOnCreationTaskRunner() const {
+    FML_DCHECK(fml::MessageLoop::IsInitializedForCurrentThread());


I think this can be a FML_CHECK.

makes sense, done!

iskakaushik · 2020-04-27T17:42:28Z

fml/memory/task_runner_checker.h

+    auto queues = MessageLoopTaskQueues::GetInstance();
+    if (queues->Owns(current_queue_id, self_)) {
+      return true;
+    }
+    if (queues->Owns(self_, current_queue_id)) {
+      return true;
+    }


Can you refactor to share code with TaskRunner::RunsTasksOnCurrentThread?

offline discussion, we will create a TaskRunnerWeakPtr to support the task runner checker

iskakaushik · 2020-04-27T17:45:05Z

fml/memory/task_runner_checker.h

+  }
+};
+
+#if !defined(NDEBUG)


shouldn't this condition be flipped? As it stands it looks like these checks will only be run when NDEBUG is not specified. I think we should rely on FLUTTER_RUNTIME_MODE macro instead and run it on DEBUG builds.

This is how the current Thread_Runner works. I'm not sure if we should change the behavior in this PR? Maybe we can do it in a separate PR to be more explicit after landing this?

googlebot · 2020-04-28T21:01:34Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

googlebot · 2020-04-28T21:01:34Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

cyanglaz · 2020-04-28T21:14:33Z

@googlebot I fixed it.

googlebot · 2020-04-28T21:19:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot · 2020-04-28T21:19:17Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

iskakaushik

LGTM

cyanglaz · 2020-04-27T18:26:54Z

fml/memory/task_runner_checker.h

+  }
+
+ private:
+  TaskQueueId self_;


cyanglaz · 2020-04-27T18:29:47Z

fml/memory/task_runner_checker.h

+  }
+};
+
+#if !defined(NDEBUG)


This is how the current Thread_Runner works. I'm not sure if we should change the behavior in this PR? Maybe we can do it in a separate PR to be more explicit after landing this?

cyanglaz · 2020-04-27T18:30:39Z

fml/memory/task_runner_checker.h

+  ~TaskRunnerChecker() = default;
+
+  bool RunsOnCreationTaskRunner() const {
+    FML_DCHECK(fml::MessageLoop::IsInitializedForCurrentThread());


makes sense, done!

cyanglaz · 2020-04-27T20:41:05Z

fml/memory/task_runner_checker.h

+    auto queues = MessageLoopTaskQueues::GetInstance();
+    if (queues->Owns(current_queue_id, self_)) {
+      return true;
+    }
+    if (queues->Owns(self_, current_queue_id)) {
+      return true;
+    }


offline discussion, we will create a TaskRunnerWeakPtr to support the task runner checker

cyanglaz · 2020-05-01T17:11:00Z

fml/memory/weak_ptr.h


  // Copy constructor.
-  WeakPtr(const WeakPtr<T>& r) = default;
+  explicit WeakPtr(const WeakPtr<T>& r) = default;


@iskakaushik Changed after your last approval:
I found implicit casting could be problematic as someone could want a TaskRunnerAffineWeakPtr but end up getting a WeakPtr.

cyanglaz · 2020-05-01T17:12:00Z

shell/common/shell.h

-  fml::WeakPtr<Engine> weak_engine_;          // to be shared across threads
-  fml::WeakPtr<Rasterizer> weak_rasterizer_;  // to be shared across threads
+  fml::WeakPtr<Engine> weak_engine_;  // to be shared across threads
+  fml::TaskRunnerAffineWeakPtr<Rasterizer>


@iskakaushik Changed after your last approval:
It was implicitly casted from a TaskRunnerAffineWeakPtr to a WeakPtr

…/engine#17649)

googlebot added the cla: yes label Apr 10, 2020

chinmaygarde self-requested a review April 10, 2020 23:50

cyanglaz changed the title ~~Task runner checker~~ Introduce task_runner_checker, use it on weakPtrs, replace the thread_checker Apr 13, 2020

cyanglaz requested a review from iskakaushik April 13, 2020 18:31

cyanglaz marked this pull request as ready for review April 13, 2020 18:31

auto-assign bot requested a review from jason-simmons April 13, 2020 18:33

iskakaushik suggested changes Apr 27, 2020

View reviewed changes

googlebot added cla: no and removed cla: yes labels Apr 28, 2020

cyanglaz changed the title ~~Introduce task_runner_checker, use it on weakPtrs, replace the thread_checker~~ Introduce task_runner_checker, task_runner_weak_ptr Apr 28, 2020

cyanglaz force-pushed the task_runner_checker branch from f109ddf to 145c604 Compare April 28, 2020 21:13

task_runner_checker

21c0bf6

cyanglaz force-pushed the task_runner_checker branch from 145c604 to 21c0bf6 Compare April 28, 2020 21:19

googlebot added cla: yes and removed cla: no labels Apr 28, 2020

cyanglaz added 2 commits April 29, 2020 16:56

rename and tests

7d9359b

fix license header

46b8aa8

iskakaushik approved these changes Apr 30, 2020

View reviewed changes

cyanglaz added 5 commits April 29, 2020 17:26

license fix

dd82047

Merge branch 'master' into task_runner_checker

a9c7423

rasterizer should be acessed as an TaskRunnerAffineWeakPtr

11bea26

formatting

b03a927

make weakPtr copy constructor explict to prevent implicit casting

5f10a5c

cyanglaz commented May 1, 2020

View reviewed changes

cyanglaz merged commit 2622cb9 into flutter:master May 1, 2020

cyanglaz deleted the task_runner_checker branch May 1, 2020 18:28

engine-flutter-autoroll mentioned this pull request May 1, 2020

Roll engine 2db327657369..2622cb9c3328 (35 commits) flutter/flutter#56140

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020

2622cb9 Introduce TaskRunnerChecker, TaskRunnerAffineWeakPtr (flutter…

d55c0da

…/engine#17649)

engine-flutter-autoroll mentioned this pull request May 1, 2020

Roll engine 2db327657369..b569260dd3e9 (36 commits) flutter/flutter#56142

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020

2622cb9 Introduce TaskRunnerChecker, TaskRunnerAffineWeakPtr (flutter…

77ebb5b

…/engine#17649)

engine-flutter-autoroll mentioned this pull request May 1, 2020

Roll engine 2db327657369..7a05fcbdbe52 (38 commits) flutter/flutter#56148

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020

2622cb9 Introduce TaskRunnerChecker, TaskRunnerAffineWeakPtr (flutter…

11b80c1

…/engine#17649)

engine-flutter-autoroll mentioned this pull request May 1, 2020

Roll engine 2db327657369..77d02719f523 (40 commits) flutter/flutter#56155

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 1, 2020

2622cb9 Introduce TaskRunnerChecker, TaskRunnerAffineWeakPtr (flutter…

8cc6876

…/engine#17649)

engine-flutter-autoroll mentioned this pull request May 2, 2020

Roll engine 2db327657369..72fe227a50d9 (42 commits) flutter/flutter#56162

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 2, 2020

2622cb9 Introduce TaskRunnerChecker, TaskRunnerAffineWeakPtr (flutter…

1807613

…/engine#17649)

engine-flutter-autoroll mentioned this pull request May 5, 2020

Roll engine 95ecd9a4050c..33d236795015 (11 commits) flutter/flutter#56378

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 5, 2020

2622cb9 Introduce TaskRunnerChecker, TaskRunnerAffineWeakPtr (flutter…

261db90

…/engine#17649)

cyanglaz mentioned this pull request May 13, 2020

Introduce TaskRunnerAffineWeakPtrFactory to generate TaskRunnerAffineWeakPtrs #18346

Merged

Conversation

cyanglaz commented Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Apr 28, 2020

Uh oh!

googlebot commented Apr 28, 2020

Uh oh!

cyanglaz commented Apr 28, 2020

Uh oh!

googlebot commented Apr 28, 2020

Uh oh!

googlebot commented Apr 28, 2020

Uh oh!

iskakaushik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cyanglaz commented Apr 10, 2020 •

edited

Loading