Enforce consistent stack size for Flutter threads #49111
Conversation
knopp
force-pushed
the
consistent_stack_size
branch
5 times, most recently
from
c806284
to
ed63891
Compare
| pthread_t current_thread = pthread_self(); | ||
| pthread_getname_np(current_thread, thread_name, 8); | ||
| pthread_getname_np(current_thread, thread_name, 16); |
There was a problem hiding this comment.
This fails with error 34 when buffer length is less than 16 bytes.
knopp
changed the title
| @@ -63,10 +109,10 @@ static void MockThreadConfigSetter(const fml::Thread::ThreadConfig& config) { | |||
| int policy = SCHED_OTHER; | |||
| switch (config.priority) { | |||
| case fml::Thread::ThreadPriority::kDisplay: | |||
| param.sched_priority = 10; | |||
| param.sched_priority = clamp_priority(10, policy); | |||
There was a problem hiding this comment.
Should the value perhaps be sched_get_priority_min and sched_get_priority_max? The values are platform specific (i.e. on macOS the allowed range is 15-47 for SCHED_OTHER, on Linux it is 0-0 for SCHED_OTHER and for other policies it requires root).
I don't quite understand how these tests were passing at all on linux.
There was a problem hiding this comment.
Ouch, actually, I know how. pthread tests never actually ran at all- because of missing build_config.h include FLUTTER_PTHREAD_SUPPORTED was 0 all the time.
|
This is only going to be sufficient for the root isolate. Should we consider this for all threads in the isolate thread pool? |
Aren't non-root isolate threads created by Dart VM? If so the stack size there is already set to 1MB, bigger than the 512kb default. The biggest problem with current stack size is dealing with deep widget hierarchy (i.e. we get stack overflow getting widget inspector snapshot on a moderately complex application). So this only concerns UI threads. |
Oh. I didn't realize this was already the case. Making the root isolate at least that big makes sense then. |
knopp
force-pushed
the
consistent_stack_size
branch
3 times, most recently
from
d4314c6
to
e483e52
Compare
|
Triage: There is consensus that this is a sound fix. |
fml/thread_unittests.cc
Outdated
| static int clamp_priority(int priority, int policy) { | ||
| int min = sched_get_priority_min(policy); | ||
| int max = sched_get_priority_max(policy); | ||
| if (priority < min) { |
There was a problem hiding this comment.
Can use std::clamp(priority, min, max) here
There was a problem hiding this comment.
I guess my head is stuck in c++ 14 :)
knopp
force-pushed
the
consistent_stack_size
branch
from
e483e52
to
ca815c3
Compare
…141112) flutter/engine@bbebee1...db564ff 2024-01-08 skia-flutter-autoroll@skia.org Roll Skia from 272281e56cdc to 4b7cab157b50 (1 revision) (flutter/engine#49594) 2024-01-08 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from I-3hiLjX2DDy6mu22... to 6z4cZ6HUidtgmhvdk... (flutter/engine#49592) 2024-01-08 matej.knopp@gmail.com Enforce consistent stack size for Flutter threads (flutter/engine#49111) 2024-01-08 skia-flutter-autoroll@skia.org Roll Skia from 384e08e5fbbe to 272281e56cdc (1 revision) (flutter/engine#49588) 2024-01-08 skia-flutter-autoroll@skia.org Roll Skia from e2621f417ff5 to 384e08e5fbbe (1 revision) (flutter/engine#49587) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from I-3hiLjX2DDy to 6z4cZ6HUidtg If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Fixes flutter/flutter#72156
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.