Skip to content
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

Merged
merged 4 commits into from
Jul 11, 2023

Conversation

noahlemen
Copy link
Member

@noahlemen noahlemen commented Jul 7, 2023

Summary

scheduler.yield is entering Origin Trial soon in Chrome 115. 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):

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                

// $FlowFixMe[cannot-resolve-module]
require('SchedulerFeatureFlags');

export const enableSchedulerDebugging = true;
export const enableProfiling: boolean =
__PROFILE__ && enableProfilingFeatureFlag;
export const enableSchedulerYield = enableSchedulerYieldFeatureFlag;
Copy link
Member Author

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

Copy link
Collaborator

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.

Copy link
Member Author

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?

Copy link
Collaborator

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.

@react-sizebot
Copy link

react-sizebot commented Jul 10, 2023

Comparing: fdc8c81...7b7e193

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 164.35 kB 164.35 kB = 51.76 kB 51.76 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 171.76 kB 171.76 kB = 53.98 kB 53.98 kB
facebook-www/ReactDOM-prod.classic.js = 567.22 kB 567.22 kB = 100.04 kB 100.04 kB
facebook-www/ReactDOM-prod.modern.js = 551.02 kB 551.02 kB = 97.21 kB 97.21 kB
facebook-www/SchedulerPostTask-prod.classic.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
facebook-www/SchedulerPostTask-prod.modern.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
facebook-www/SchedulerPostTask-profiling.classic.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
facebook-www/SchedulerPostTask-profiling.modern.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task.production.min.js +2.69% 1.97 kB 2.02 kB +3.91% 0.84 kB 0.88 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_post_task.production.min.js +2.69% 1.97 kB 2.02 kB +3.91% 0.84 kB 0.88 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task.production.min.js +2.69% 1.97 kB 2.02 kB +3.91% 0.84 kB 0.88 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task.development.js +2.69% 6.88 kB 7.07 kB +2.59% 2.08 kB 2.14 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_post_task.development.js +2.69% 6.88 kB 7.07 kB +2.59% 2.08 kB 2.14 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task.development.js +2.69% 6.88 kB 7.07 kB +2.59% 2.08 kB 2.14 kB
facebook-www/SchedulerPostTask-dev.classic.js +2.40% 7.01 kB 7.18 kB +2.28% 2.11 kB 2.16 kB
facebook-www/SchedulerPostTask-dev.modern.js +2.40% 7.01 kB 7.18 kB +2.28% 2.11 kB 2.16 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/SchedulerPostTask-prod.classic.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
facebook-www/SchedulerPostTask-prod.modern.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
facebook-www/SchedulerPostTask-profiling.classic.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
facebook-www/SchedulerPostTask-profiling.modern.js +5.35% 4.10 kB 4.31 kB +5.40% 1.13 kB 1.19 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task.production.min.js +2.69% 1.97 kB 2.02 kB +3.91% 0.84 kB 0.88 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_post_task.production.min.js +2.69% 1.97 kB 2.02 kB +3.91% 0.84 kB 0.88 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task.production.min.js +2.69% 1.97 kB 2.02 kB +3.91% 0.84 kB 0.88 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task.development.js +2.69% 6.88 kB 7.07 kB +2.59% 2.08 kB 2.14 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_post_task.development.js +2.69% 6.88 kB 7.07 kB +2.59% 2.08 kB 2.14 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task.development.js +2.69% 6.88 kB 7.07 kB +2.59% 2.08 kB 2.14 kB
facebook-www/SchedulerPostTask-dev.classic.js +2.40% 7.01 kB 7.18 kB +2.28% 2.11 kB 2.16 kB
facebook-www/SchedulerPostTask-dev.modern.js +2.40% 7.01 kB 7.18 kB +2.28% 2.11 kB 2.16 kB

Generated by 🚫 dangerJS against 7b7e193

@noahlemen noahlemen changed the title integrate scheduler.yield in SchedulerPostTask behind a flag integrate scheduler.yield in SchedulerPostTask Jul 11, 2023
@noahlemen noahlemen merged commit 0a36064 into facebook:main Jul 11, 2023
2 checks passed
@noahlemen noahlemen deleted the scheduler-yield branch July 11, 2023 18:36
github-actions bot pushed a commit that referenced this pull request Jul 11, 2023
## 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)
@LeeeeeeM
Copy link

there is another scheduler.postTask in line 110.

EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
## 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                
```
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants