Use a microtask to detect end of event #17158
Conversation
acdlite
force-pushed
the
reset-event-time-microtask
branch
from
bbd7729 to
43129fa
Compare
|
From what I gathered from @gaearon in his repo it was indefinitely stalled until returning back to the tab and clicking something. Yet nothing was running on the thread. In your description you mention that everything is accompanied by render tasks. But if that’s the case, how do we explain his bug? I believe Brian had a similar one. Not sure if we fixed that one. Seems like we need to truly understand that bug until we have a fix. Not sure about this micro task thing. We need to cut down on overhead as part of doing something as simple as setting state and we end up with a lot of scheduling now and a lot or computing of expirations. Is there a simpler model here? |
Believe it or not, I asked myself that question before I opened the PR! With that out of the way...
It's because the DevTools hook calls So SiMplEr fixes would include:
I'm not satisfied with those solutions because getting this wrong is subtle and the consequences when it goes wrong are bad.
I don't think it adds that much relative overhead because it's only for the first update per task. So in the Flux case, it's constant time overhead. I think the worst case is if you have many separate Scheduler or browser tasks that all schedule individual updates really quickly. I could lower the priority from Immediate down to Normal/Default. |
|
I think we should also have separate callers for reading the current time without freezing it, which is only needed when computing an update, versus reading the current time for any other reason. Perhaps:
|
The DevTools hook calls `requestCurrentTime` after the commit phase has ended, which has the accidnental consequence of freezing the start time for subsequent updates. If enough time goes by, the next update will instantly expire. I'll push a fix in the next commit.
acdlite
force-pushed
the
reset-event-time-microtask
branch
from
43129fa to
b486715
Compare
Updates of like priority that occur within the same browser event are guaranteed to be batched. This is an important guarantee for event emitter or subscription-like patterns where a single browser event can spawn updates on many different components, all of which need to commit consistently. To do this, we have a heuristic of marking the first "event time" of an update inside an event. Subsequent updates in the same event will reuse the same event time. At the end of the event, the event time should be cleared. Failing to clear the event time has the effect of "freezing" the timeline, causing weird expiration quirks. The event time used to be cleared right before entering the render phase. This mostly works, since every update will eventually be followed by a render task. However, it has some flaws: if there's lots of other non-React main thread work in the mean time, subsequent events could get overbatched. The heuristic is also a bit fragile; it happens to be the case that every call to `requestCurrentTime` is accompanied by a render task, but that may not necessarily always be the case, creating a subtle refactor hazard. Instead of relying on the next render task to reset the current event time, this change schedules a microtask. I didn't use an actual microtask because React does not currently require the use a Promise polyfill. Instead, I'm using a Scheduler task at immediate priority, which has almost the same semantics. (There's even an open question of whether we should get rid of Scheduler's "immediate" priority in favor of microtasks, but since it currently exists in the browser proposal, too, we can figure that out later.)
acdlite
force-pushed
the
reset-event-time-microtask
branch
from
b486715 to
34bba1c
Compare
|
Bug that motivated this change was fixed in #17158. I'm still concerned about the refactor hazard but I'll close this for now. |
|
You mean it was fixed in #17160 |
Updates of like priority that occur within the same browser event are guaranteed to be batched. This is an important guarantee for event emitter or subscription-like patterns where a single browser event can spawn updates on many different components, all of which need to commit consistently.
To do this, we have a heuristic of marking the first "event time" of an update inside an event. Subsequent updates in the same event will reuse the same event time.
At the end of the event, the event time should be cleared. Failing to clear the event time has the effect of "freezing" the timeline, causing weird expiration quirks.
The event time used to be cleared right before entering the render phase. This mostly works, since every update will eventually be followed by a render task. However, it has some flaws: if there's lots of other non-React main thread work in the mean time, subsequent events could get overbatched. The heuristic is also a bit fragile; it happens to be the case that every call to
requestCurrentTimeis accompanied by a render task, but that may not necessarily always be the case, creating a subtle refactor hazard.Instead of relying on the next render task to reset the current event time, this change schedules a microtask.
I didn't use an actual microtask because React does not currently require a Promise polyfill. Instead, I'm using a Scheduler task at immediate priority, which has almost the same semantics. (There's even an open question of whether we should get rid of Scheduler's "immediate" priority in favor of microtasks, but since it currently exists in the browser proposal, too, we can figure that out later.)
See #17159 for context on what prompted this PR