Add the functionality to merge and unmerge MessageLoopTaskQueues by iskakaushik · Pull Request #9436 · flutter/engine

iskakaushik · 2019-06-21T23:33:27Z

This introduces a notion of a "owning" and "subsumed" queue ids.
Owning queue will take care of the tasks submitted to both that and it's
subsumed queue.

The tasks submitted still maintain the queue affinity
Same for the task observers
Also adds MergedQueuesRunner which grabs both the locks owner
and subsumed queues in RAII fashion.

This introduces a notion of a "owning" and "subsumed" queue ids. Owning queue will take care of the tasks submitted to both that and it's subsumed queue. - The tasks submitted still maintain the queue affinity - Same for the task observers - Also adds MergedQueuesRunner which grabs both the locks owner and subsumed queues in RAII fashion.

- Also use task queue id to verify if we are running in the same thread. - This is to enable merging the backed message loop task queues to enable dynamic thread merging in IOS.

…ged_lock

…-on-same-thread

chinmaygarde · 2019-07-12T00:09:57Z

.gitignore

 .idea
 pubspec.lock
 .vscode/
+compile_commands.json


Compile commands should only end up in out/. This should not be necessary.

chinmaygarde · 2019-07-12T00:10:34Z

fml/merged_queues_runner.cc

+ public:
+  // TODO (kaushikiska): refactor mutexes out side of MessageLoopTaskQueues
+  // for better DI.
+  MergedQueuesRunner(MessageLoopTaskQueues* task_queues,


Since the queue can't ever be nullptr, use a reference for clarity.

chinmaygarde · 2019-07-12T00:25:46Z

fml/message_loop_task_queues.h

  std::vector<TaskObservers> task_observers_;
  std::vector<DelayedTaskQueue> delayed_tasks_;

+  static const size_t _kUnmerged = ULONG_MAX;


Lets typedef size_t

Moved this to a struct

chinmaygarde · 2019-07-12T00:38:42Z

fml/message_loop_task_queues.cc

+  std::mutex& t1 = GetMutex(owner, MutexType::kTasks);
+  std::mutex& t2 = GetMutex(subsumed, MutexType::kTasks);
+
+  std::scoped_lock(o1, o2, t1, t2);


This does not actually perform the lock. Please use a variable here.

Apparently I also missed the scoped_lock in the other review. Just global search and replace this pattern in the codebase.

chinmaygarde · 2019-07-12T00:38:48Z

fml/message_loop_task_queues.cc

+  // delayed_tasks locks
+  std::mutex& t = GetMutex(owner, MutexType::kTasks);
+
+  std::scoped_lock(o, t);


This does not actually perform the lock. Please use a variable here.

chinmaygarde · 2019-07-12T00:39:39Z

fml/message_loop_task_queues.cc

+  std::mutex& o = GetMutex(owner, MutexType::kObservers);
+
+  // delayed_tasks locks
+  std::mutex& t = GetMutex(owner, MutexType::kTasks);


Not acquiring the subsumed lock will cause unsafe access to the tasks/observers heap in HasPendingTasksUnlocked(subsumed). Just acquire all four locks.

chinmaygarde · 2019-07-12T00:43:03Z

fml/message_loop_task_queues.cc

    invocations.emplace_back(std::move(top.GetTask()));
-    tasks.pop();
+    delayed_tasks_[top_queue].pop();
    if (type == FlushType::kSingle) {


We no longer make use of this in the concurrent tasks queue. So lets just get rid of flush type with the default being multiple.

Created this flutter/flutter#36083, will follow this up.

chinmaygarde · 2019-07-12T00:45:50Z

fml/merged_queues_runner.cc

+  }
+
+  const TaskQueueId owner_;
+  TaskQueueId subsumed_;


Lets make task queue ID a struct with a single member of size_t.

chinmaygarde · 2019-07-12T00:48:29Z

The approach looks good. Have highlighted the thread safety issue and some other improvements.

…ged_lock

chinmaygarde

LGTM with the one nit.

chinmaygarde · 2019-07-12T23:22:06Z

fml/message_loop_task_queues.h

+  operator int() const { return value_; }
+
+ private:
+  size_t value_;


Zero it out to kUnused here by default.

…ues (flutter/engine#9436)

flutter/engine@919e353...76a91d3 git log 919e353..76a91d3 --no-merges --oneline 76a91d3 Roll src/third_party/skia 8590026dbf0d..563d044c0e76 (38 commits) (flutter/engine#9824) 8143845 Roll fuchsia/sdk/core/mac-amd64 from olF8ZjlM3lWSmWq8XyfXTKoPveuZtTcrGJINk_ERHhwC to pU_n3ahOhH6HJhE4vs5pUR8cg5U22TItP_Ds_N2OXPIC (flutter/engine#9820) 379028a Add the functionality to merge and unmerge MessageLoopTaskQueues (flutter/engine#9436) 8abe85b Revert "Roll src/third_party/dart 24725a8559..28f95fcd24 (32 commits)" (flutter/engine#9817) 96746bc Roll src/third_party/skia c3f28e3cf0d4..8590026dbf0d (2 commits) (flutter/engine#9805) 3652a68 Roll fuchsia/sdk/core/mac-amd64 from CDbRdGJ3bu-aWMCZqN5VzfQqIBwDGL2wfFodWABKdCIC to olF8ZjlM3lWSmWq8XyfXTKoPveuZtTcrGJINk_ERHhwC (flutter/engine#9803) 67d284d Roll src/third_party/dart 24725a8559..28f95fcd24 (32 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

flutter/engine@919e353...76a91d3 git log 919e353..76a91d3 --no-merges --oneline 76a91d3 Roll src/third_party/skia 8590026dbf0d..563d044c0e76 (38 commits) (flutter/engine#9824) 8143845 Roll fuchsia/sdk/core/mac-amd64 from olF8ZjlM3lWSmWq8XyfXTKoPveuZtTcrGJINk_ERHhwC to pU_n3ahOhH6HJhE4vs5pUR8cg5U22TItP_Ds_N2OXPIC (flutter/engine#9820) 379028a Add the functionality to merge and unmerge MessageLoopTaskQueues (flutter/engine#9436) 8abe85b Revert &flutter#34;Roll src/third_party/dart 24725a8559..28f95fcd24 (32 commits)&flutter#34; (flutter/engine#9817) 96746bc Roll src/third_party/skia c3f28e3cf0d4..8590026dbf0d (2 commits) (flutter/engine#9805) 3652a68 Roll fuchsia/sdk/core/mac-amd64 from CDbRdGJ3bu-aWMCZqN5VzfQqIBwDGL2wfFodWABKdCIC to olF8ZjlM3lWSmWq8XyfXTKoPveuZtTcrGJINk_ERHhwC (flutter/engine#9803) 67d284d Roll src/third_party/dart 24725a8559..28f95fcd24 (32 commits) The AutoRoll server is located here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+/master/autoroll/README.md If the roll is causing failures, please contact the current sheriff ([email protected]), and stop the roller if necessary.

iskakaushik requested a review from chinmaygarde June 21, 2019 23:33

googlebot added the cla: yes label Jun 21, 2019

fix licenses

3edf1a9

cbracken mentioned this pull request Jun 24, 2019

iOS Keyboard Lags flutter/flutter#31138

Closed

iskakaushik mentioned this pull request Jul 3, 2019

Pipeline allows continuations that can produce to front #9652

Merged

Kaushik Iska and others added 7 commits July 3, 2019 10:35

Expose TaskQueueId from TaskRunner interface

3930258

- Also use task queue id to verify if we are running in the same thread. - This is to enable merging the backed message loop task queues to enable dynamic thread merging in IOS.

Merge branch 'master' into introduce_merged_lock

06b7c06

Merge branch 'master' of github.com:flutter/engine into introduce_mer…

a5087fe

…ged_lock

Fix wake-up and also add a method to check Owns

a33cb8e

Merge branch 'master' of github.com:flutter/engine into check-if-runs…

d1492d3

…-on-same-thread

fix merge conflicts for realz

997cbfc

Merge branch 'check-if-runs-on-same-thread' into introduce_merged_lock

f341bec

chinmaygarde reviewed Jul 12, 2019

View reviewed changes

Kaushik Iska added 4 commits July 12, 2019 11:05

Fix scope of scoped locks and make queue_id a struct

b897c5a

Merge branch 'master' of github.com:flutter/engine into introduce_mer…

608a9ee

…ged_lock

fix benchmarks

c9d4e82

fix some ownership semantics

926e9c2

chinmaygarde approved these changes Jul 12, 2019

View reviewed changes

Zero out TaskQueueId

3b96103

iskakaushik merged commit 379028a into flutter:master Jul 12, 2019

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 15, 2019

379028a Add the functionality to merge and unmerge MessageLoopTaskQue…

4b612eb

…ues (flutter/engine#9436)

engine-flutter-autoroll mentioned this pull request Jul 15, 2019

Roll engine 919e3532368f..76a91d39b4b2 (7 commits) flutter/flutter#36203

Merged

TahaTesser mentioned this pull request May 11, 2021

Get rid of FlushType::kSingle (we don't need to enum even) flutter/flutter#36083

Closed

Conversation

iskakaushik commented Jun 21, 2019

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!

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!

Choose a reason for hiding this comment

Uh oh!

chinmaygarde commented Jul 12, 2019

Uh oh!

chinmaygarde left a comment

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