Define thread priority enum and set thread priority for all threads in Engine #30605

JsouLiang · 2021-12-31T02:32:31Z

Refactoring the fml::Thread, flutter::ThreadHost which the aim is can set the Thread Priority.
First: define the ThreadConfig which hold the thread priority and thread name and we can add another thread priorities in this class.
Second: fml::Thread can create by ThreadConfig, we can set the thread name by ThreadConfig, for example:
std::make_unique<fml::Thread>(ThreadConfig("com.flutter.ui", Thread::ThreadPriority::DISPLAY));. In std::thread call-back we set the priorities:

thread_ = std::make_unique<std::thread>([&]() -> void {
    thread_config_->SetCurrentThreadName();
    thread_config_->SetCurrentThreadPriority();
    ...
  });

Third: : Cause to set the thread priority, we may be use the platform-api, so we can inherit the ThreadConfig in iOS or Android, and override the ThreadConfig::SetCurrentThreadPriority(ThreadConfig::SetCurrentThreadPriority is do nothing)

class PlatformIOSThreadConfig {
   void SetCurrentThreadPriority() {
     // set thread priority by QOS
   }
};

Finally: Cause of all the flutter thread are created by the flutter::ThreadHost, so i modify the flutter::ThreadHost constructor in order to i can through the ThreadConfig from platform-engine

