Legacy Suspense is weird. We intentionally commit a suspended fiber in
an inconsistent state. If the fiber suspended before it mounted any
effects, then the fiber won't have a PassiveStatic effect flag, which
will trigger the "missing expected subtreeFlag" warning.
To avoid the false positive, we'd need to mark fibers that commit in an
incomplete state, somehow. For now I'll disable the warning in legacy
mode, with the assumption that most of the bugs that would trigger it
are either exclusive to concurrent mode or exist in both.
This reverts commit 8ed0c85bf174ce6e501be62d9ccec1889bbdbce1.
The host tree is a cyclical structure. Leaking a single DOM node can
retain a large amount of memory. React-managed DOM nodes also point
back to a fiber tree.
Perf testing suggests that disconnecting these fields has a big memory
impact. That suggests leaks in non-React code but since it's hard to
completely eliminate those, it may still be worth the extra work to
clear these fields.
I'm moving this to level 2 to confirm whether this alone is responsible
for the memory savings, or if there are other fields that are retaining
large amounts of memory.
In our plan for removing the alternate model, DOM nodes would not be
connected to fibers, except at the root of the whole tree, which is
easy to disconnect on deletion. So in that world, we likely won't have
to do any additional work.
This reverts commit 0e3c7e1d62efb6238b69e5295d45b9bd2dcf9181.
When called from inside an effect, flushSync cannot synchronously flush
its updates because React is already working. So we fire a warning.
However, we should still change the priority of the updates to sync so
that they flush at the end of the current task.
This only affects useEffect because updates inside useLayoutEffect (and
the rest of the commit phase, like ref callbacks) are already sync.
This reverts commit 2e7aceeb5c8b6e5c61174c0e9731e263e956e445.
If a discrete render results in passive effects, we should flush them
synchronously at the end of the current task so that the result is
immediately observable. For example, if a passive effect adds an event
listener, the listener will be added before the next input.
We don't need to do this for effects that don't have discrete/sync
priority, because we assume they are not order-dependent and do not
need to be observed by external systems.
For legacy mode, we will maintain the existing behavior, since it hasn't
been reported as an issue, and we'd have to do additional work to
distinguish "legacy default sync" from "discrete sync" to prevent all
passive effects from being treated this way.
This reverts commit faa1e127f1ba755da846bc6ce299cdefaf97721f.
* Support nesting of startTransition and flushSync
* Unset transition before entering any special execution contexts
Co-authored-by: Andrew Clark <git@andrewclark.io>
We have a feature called "expiration" whose purpose is to prevent
a concurrent update from being starved by higher priority events.
If a lane is CPU-bound for too long, we finish the rest of the work
synchronously without allowing further interruptions.
In the current implementation, we do this in sort of a roundabout way:
once a lane is determined to have expired, we entangle it with SyncLane
and switch to the synchronous work loop.
There are a few flaws with the approach. One is that SyncLane has a
particular semantic meaning besides its non-yieldiness. For example,
`flushSync` will force remaining Sync work to finish; currently, that
also includes expired work, which isn't an intended behavior, but rather
an artifact of the implementation.
An event worse example is that passive effects triggered by a Sync
update are flushed synchronously, before paint, so that its result
is guaranteed to be observed by the next discrete event. But expired
work has no such requirement: we're flushing expired effects before
paint unnecessarily.
Aside from the behaviorial implications, the current implementation has
proven to be fragile: more than once, we've accidentally regressed
performance due to a subtle change in how expiration is handled.
This PR aims to radically simplify how we model starvation protection by
scaling back the implementation as much as possible. In this new model,
if a lane is expired, we disable time slicing. That's it. We don't
entangle it with SyncLane. The only thing we do is skip the call to
`shouldYield` in between each time slice. This is identical to how we
model synchronous-by-default updates in React 18.
The outermost `batchedUpdates` call flushes pending sync updates at the
end. This was intended for legacy sync mode, but it also happens to
flush discrete updates in concurrent mode.
Instead, we should only flush sync updates at the end of
`batchedUpdates` for legacy roots. Discrete sync updates can wait to
flush in the microtask.
`discreteUpdates` has the same issue, which is how I originally noticed
this, but I'll change that one in a separate commit since it requires
updating a few (no longer relevant) internal tests.
Even when updates are sync by default.
Discovered this quirk while working on #21322. Previously, when sync
default updates are enabled, continuous updates are treated like
default updates. We implemented this by assigning DefaultLane to
continous updates. However, an unintended consequence of that approach
is that continuous updates would no longer interrupt transitions,
because default updates are not supposed to interrupt transitions.
To fix this, I changed the implementation to always assign separate
lanes for default and continuous updates. Then I entangle the
lanes together.
Instead of `performSyncWorkOnRoot`.
The conceptual model is that the only difference between sync default
updates (in React 18) and concurrent default updates (in a future major
release) is time slicing. All other behavior should be the same
(i.e. the stuff in `finishConcurrentRender`).
Given this, I think it makes more sense to model the implementation this
way, too. This exposed a quirk in the previous implementation where
non-sync work was sometimes mistaken for sync work and flushed too
early. In the new implementation, `performSyncWorkOnRoot` is only used
for truly synchronous renders (i.e. `SyncLane`), which should make these
mistakes less common.
Fixes most of the tests marked with TODOs from #21072.
* Warn if `finishedLanes` is empty in commit phase
See #21233 for context.
* Fix sloppy factoring when assigning finishedLanes
`finishedLanes` is assigned in `performSyncWorkOnRoot` and
`performSyncWorkOnRoot`. It's meant to represent whichever lanes we
used to render, but because of some sloppy factoring, it can sometimes
equal `NoLanes`.
The fixes are:
- Always check if the lanes are not `NoLanes` before entering the work
loop. There was a branch where this wasn't always true.
- In `performSyncWorkOnRoot`, don't assume the next lanes are sync; the
priority may have changed, or they may have been flushed by a
previous task.
- Don't re-assign the `lanes` variable (the one that gets assigned to
`finishedLanes` until right before we enter the work loop, so that it
is always corresponds to the newest complete root.
Retries should be allowed to expire if they are CPU bound for too long,
but when I made this change it caused a spike in browser crashes. There
must be some other underlying bug; not super urgent but ideally should
figure out why and fix it. Unfortunately we don't have a repro for the
crashes, only detected via production metrics.
I recently started using `pendingPassiveEffectsLanes` to check if there were any pending
passive effects (530027a). `pendingPassiveEffectsLanes` is the value of
`root.finishedLanes` at the beginning of the commit phase. When there
are pending passive effects, it should always be a non-zero value,
because it represents the lanes used to render the effects.
But it turns out that `root.finishedLanes` isn't always correct.
Sometimes it's `NoLanes` even when there's a new commit.
I found this while investigating an internal bug report. The only repro
I could get was via a headless e2e test runner; I couldn't get one in an
actual browser, or other interactive environment. I used the e2e test to
bisect and confirm the fix. But I don't know yet know how to write a
regression test for the precise underlying scenario. I can probably
reverse engineer one by studying the code; after a quick glance
at `performConcurrentWorkOnRoot` and `performSyncWorkOnRoot`, it's not
hard to see how this might happen.
In the meantime, I'll revert the recent change that exposed the bug.
I was surprised that this had never come up before, since the code that
assigns `root.finishedLanes` is in an extremely hot path, and it hasn't
changed in a while. The reason is that, before 530027a,
`root.finishedLanes` was only used by the DevTools profiler, which is
probably why we had never noticed any issues. In addition to fixing the
inconsistency, we might also consider making `finishedLanes` a
profiling-only field.
We assume that isArray and getIteratorFn are only called on objects.
So we shouldn't have to check that again and again, and then check a flag.
We can just stay in this branch.
There is a slight semantic breakage here because you could have an
iterator on a function, such as if it's a generator function. But that's
not supported and that currently only works at the root. The inner slots
don't support this.
So this just makes it consistent.
Tracked Fibers are called "updaters" and are exposed to DevTools via a 'memoizedUpdaters' property on the ReactFiberRoot. The implementation of this feature follows a vaguely similar approach as interaction tracing, but does not require reference counting since there is no subscriptions API.
This change is in support of a new DevTools Profiler feature that shows which Fiber(s) scheduled the selected commit in the Profiler.
All changes have been gated behind a new feature flag, 'enableUpdaterTracking', which is enabled for Profiling builds by default. We also only track updaters when DevTools has been detected, to avoid doing unnecessary work.
I recently added UI for the Profiler's commit and post-commit durations to the DevTools, but I made two pretty silly oversights:
1. I used the commit hook (called after mutation+layout effects) to read both the layout and passive effect durations. This is silly because passive effects may not have flushed yet git at this point.
2. I didn't reset the values on the HostRoot node, so they accumulated with each commit.
This commitR addresses both issues:
1. First it adds a new DevTools hook, onPostCommitRoot*, to be called after passive effects get flushed. This gives DevTools the opportunity to read passive effect durations (if the build of React being profiled supports it).
2. Second the work loop resets these durations (on the HostRoot) after calling the post-commit hook so address the accumulation problem.
I've also added a unit test to guard against this regressing in the future.
* Doing this in flushPassiveEffectsImpl seemed simplest, since there are so many places we flush passive effects. Is there any potential problem with this though?
I screwed this up in #21082. Got confused by the < versus > thing again.
The helper functions are annoying, too, because I always forget the
intended order of the arguments. But they're still helpful because when
we refactor the type we only have the change the logic in one place.
Added a regression test.
We've just initialized the update queue above this and there's no user
code that executes between.
The general API that prevents this from mattering is that you can't
call setState in the constructor.
* Remove redundant initial of isArray (#21163)
* Reapply prettier
* Type the isArray function with refinement support
This ensures that an argument gets refined just like it does if isArray is
used directly.
I'm not sure how to express with just a direct reference so I added a
function wrapper and confirmed that this does get inlined properly by
closure compiler.
* A few more
* Rename unit test to internal
This is not testing a bundle.
Co-authored-by: Behnam Mohammadi <itten@live.com>
This commit contains a proposed change to layout effect semantics within Suspense subtrees: If a component mounts within a Suspense boundary and is later hidden (because of something else suspending) React will cleanup that component’s layout effects (including React-managed refs).
This change will hopefully fix existing bugs that occur because of things like reading layout in a hidden tree and will also enable a point at which to e.g. pause videos and hide user-managed portals. After the suspended boundary resolves, React will setup the component’s layout effects again (including React-managed refs).
The scenario described above is not common. The useTransition API should ensure that Suspense does not revert to its fallback state after being mounted.
Note that these changes are primarily written in terms of the (as of yet internal) Offscreen API as we intend to provide similar effects semantics within recently shown/hidden Offscreen trees in the future. (More to follow.)
(Note that all changes in this PR are behind a new feature flag, enableSuspenseLayoutEffectSemantics, which is disabled for now.)
Ricky
Brian VaughnandiChenLei
Andrew Clark
Sebastian Markbåge
inokawa
Sebastian Silbermann
wangao
