-
Notifications
You must be signed in to change notification settings - Fork 47.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
integrate scheduler.yield in SchedulerPostTask #27069
Conversation
38629af
to
213adc9
Compare
// $FlowFixMe[cannot-resolve-module] | ||
require('SchedulerFeatureFlags'); | ||
|
||
export const enableSchedulerDebugging = true; | ||
export const enableProfiling: boolean = | ||
__PROFILE__ && enableProfilingFeatureFlag; | ||
export const enableSchedulerYield = enableSchedulerYieldFeatureFlag; |
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.
i had overlooked updating this before, which is (apparently) what broke the build with these changes
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.
Is Meta actually running the postTask build anywhere? I don't think there's much benefit to switching to postTask unless we can also use yield so maybe we can skip this feature flag and just bundle the postTask + yield changes together.
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.
no, we're not - that's a good point.
the only justification for the flag i can think of is that A/Bing the PostTask scheduler with and without yield
might provide better signal for giving feedback to the chrome team for the origin trial?
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.
Personally I'd rather just kill any flag that we don't have a solid reason for, it's already too complicated. But I'll leave it up to you.
## Summary `scheduler.yield` is entering [Origin Trial soon in Chrome 115](https://chromestatus.com/feature/6266249336586240). This diff adds it to `SchedulerPostTask` when scheduling continuations to allow Origin Trial participation for early feedback on the new API. It seems the difference here versus the current use of `postTask` will be minor – the intent behind `scheduler.yield` seems to mostly be better ergonomics for scheduling continuations, but it may be interesting to see if the follow aspect of it results in any tangible difference in scheduling (from [here](https://github.com/WICG/scheduling-apis/blob/main/explainers/yield-and-continuation.md#introduction)): > To mitigate yielding performance penalty concerns, UAs prioritize scheduler.yield() continuations over tasks of the same priority or similar task sources. ## How did you test this change? ``` yarn test SchedulerPostTask ``` DiffTrain build for [0a36064](0a36064)
### React upstream changes - facebook/react#27096 - facebook/react#27069 - facebook/react#27033 - facebook/react#27061 - facebook/react#27030 - facebook/react#27056
there is another scheduler.postTask in line 110. |
## Summary `scheduler.yield` is entering [Origin Trial soon in Chrome 115](https://chromestatus.com/feature/6266249336586240). This diff adds it to `SchedulerPostTask` when scheduling continuations to allow Origin Trial participation for early feedback on the new API. It seems the difference here versus the current use of `postTask` will be minor – the intent behind `scheduler.yield` seems to mostly be better ergonomics for scheduling continuations, but it may be interesting to see if the follow aspect of it results in any tangible difference in scheduling (from [here](https://github.com/WICG/scheduling-apis/blob/main/explainers/yield-and-continuation.md#introduction)): > To mitigate yielding performance penalty concerns, UAs prioritize scheduler.yield() continuations over tasks of the same priority or similar task sources. ## How did you test this change? ``` yarn test SchedulerPostTask ```
## Summary `scheduler.yield` is entering [Origin Trial soon in Chrome 115](https://chromestatus.com/feature/6266249336586240). This diff adds it to `SchedulerPostTask` when scheduling continuations to allow Origin Trial participation for early feedback on the new API. It seems the difference here versus the current use of `postTask` will be minor – the intent behind `scheduler.yield` seems to mostly be better ergonomics for scheduling continuations, but it may be interesting to see if the follow aspect of it results in any tangible difference in scheduling (from [here](https://github.com/WICG/scheduling-apis/blob/main/explainers/yield-and-continuation.md#introduction)): > To mitigate yielding performance penalty concerns, UAs prioritize scheduler.yield() continuations over tasks of the same priority or similar task sources. ## How did you test this change? ``` yarn test SchedulerPostTask ``` DiffTrain build for commit 0a36064.
Summary
scheduler.yield
is entering Origin Trial soon in Chrome 115. This diff adds it toSchedulerPostTask
when scheduling continuations to allow Origin Trial participation for early feedback on the new API.It seems the difference here versus the current use of
postTask
will be minor – the intent behindscheduler.yield
seems to mostly be better ergonomics for scheduling continuations, but it may be interesting to see if the follow aspect of it results in any tangible difference in scheduling (from here):How did you test this change?