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

Delete requestPaint from Scheduler #31782

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sebmarkbage
Copy link
Collaborator

It's a noop so it's misleading. It's only implemented in the SchedulerMock. Which means that a bunch of our tests are actually testing behavior that doesn't work in the real runtime nor in tests when not using the mock. See failing tests.

It's a noop so it's misleading.
Copy link

vercel bot commented Dec 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 14, 2024 8:11pm

@sebmarkbage sebmarkbage marked this pull request as draft December 14, 2024 20:10
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 14, 2024
@react-sizebot
Copy link

Comparing: c32780e...72ca3d5

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.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 510.72 kB 510.65 kB = 91.48 kB 91.46 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 515.66 kB 515.59 kB = 92.19 kB 92.16 kB
facebook-www/ReactDOM-prod.classic.js = 595.04 kB 594.97 kB = 105.00 kB 104.98 kB
facebook-www/ReactDOM-prod.modern.js = 585.31 kB 585.24 kB = 103.44 kB 103.42 kB
oss-experimental/scheduler/cjs/scheduler.native.development.js = 12.39 kB 12.13 kB = 2.56 kB 2.53 kB
oss-stable-semver/scheduler/cjs/scheduler.native.development.js = 12.39 kB 12.13 kB = 2.56 kB 2.53 kB
oss-stable/scheduler/cjs/scheduler.native.development.js = 12.39 kB 12.13 kB = 2.56 kB 2.53 kB
oss-experimental/scheduler/cjs/scheduler.native.production.js = 10.97 kB 10.73 kB = 2.46 kB 2.43 kB
oss-stable-semver/scheduler/cjs/scheduler.native.production.js = 10.97 kB 10.73 kB = 2.46 kB 2.43 kB
oss-stable/scheduler/cjs/scheduler.native.production.js = 10.97 kB 10.73 kB = 2.46 kB 2.43 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/Scheduler-dev.classic.js = 16.39 kB 16.34 kB = 3.48 kB 3.47 kB
facebook-www/Scheduler-dev.modern.js = 16.39 kB 16.34 kB = 3.48 kB 3.47 kB
oss-experimental/scheduler/cjs/scheduler.development.js = 12.30 kB 12.25 kB = 2.71 kB 2.70 kB
oss-stable-semver/scheduler/cjs/scheduler.development.js = 12.30 kB 12.25 kB = 2.71 kB 2.70 kB
oss-stable/scheduler/cjs/scheduler.development.js = 12.30 kB 12.25 kB = 2.71 kB 2.70 kB
facebook-react-native/scheduler/cjs/Scheduler-dev.js = 12.26 kB 12.21 kB = 2.68 kB 2.67 kB
facebook-www/Scheduler-profiling.classic.js = 11.28 kB 11.23 kB = 2.76 kB 2.75 kB
facebook-www/Scheduler-profiling.modern.js = 11.28 kB 11.23 kB = 2.76 kB 2.75 kB
facebook-www/Scheduler-prod.classic.js = 10.84 kB 10.79 kB = 2.66 kB 2.65 kB
facebook-www/Scheduler-prod.modern.js = 10.84 kB 10.79 kB = 2.66 kB 2.65 kB
facebook-react-native/scheduler/cjs/Scheduler-profiling.js = 10.77 kB 10.73 kB = 2.64 kB 2.63 kB
facebook-www/SchedulerMock-dev.classic.js = 17.59 kB 17.51 kB = 3.69 kB 3.67 kB
facebook-www/SchedulerMock-dev.modern.js = 17.59 kB 17.51 kB = 3.69 kB 3.67 kB
oss-experimental/scheduler/cjs/scheduler.production.js = 10.35 kB 10.30 kB = 2.54 kB 2.53 kB
oss-stable-semver/scheduler/cjs/scheduler.production.js = 10.35 kB 10.30 kB = 2.54 kB 2.53 kB
oss-stable/scheduler/cjs/scheduler.production.js = 10.35 kB 10.30 kB = 2.54 kB 2.53 kB
facebook-react-native/scheduler/cjs/Scheduler-prod.js = 10.34 kB 10.29 kB = 2.54 kB 2.53 kB
facebook-www/SchedulerMock-prod.classic.js = 12.48 kB 12.41 kB = 2.93 kB 2.92 kB
facebook-www/SchedulerMock-prod.modern.js = 12.48 kB 12.41 kB = 2.93 kB 2.92 kB
oss-experimental/scheduler/cjs/scheduler-unstable_mock.production.js = 12.30 kB 12.23 kB = 2.87 kB 2.86 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_mock.production.js = 12.30 kB 12.23 kB = 2.87 kB 2.86 kB
oss-stable/scheduler/cjs/scheduler-unstable_mock.production.js = 12.30 kB 12.23 kB = 2.87 kB 2.86 kB
facebook-react-native/scheduler/cjs/SchedulerMock-prod.js = 12.28 kB 12.21 kB = 2.86 kB 2.85 kB
oss-experimental/scheduler/cjs/scheduler-unstable_mock.development.js = 14.03 kB 13.95 kB = 2.97 kB 2.95 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_mock.development.js = 14.03 kB 13.95 kB = 2.97 kB 2.95 kB
oss-stable/scheduler/cjs/scheduler-unstable_mock.development.js = 14.03 kB 13.95 kB = 2.97 kB 2.95 kB
facebook-react-native/scheduler/cjs/SchedulerMock-dev.js = 13.98 kB 13.90 kB = 2.93 kB 2.92 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task.development.js = 4.83 kB 4.78 kB = 1.22 kB 1.21 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_post_task.development.js = 4.83 kB 4.78 kB = 1.22 kB 1.21 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task.development.js = 4.83 kB 4.78 kB = 1.22 kB 1.21 kB
facebook-www/SchedulerPostTask-dev.classic.js = 4.81 kB 4.76 kB = 1.21 kB 1.20 kB
facebook-www/SchedulerPostTask-dev.modern.js = 4.81 kB 4.76 kB = 1.21 kB 1.20 kB
facebook-www/SchedulerPostTask-prod.classic.js = 4.16 kB 4.11 kB = 1.16 kB 1.15 kB
facebook-www/SchedulerPostTask-prod.modern.js = 4.16 kB 4.11 kB = 1.16 kB 1.15 kB
facebook-www/SchedulerPostTask-profiling.classic.js = 4.16 kB 4.11 kB = 1.16 kB 1.15 kB
facebook-www/SchedulerPostTask-profiling.modern.js = 4.16 kB 4.11 kB = 1.16 kB 1.15 kB
oss-experimental/scheduler/cjs/scheduler-unstable_post_task.production.js = 4.15 kB 4.10 kB = 1.15 kB 1.14 kB
oss-stable-semver/scheduler/cjs/scheduler-unstable_post_task.production.js = 4.15 kB 4.10 kB = 1.15 kB 1.14 kB
oss-stable/scheduler/cjs/scheduler-unstable_post_task.production.js = 4.15 kB 4.10 kB = 1.15 kB 1.14 kB
oss-experimental/scheduler/cjs/scheduler.native.development.js = 12.39 kB 12.13 kB = 2.56 kB 2.53 kB
oss-stable-semver/scheduler/cjs/scheduler.native.development.js = 12.39 kB 12.13 kB = 2.56 kB 2.53 kB
oss-stable/scheduler/cjs/scheduler.native.development.js = 12.39 kB 12.13 kB = 2.56 kB 2.53 kB
oss-experimental/scheduler/cjs/scheduler.native.production.js = 10.97 kB 10.73 kB = 2.46 kB 2.43 kB
oss-stable-semver/scheduler/cjs/scheduler.native.production.js = 10.97 kB 10.73 kB = 2.46 kB 2.43 kB
oss-stable/scheduler/cjs/scheduler.native.production.js = 10.97 kB 10.73 kB = 2.46 kB 2.43 kB

