-
Notifications
You must be signed in to change notification settings - Fork 6k
Fix Android IO and Worker threads priority issue #23911
Conversation
|
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. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1 similar comment
|
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
|
@googlebot I signed it! |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
1 similar comment
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
|
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. |
|
cc @dnfield. This seem like a sound patch. Though, we should followup on an iOS implementation as well. |
|
This does not impact the Dart worker threads, which should likely get similar prioritization. |
|
(not that it should block this patch) |
Dart worker inherit Flutter UI thread's -1 priority, lower it to 1 maybe better, although not as necessary as this |
chinmaygarde
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should figure out a way to generalize this to all platforms. In the meantime, I think this is fine for Android.
Manually set IO and Worker threads priority to THREAD_PRIORITY_LESS_FAVORABLE(1).
So manually set IO and Worker threads to an appropriate background priority such as THREAD_PRIORITY_LESS_FAVORABLE(1) will be a good move, it will work nicely on both Android 8+ and lower version.
List which issues are fixed by this PR. You must list at least one issue.
The background threads - IO and Worker threads, have much higher priority than UI and Raster thread, and easy to make them blocked and cause jank.
If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.
No
Pre-launch Checklist
writing and running engine tests.
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.