Skip to content

Conversation

@gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 13, 2018

I don't know if this is a bug but this behavior seems weird.

The sequence is:

  • Perform update 1
  • Schedule update 2
  • Start rendering update 2
  • Advance time so that update 2 is expired
  • Schedule update 3
  • Flush expired work

My expectation is this should only flush update 2 (it's the one that expired). Update 3 hasn't expired yet.

However, this flushes update 3 instead. If I revert #13503, this flushes both 2 and 3. I can't get it to only flush 2, but that seems like the desired behavior.

By itself flushing 3 isn't that bad. The real situation I was trying to reproduce is when we expire another update while rendering a just-expired update, and that kicks off another rendering cycle without yielding. But I couldn't get to reproducing that because even a more basic scenario above didn't work as I expected.

@sizebot
Copy link

sizebot commented Oct 13, 2018

Details of bundled changes.

Comparing: 4773fdf...d60577c

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 14, 2018

ok, this particular behavior is because we don't recalculate time:

// There's already pending work. We might be in the middle of a browser
// event. If we were to read the current time, it could cause multiple updates
// within the same event to receive different expiration times, leading to
// tearing. Return the last read time. During the next idle callback, the
// time will be updated.
return currentSchedulerTime;

So in the test "advance time" doesn't actually advance anything for the scheduler, and that's why test gives confusing results.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 14, 2018

Even if we recalculated time in the test (which I'm not sure how to do), the actual issue I'm trying to debug is here:

(!deadlineDidExpire || currentRendererTime >= nextFlushedExpirationTime)

When something expires, we keep doing all expired work. But we don't show it to the user so from their perspective we're just freezing the browser. Not very useful. Seems like we should yield sometimes (always?) if we just flushed something that expired.

I tried just adding a break into this loop when currentRendererTime >= nextFlushedExpirationTime (which means we just rendered something expired). But this wasn't sufficient because some other callback from the scheduler got called in the same tick, pulling us into this loop again.

Remembering currentRendererTime during break and exiting further performWork calls with the same currentRendererTime seemed to work but is gross?

@necolas necolas added the React Core Team Opened by a member of the React Core Team label Jan 8, 2020
@gaearon
Copy link
Collaborator Author

gaearon commented Jan 23, 2020

This might have to do with the "terminal updates" work @acdlite is doing. Or maybe not. I don't know if this is still a problem but it came up in my time slicing demos.

@gaearon gaearon closed this Feb 11, 2021
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.

4 participants