Generated by 🚫 dangerJS against 608d5dd

sebmarkbage added a commit that referenced this pull request Dec 14, 2024
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in #31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬
github-actions bot pushed a commit that referenced this pull request Dec 14, 2024
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in #31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬

DiffTrain build for [c80b336](c80b336)
github-actions bot pushed a commit that referenced this pull request Dec 14, 2024
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in #31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬

DiffTrain build for [c80b336](c80b336)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Dec 15, 2024
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in facebook#31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬

DiffTrain build for [c80b336](facebook@c80b336)
github-actions bot pushed a commit to code/lib-react that referenced this pull request Dec 15, 2024
When implementing passive effects we did a pretty massive oversight.
While the passive effect is scheduled into its own scheduler task, the
scheduler doesn't always yield to the browser if it has time left. That
means that if you have a fast commit phase, it might try to squeeze in
the passive effects in the same frame but those then might end being
very heavy.

We had `requestPaint()` for this but that was only implemented for the
`isInputPending` experiment. It wasn't thought we needed it for the
regular scheduler because it yields "every frame" anyway - but it
doesn't yield every task. While the `isInputPending` experiment showed
that it wasn't actually any significant impact, and it was better to
keep shorter yield time anyway. Which is why we deleted the code.
Whatever small win it did see in some cases might have been actually due
to this issue rather than anything to do with `isInputPending` at all.

As you can see in facebook#31782 we do
have this implemented in the mock scheduler and a lot of behavior that
we assert assumes that this works.

So this just implements yielding after `requestPaint` is called.

Before:

<img width="1023" alt="Screenshot 2024-12-14 at 3 40 24 PM"
src="https://github.com/user-attachments/assets/d60f4bb2-c8f8-4f91-a402-9ac25b278450"
/>

After:

<img width="1108" alt="Screenshot 2024-12-14 at 3 41 25 PM"
src="https://github.com/user-attachments/assets/170cdb90-a049-436f-9501-be3fb9bc04ca"
/>

Notice how in after the native task is split into two. It might not
always actually paint and the native scheduler might make the same
mistake and think it has enough time left but it's at least less likely
to.

We do have another way to do this. When we yield a continuation we also
yield to the native browser. This is to enable the Suspense Optimization
(currently disabled) to work. We could do the same for passive effects
and, in fact, I have a branch that does but because that requires a lot
more tests to be fixed it's a lot more invasive of a change. The nice
thing about this approach is that this is not even running in tests at
all and the tests we do have assert that this is the behavior already. 😬

DiffTrain build for [c80b336](facebook@c80b336)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants