Replace flutter_runner::Thread with fml::Thread #26783
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
flutter-dashboard
bot
added
platform-fuchsia
platform-web
|
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. ℹ️ Googlers: Go here for more info. |
added 3 commits
July 2, 2021 19:02
This is a preparatory CL. The next CL will cause task_observers.h to be included from both shell/platform/fuchsia _and_ fml/platform/fuchsia. I imagine it might be gauche for a single file to be used from both those directories, but it seems way worse for fml to depend on shell, rather than the other way around.
This CL makes the treading logic in Fuchsia's flutter_runner more consistent with fml expectations, but it still has quirks. Notably: - Not all async work get posted to a fml::TaskRunner. Some work done by Fuchsia libraries gets posted directly to async_get_default_dispatcher(). This work doesn't trigger the fml::MessageLoop's task observers. As a result, we continue to have Fuchsia-specific task observers which fire for all async work, regardless of which way it was posted. - There's awkwardness when trying to run Fuchsia code on a specific fml::TaskRunner if that fuchsia code accepts an async_dispatcher_t. Since you can no longer get an async_dispatcher_t for a given thread, you instead must post a closure to the fml::TaskRunner that calls async_get_default_dispatcher(), and then calls the fuchsia library with the default dispatcher. - Some tests still use task_runner_adapter.h because async::Loop offers more control in unit tests than fml::MessageLoop does. - If this successfully lands, there will be some cosmetic follow-up changes to make, like using ThreadHost instead of an array of fml::Threads.
The previous change removes our custom 1MiB limit on the stack of newly created threads, so these threads revert to the Fuchsia default. google-internal tests were failing (without any particular error in the logs) after this change, so here we just set the default stack size to 1MiB.
|
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. |
|
@arbreng Ready when you are! This has passed Fuchsia GI. |
arbreng
approved these changes
Jul 6, 2021
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/flutter
that referenced
this pull request
Jul 6, 2021
moffatman
pushed a commit
to moffatman/engine
that referenced
this pull request
Aug 5, 2021
* Move task_observers.{cc,h} into fml.
This is a preparatory CL. The next CL will cause task_observers.h to
be included from both shell/platform/fuchsia _and_
fml/platform/fuchsia. I imagine it might be gauche for a single file
to be used from both those directories, but it seems way worse for fml
to depend on shell, rather than the other way around.
* Replace flutter_runner::Thread with fml::Thread.
This CL makes the treading logic in Fuchsia's flutter_runner more
consistent with fml expectations, but it still has quirks. Notably:
- Not all async work get posted to a fml::TaskRunner. Some work done
by Fuchsia libraries gets posted directly to
async_get_default_dispatcher(). This work doesn't trigger the
fml::MessageLoop's task observers. As a result, we continue to have
Fuchsia-specific task observers which fire for all async work,
regardless of which way it was posted.
- There's awkwardness when trying to run Fuchsia code on a specific
fml::TaskRunner if that fuchsia code accepts an
async_dispatcher_t. Since you can no longer get an
async_dispatcher_t for a given thread, you instead must post a
closure to the fml::TaskRunner that calls
async_get_default_dispatcher(), and then calls the fuchsia library
with the default dispatcher.
- Some tests still use task_runner_adapter.h because async::Loop
offers more control in unit tests than fml::MessageLoop does.
- If this successfully lands, there will be some cosmetic follow-up
changes to make, like using ThreadHost instead of an array of
fml::Threads.
* Increase stack size in flutter runner.
The previous change removes our custom 1MiB limit on the stack of
newly created threads, so these threads revert to the Fuchsia
default. google-internal tests were failing (without any particular
error in the logs) after this change, so here we just set the default
stack size to 1MiB.
Co-authored-by: Hunter Freyer <hjfreyer@google.com>
naudzghebre
pushed a commit
to naudzghebre/engine
that referenced
this pull request
Sep 2, 2021
* Move task_observers.{cc,h} into fml.
This is a preparatory CL. The next CL will cause task_observers.h to
be included from both shell/platform/fuchsia _and_
fml/platform/fuchsia. I imagine it might be gauche for a single file
to be used from both those directories, but it seems way worse for fml
to depend on shell, rather than the other way around.
* Replace flutter_runner::Thread with fml::Thread.
This CL makes the treading logic in Fuchsia's flutter_runner more
consistent with fml expectations, but it still has quirks. Notably:
- Not all async work get posted to a fml::TaskRunner. Some work done
by Fuchsia libraries gets posted directly to
async_get_default_dispatcher(). This work doesn't trigger the
fml::MessageLoop's task observers. As a result, we continue to have
Fuchsia-specific task observers which fire for all async work,
regardless of which way it was posted.
- There's awkwardness when trying to run Fuchsia code on a specific
fml::TaskRunner if that fuchsia code accepts an
async_dispatcher_t. Since you can no longer get an
async_dispatcher_t for a given thread, you instead must post a
closure to the fml::TaskRunner that calls
async_get_default_dispatcher(), and then calls the fuchsia library
with the default dispatcher.
- Some tests still use task_runner_adapter.h because async::Loop
offers more control in unit tests than fml::MessageLoop does.
- If this successfully lands, there will be some cosmetic follow-up
changes to make, like using ThreadHost instead of an array of
fml::Threads.
* Increase stack size in flutter runner.
The previous change removes our custom 1MiB limit on the stack of
newly created threads, so these threads revert to the Fuchsia
default. google-internal tests were failing (without any particular
error in the logs) after this change, so here we just set the default
stack size to 1MiB.
Co-authored-by: Hunter Freyer <hjfreyer@google.com>
arbreng
added a commit
to arbreng/flutter_engine
that referenced
this pull request
Sep 21, 2021
This reverts commit 46178d6.
arbreng
added a commit
to arbreng/flutter_engine
that referenced
this pull request
Sep 21, 2021
This reverts commit 46178d6.
arbreng
added a commit
that referenced
this pull request
Sep 21, 2021
This was referenced Oct 5, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Another attempt to land #26193. The previous version changed the stack size for threads, which caused test failures. This version passes those tests.
Original description:
This CL makes the treading logic in Fuchsia's flutter_runner more
consistent with fml expectations, but it still has quirks. Notably:
Not all async work get posted to a fml::TaskRunner. Some work done
by Fuchsia libraries gets posted directly to
async_get_default_dispatcher(). This work doesn't trigger the
fml::MessageLoop's task observers. As a result, we continue to have
Fuchsia-specific task observers which fire for all async work,
regardless of which way it was posted.
There's awkwardness when trying to run Fuchsia code on a specific
fml::TaskRunner if that fuchsia code accepts an
async_dispatcher_t. Since you can no longer get an
async_dispatcher_t for a given thread, you instead must post a
closure to the fml::TaskRunner that calls
async_get_default_dispatcher(), and then calls the fuchsia library
with the default dispatcher.
Some tests still use task_runner_adapter.h because async::Loop
offers more control in unit tests than fml::MessageLoop does.
If this successfully lands, there will be some cosmetic follow-up
changes to make, like using ThreadHost instead of an array of
fml::Threads.