/// FlutterEngine.mm
std::make_shared<flutter::ThreadHost>("com.flutter", uint_64, (ThreadHostConfig){
.ui_config = std::make_unique<PlatformIOSThreadConfig>("com.flutter.ui", Thread::Priority::DISPLAY),
.io_config = std::make_unique<PlatformIOSThreadConfig>("com.flutter.io", Thread::Priority::BACKGROUND),
.raster_config = std::make_unique<PlatformIOSThreadConfig>("com.flutter.io", Thread::Priority::RASTER),
}

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.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

flutter-dashboard · 2021-12-31T02:32:33Z

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 · 2022-01-13T21:10:53Z

I think this patch makes a lot of sense. I'll take a stab at reviewing it. A couple of suggestions though:

Can you separate the change where you introduced Thread::Priority enum and associated API updates into it own patch. In a separate patch, you can actually set the priorities on iOS. That way, if we have to revert the patch due regressions on specific platform, we won't have to revert everything.
Can you provide numbers on the improvements observed on the platforms where the priorities have been updated (iOS).

JsouLiang · 2022-01-14T02:35:51Z

Thank you reviewed;
Let me try to understand what you mean:Can you separate the change where you introduced Thread::Priority enum and associated API updates into it own patch. Do you mean i need a independent patch which only add Thread::Priority enum and associated API . At the seam time I need another patch to set the platform thread priorities?

JsouLiang · 2022-01-14T08:41:20Z

About the number of priority, I create two trace file in here: https://github.com/JsouLiang/ThreadPriorityTrace, you can use Xcode instrument to see.
In a words, if not improve UI and Raster thread priority, them would be preempted by IO thread, and the total preempted number is: 979

Then if i improve the UI thread priority, the total preempted number is: 142, and no IO thread break UI thread

JsouLiang · 2022-01-24T13:08:27Z

@chinmaygarde hey, the pr has any progress?

zanderso · 2022-02-02T07:24:58Z

@chinmaygarde is a bit overloaded, so asking @dnfield and @gaaclarke to review. /cc @iskakaushik FYI and to review if needed.

gaaclarke

Just some quick notes. I'll finish the review later.

fml/thread.h

gaaclarke · 2022-02-02T21:37:31Z

fml/thread.h

+    const std::string& thread_name() const { return thread_name_; }
+
+    /// Set current thread name
+    virtual void SetCurrentThreadName();


Why is this virtual? Maybe instead of making this a class, you want it to be an interface?

Yes, I think the method in the ThreadConfigure class user can have their own implementation by the user.

fml/thread.h

gaaclarke · 2022-02-02T21:38:39Z

fml/thread.h

@@ -28,6 +74,8 @@ class Thread {

 private:
  std::unique_ptr<std::thread> thread_;
+  /// ThreadConfigure is used for setting thread configure some like `priority`
+  std::unique_ptr<ThreadConfig> thread_config_;


There isn't a need to hold onto this variable is there?

The ThreadConfig set Thread is in the Thread callback, If don't hold it , it may be released when use it.

But it's only used in the constructor.

(the lambda can just take ownership of it)

fml/thread_unittests.cc

gaaclarke

Thanks, this is looking better. You didn't have to do the functor refactor but I appreciate it. If you want to clean it up further, using an std::function would be even cleaner.

gaaclarke · 2022-02-09T17:37:27Z

shell/platform/android/android_shell_holder.cc

+  auto mask =
+      ThreadHost::Type::UI | ThreadHost::Type::RASTER | ThreadHost::Type::IO;
+
+  PlatformAndroidThreadConfigSetter android_thread_config_setter;


This variable isn't used.

gaaclarke · 2022-02-09T17:41:40Z

fml/thread.h

+    ThreadPriority priority;
+  };
+
+  class ThreadConfigSetter {


Please add a docstring.

gaaclarke · 2022-02-09T17:47:10Z

shell/common/thread_host.h

+    explicit ThreadHostConfig(
+        const std::string& name_prefix,
+        uint64_t mask,
+        std::shared_ptr<ThreadConfigSetter> setter =


The constructors should actually take in a unique_ptr of the ThreadConfigSetter and graduate it to a shared_ptr.

gaaclarke · 2022-02-09T17:48:19Z

shell/common/thread_host.h

+        : ThreadHostConfig("", mask, setter) {}
+
+    /// Check if need to create thread.
+    bool isNeedThread(Type type) const { return type_mask & type; }


Suggested change

bool isNeedThread(Type type) const { return type_mask & type; }

bool isThreadNeeded(Type type) const { return type_mask & type; }

gaaclarke · 2022-02-09T17:55:43Z

fml/thread.h

+    ThreadPriority priority;
+  };
+
+  class ThreadConfigSetter {


nit: Now that you've factored this out, it would actually be cleaner as a std::function instead of a functor. That would eliminate your need for shared_ptr.

JsouLiang · 2022-02-10T06:49:36Z

@gaaclarke hey, listen to your suggestions, I change the ThreadConfigSetter to be a std::function. Please give me a review again. Thanks!🤟

gaaclarke

Hey awesome. I like the looks of this now. I hope you feel it's an improvement too. We've split apart the idea of the data versus the operation to set the data which better highlights that that is what is different between each platform. We didn't have to muck around with virtual functions or inheritance. Thanks buddy!

zanderso

/cc @iskakaushik apropos of our discussion yesterday.

shell/common/thread_host.h

fluttergithubbot · 2022-02-10T17:25:03Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

fluttergithubbot · 2022-02-10T21:20:13Z

This pull request is not suitable for automatic merging in its current state.

Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

JsouLiang · 2022-02-11T02:23:56Z

@gaaclarke I think your suggestion is very correct, thank you.

…hreads in Engine (flutter/engine#30605)

* 2a8f388 Roll Skia from a424b619bc40 to ec0af1664478 (6 revisions) (flutter/engine#31352) * ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (flutter/engine#31291) * 1eafb40 Use H5vcc CanvasKit implementation if it is detected. (flutter/engine#31191) * 6d99e90 Roll Dart SDK from 55c93c732da9 to b7eea441d7d1 (4 revisions) (flutter/engine#31353) * 935b46a Roll Fuchsia Mac SDK from 5_CZ81mTD... to 5CmPcHTb1... (flutter/engine#31356) * 244bb25 Roll Skia from ec0af1664478 to 9cb74e90792d (4 revisions) (flutter/engine#31357) * b6b5759 Roll Skia from 9cb74e90792d to 81d4b5d5b45d (6 revisions) (flutter/engine#31360) * 0f6db11 Roll Skia from 81d4b5d5b45d to 1f813e4c7f6d (1 revision) (flutter/engine#31362) * 321a482 Fix AccessibilityBridge crash due to invalid access during ReplaceSemanticsObject (flutter/engine#31351) * 2cc3abb Manual roll of ICU (flutter/engine#31132) * c1e843d Roll Fuchsia Linux SDK from 4VEg4eRJS... to YGS2LvlDy... (flutter/engine#31364) * fe08734 Roll Dart SDK from b7eea441d7d1 to 7e3310bbe1ed (2 revisions) (flutter/engine#31365) * 2062e57 Add trackpad gesture PointerData types (flutter/engine#28571) * 24fe585 Roll Skia from 1f813e4c7f6d to 5a2135af5623 (1 revision) (flutter/engine#31366) * 11bf86a Migrate string encoding conversions to FML (flutter/engine#31334) * 6d9f479 Roll Skia from 5a2135af5623 to 21a92dff8fdc (3 revisions) (flutter/engine#31368) * 417a05e Roll Dart SDK from 7e3310bbe1ed to a9cfcc289ed4 (1 revision) (flutter/engine#31369) * b04ed63 Change link to felt documentation (flutter/engine#31312) * 3e7515a Roll Fuchsia Mac SDK from 5CmPcHTb1... to ZA5ZzabQM... (flutter/engine#31370) * 90effff Revert "Add trackpad gesture PointerData types (#28571)" (flutter/engine#31375) * 3764a8f Change support for VM service message from "The Dart VM Service is listening" to "The Dart VM service is listening" (flutter/engine#31361) * 3d629c5 Fix html gradient rendering (#97762) (flutter/engine#31355) * 963c449 [Android] Show deprecation warnings for Android tests (flutter/engine#31246) * 0be895c Roll Dart SDK from a9cfcc289ed4 to 0041431f5ec7 (1 revision) (flutter/engine#31372) * 18f2faf Roll Skia from 21a92dff8fdc to c5d3326d767d (7 revisions) (flutter/engine#31373) * 1b762d3 Roll Fuchsia Linux SDK from YGS2LvlDy... to yDo1mhBKz... (flutter/engine#31374) * 7b6a7b6 Roll Skia from c5d3326d767d to b6dfd97c5290 (10 revisions) (flutter/engine#31377) * f55b161 [a11y] Delegate UTF8ToUTF16 to FML (flutter/engine#31376) * d36d25f TextEditingDelta Support for the Web (flutter/engine#28527) * aa17186 [fml] Use common FML string encoding utils (flutter/engine#31378) * 97d9ec3 Renamed the scenario tests target to be generic emulator tests. (flutter/engine#28919) * 902717f Roll Skia from b6dfd97c5290 to e1e2a858205f (3 revisions) (flutter/engine#31380) * 877f820 Roll Dart SDK from 0041431f5ec7 to a15d19a0d914 (2 revisions) (flutter/engine#31381) * 2d16729 Roll Skia from e1e2a858205f to 74ce095463e1 (2 revisions) (flutter/engine#31383) * 8d3c0fb [web] PathRef: do not use == with doubles in assertions (flutter/engine#31382) * d1164c1 Move recipes to repository folders. (flutter/engine#31367) * e031e07 Roll Skia from 74ce095463e1 to 82d65d0487bd (1 revision) (flutter/engine#31384) * 5140a44 Define thread priority enum and set thread priority for all threads in Engine (flutter/engine#30605) * 1e46918 Roll Dart SDK from a15d19a0d914 to a3736d4e9b1b (1 revision) (flutter/engine#31397) * 8a28948 Roll Fuchsia Linux SDK from yDo1mhBKz... to UHV3HWM3d... (flutter/engine#31398) * ca86c79 Roll Fuchsia Mac SDK from ZA5ZzabQM... to jjxstNbgZ... (flutter/engine#31399)

* 2a8f388 Roll Skia from a424b619bc40 to ec0af1664478 (6 revisions) (flutter/engine#31352) * ba8f1c5 Add clang-analyzer-* and clang-diagnostic-* to .clang-tidy (flutter/engine#31291) * 1eafb40 Use H5vcc CanvasKit implementation if it is detected. (flutter/engine#31191) * 6d99e90 Roll Dart SDK from 55c93c732da9 to b7eea441d7d1 (4 revisions) (flutter/engine#31353) * 935b46a Roll Fuchsia Mac SDK from 5_CZ81mTD... to 5CmPcHTb1... (flutter/engine#31356) * 244bb25 Roll Skia from ec0af1664478 to 9cb74e90792d (4 revisions) (flutter/engine#31357) * b6b5759 Roll Skia from 9cb74e90792d to 81d4b5d5b45d (6 revisions) (flutter/engine#31360) * 0f6db11 Roll Skia from 81d4b5d5b45d to 1f813e4c7f6d (1 revision) (flutter/engine#31362) * 321a482 Fix AccessibilityBridge crash due to invalid access during ReplaceSemanticsObject (flutter/engine#31351) * 2cc3abb Manual roll of ICU (flutter/engine#31132) * c1e843d Roll Fuchsia Linux SDK from 4VEg4eRJS... to YGS2LvlDy... (flutter/engine#31364) * fe08734 Roll Dart SDK from b7eea441d7d1 to 7e3310bbe1ed (2 revisions) (flutter/engine#31365) * 2062e57 Add trackpad gesture PointerData types (flutter/engine#28571) * 24fe585 Roll Skia from 1f813e4c7f6d to 5a2135af5623 (1 revision) (flutter/engine#31366) * 11bf86a Migrate string encoding conversions to FML (flutter/engine#31334) * 6d9f479 Roll Skia from 5a2135af5623 to 21a92dff8fdc (3 revisions) (flutter/engine#31368) * 417a05e Roll Dart SDK from 7e3310bbe1ed to a9cfcc289ed4 (1 revision) (flutter/engine#31369) * b04ed63 Change link to felt documentation (flutter/engine#31312) * 3e7515a Roll Fuchsia Mac SDK from 5CmPcHTb1... to ZA5ZzabQM... (flutter/engine#31370) * 90effff Revert "Add trackpad gesture PointerData types (flutter#28571)" (flutter/engine#31375) * 3764a8f Change support for VM service message from "The Dart VM Service is listening" to "The Dart VM service is listening" (flutter/engine#31361) * 3d629c5 Fix html gradient rendering (flutter#97762) (flutter/engine#31355) * 963c449 [Android] Show deprecation warnings for Android tests (flutter/engine#31246) * 0be895c Roll Dart SDK from a9cfcc289ed4 to 0041431f5ec7 (1 revision) (flutter/engine#31372) * 18f2faf Roll Skia from 21a92dff8fdc to c5d3326d767d (7 revisions) (flutter/engine#31373) * 1b762d3 Roll Fuchsia Linux SDK from YGS2LvlDy... to yDo1mhBKz... (flutter/engine#31374) * 7b6a7b6 Roll Skia from c5d3326d767d to b6dfd97c5290 (10 revisions) (flutter/engine#31377) * f55b161 [a11y] Delegate UTF8ToUTF16 to FML (flutter/engine#31376) * d36d25f TextEditingDelta Support for the Web (flutter/engine#28527) * aa17186 [fml] Use common FML string encoding utils (flutter/engine#31378) * 97d9ec3 Renamed the scenario tests target to be generic emulator tests. (flutter/engine#28919) * 902717f Roll Skia from b6dfd97c5290 to e1e2a858205f (3 revisions) (flutter/engine#31380) * 877f820 Roll Dart SDK from 0041431f5ec7 to a15d19a0d914 (2 revisions) (flutter/engine#31381) * 2d16729 Roll Skia from e1e2a858205f to 74ce095463e1 (2 revisions) (flutter/engine#31383) * 8d3c0fb [web] PathRef: do not use == with doubles in assertions (flutter/engine#31382) * d1164c1 Move recipes to repository folders. (flutter/engine#31367) * e031e07 Roll Skia from 74ce095463e1 to 82d65d0487bd (1 revision) (flutter/engine#31384) * 5140a44 Define thread priority enum and set thread priority for all threads in Engine (flutter/engine#30605) * 1e46918 Roll Dart SDK from a15d19a0d914 to a3736d4e9b1b (1 revision) (flutter/engine#31397) * 8a28948 Roll Fuchsia Linux SDK from yDo1mhBKz... to UHV3HWM3d... (flutter/engine#31398) * ca86c79 Roll Fuchsia Mac SDK from ZA5ZzabQM... to jjxstNbgZ... (flutter/engine#31399)

flutter-dashboard bot added platform-android platform-ios needs tests labels Dec 31, 2021

JsouLiang mentioned this pull request Dec 31, 2021

Set QOS and thread priorities for all threads managed by the Flutter engine. flutter/flutter#63812

Closed

JsouLiang force-pushed the JsouLiang branch 2 times, most recently from 2963aaf to e24ee60 Compare December 31, 2021 07:15

JsouLiang closed this Jan 5, 2022

JsouLiang force-pushed the JsouLiang branch from e24ee60 to 436a346 Compare January 5, 2022 03:10

JsouLiang reopened this Jan 5, 2022

JsouLiang force-pushed the JsouLiang branch from 4fef409 to 243c106 Compare January 5, 2022 03:17

JsouLiang changed the title ~~[WIP] Define thread priority enum and set thread priority for all threads in Engine~~ Define thread priority enum and set thread priority for all threads in Engine Jan 5, 2022

JsouLiang force-pushed the JsouLiang branch 4 times, most recently from 78321b4 to af19259 Compare January 5, 2022 15:35

JsouLiang mentioned this pull request Jan 6, 2022

Increasing UI and Raster thread priority on iOS flutter/flutter#65752

Closed

JsouLiang force-pushed the JsouLiang branch from a85f986 to af19259 Compare January 14, 2022 08:24

JsouLiang force-pushed the JsouLiang branch 2 times, most recently from 3f66389 to 49f45a9 Compare January 14, 2022 09:51

zanderso requested review from gaaclarke and dnfield February 2, 2022 07:23

gaaclarke requested changes Feb 2, 2022

View reviewed changes

JsouLiang force-pushed the JsouLiang branch 2 times, most recently from 5976fb3 to f4bf7fa Compare February 3, 2022 04:14

JsouLiang force-pushed the JsouLiang branch from 1535f76 to b130927 Compare February 9, 2022 09:02

gaaclarke requested changes Feb 9, 2022

View reviewed changes

JsouLiang force-pushed the JsouLiang branch 2 times, most recently from 9f3bf28 to 151ab6d Compare February 10, 2022 06:35

gaaclarke approved these changes Feb 10, 2022

View reviewed changes

gaaclarke added waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. and removed needs tests labels Feb 10, 2022

zanderso approved these changes Feb 10, 2022

View reviewed changes

shell/common/thread_host.h Outdated Show resolved Hide resolved

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 10, 2022

chinmaygarde 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 Feb 10, 2022

fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Feb 10, 2022

Split out the thread data and the thread data set operator

f18cf20

JsouLiang force-pushed the JsouLiang branch from 151ab6d to f18cf20 Compare February 11, 2022 02:48

zanderso merged commit 5140a44 into flutter:main Feb 11, 2022

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Engine from bed591d39448 to 5140a440c0b5 (38 revisions) flutter/flutter#98234

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

5140a44 Define thread priority enum and set thread priority for all t…

e014f7f

…hreads in Engine (flutter/engine#30605)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Engine from bed591d39448 to 1e46918c13d9 (39 revisions) flutter/flutter#98246

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

5140a44 Define thread priority enum and set thread priority for all t…

df6434f

…hreads in Engine (flutter/engine#30605)

engine-flutter-autoroll mentioned this pull request Feb 11, 2022

Roll Engine from bed591d39448 to ca86c7938d6b (41 revisions) flutter/flutter#98249

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 11, 2022

5140a44 Define thread priority enum and set thread priority for all t…

efb9204

…hreads in Engine (flutter/engine#30605)

yeatse mentioned this pull request Jul 9, 2022

Thread priority changes make app more easily to be stuck in production flutter/flutter#107351

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define thread priority enum and set thread priority for all threads in Engine #30605

Define thread priority enum and set thread priority for all threads in Engine #30605

JsouLiang commented Dec 31, 2021 •

edited

Loading

flutter-dashboard bot commented Dec 31, 2021

chinmaygarde commented Jan 13, 2022

JsouLiang commented Jan 14, 2022

JsouLiang commented Jan 14, 2022 •

edited

Loading

JsouLiang commented Jan 24, 2022

zanderso commented Feb 2, 2022

gaaclarke left a comment

gaaclarke Feb 2, 2022

JsouLiang Feb 3, 2022

gaaclarke Feb 2, 2022

JsouLiang Feb 3, 2022

gaaclarke Feb 3, 2022

gaaclarke Feb 3, 2022

gaaclarke left a comment

gaaclarke Feb 9, 2022

gaaclarke Feb 9, 2022

gaaclarke Feb 9, 2022

gaaclarke Feb 9, 2022

gaaclarke Feb 9, 2022

JsouLiang commented Feb 10, 2022

gaaclarke left a comment

zanderso left a comment

fluttergithubbot commented Feb 10, 2022

fluttergithubbot commented Feb 10, 2022

JsouLiang commented Feb 11, 2022

	bool isNeedThread(Type type) const { return type_mask & type; }
	bool isThreadNeeded(Type type) const { return type_mask & type; }

Define thread priority enum and set thread priority for all threads in Engine #30605

Define thread priority enum and set thread priority for all threads in Engine #30605

Conversation

JsouLiang commented Dec 31, 2021 • edited Loading

Pre-launch Checklist

flutter-dashboard bot commented Dec 31, 2021

chinmaygarde commented Jan 13, 2022

JsouLiang commented Jan 14, 2022

JsouLiang commented Jan 14, 2022 • edited Loading

JsouLiang commented Jan 24, 2022

zanderso commented Feb 2, 2022

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaaclarke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JsouLiang commented Feb 10, 2022

gaaclarke left a comment

Choose a reason for hiding this comment

zanderso left a comment

Choose a reason for hiding this comment

fluttergithubbot commented Feb 10, 2022

fluttergithubbot commented Feb 10, 2022

JsouLiang commented Feb 11, 2022

JsouLiang commented Dec 31, 2021 •

edited

Loading

JsouLiang commented Jan 14, 2022 •

edited

Loading