Add support for setting thread priority in embedder.h and update windows embedding to do so #31778
Conversation
| break; | ||
| } | ||
| case fml::Thread::ThreadPriority::RASTER: { | ||
| if (SetPriorityClass(GetCurrentProcess(), ABOVE_NORMAL_PRIORITY_CLASS) != 0) { |
There was a problem hiding this comment.
Chrome equivalent might be GPU thread: https://source.chromium.org/chromium/chromium/src/+/main:content/gpu/gpu_main.cc;l=234?q=ABOVE_NORMAL_PRIORITY_CLASS&ss=chromium
Uses ABOVE_NORMAL_PRIORITY_CLASS - the flag is enabled by default
There was a problem hiding this comment.
This isn't correct since this refers to the process priority, which we may need to consider as well
jonahwilliams
changed the title
| EXPECT_EQ(strcmp(args->dart_entrypoint_argv[1], "arg2"), 0); | ||
| EXPECT_NE(args->platform_message_callback, nullptr); | ||
| EXPECT_NE(args->custom_task_runners, nullptr); | ||
| EXPECT_NE(args->custom_task_runners->thread_priority_setter, nullptr); |
There was a problem hiding this comment.
This needs better tests naturally...
There was a problem hiding this comment.
Can you add some tests where you call the setter function with different priority levels and check the priority of the current thread after each call?
There was a problem hiding this comment.
Yeah, when I did that I noticed I was setting process priority level 🤦
There was a problem hiding this comment.
The added test is good, but can you also call the thread_priority_setter here to make sure it's the one we think it is?
| // mark thread priorities and success/failure. | ||
| switch (priority) { | ||
| case FlutterThreadPriority::kBackground: { | ||
| SetPriorityClass(GetCurrentProcess(), BELOW_NORMAL_PRIORITY_CLASS); |
There was a problem hiding this comment.
Shouldn't this be SetThreadPriority/GetCurrentThread?
There was a problem hiding this comment.
Yeah, just realized.
dnfield
left a comment
dnfield
left a comment
There was a problem hiding this comment.
Suggestion for test, seems like we are setting process class rather than thread priority here.
|
Somehow the license bot is unhappy? Otherwise LGTM. |
|
Might be because I branched off of master instead of main |
jonahwilliams
added
the
waiting for tree to go green
…ate windows embedding to do so (flutter/engine#31778)
3 participants
Currently windows does not set a thread priority, this may be the cause of pauses in the raster thread - outside of occasionally blocking on SwapBuffers.
Expose a new field on the custom task runners embedder struct which allows users to set the thread priority on the current thread given an enum describing the purpose of that thread.
Does not expose the thread name
Also wires this support into the windows embedding