Effects list refactor continued: passive effects traversal #19374
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit ae37c7c:
|
Details of bundled changes.Comparing: 0eea166...ae37c7c react-dom
Size changes (experimental) |
Details of bundled changes.Comparing: 0eea166...ae37c7c react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
|
Is the delta link out of date? I see no changes. |
|
@gaearon Whoops, the other two PRs have landed now 😄 You can just view it normally (although please keep in mind it's not ready for review, just a draft). I'll delete that line though. |
9b894fe
to
c536679
Compare
|
Okay. This PR is ready for review now 😄 |
352b88b
to
d96907c
Compare
d96907c
to
9b91189
Compare
9b91189
to
157aa81
Compare
5b6bc9d
to
15288ed
Compare
| // mark the parent so that we're sure to traverse after commit and run any unmount functions. | ||
| const primaryEffectTag = childToDelete.effectTag & PassiveMask; | ||
| const primarySubtreeTag = childToDelete.subtreeTag & PassiveSubtreeTag; | ||
| if (primaryEffectTag !== NoEffect || primarySubtreeTag !== NoSubtreeTag) { |
There was a problem hiding this comment.
Maybe this optimization isn't worth it and we should always set the PassiveSubtreeTag on the parent when a child is deleted. i.e. add Deletion to PassiveMask.
Effects are very common — any large tree is almost certain to have at least one. So the optimization really only kicks in when you delete a leaf node. Not sure it's worth the extra code.
I'm almost tempted to say that PassiveStatic isn't worth it for the same reason; but since we're planning to have other static tags for things like portals, PassiveStatic doesn't add any net additional overhead, so might as well keep it.
What do you think?
There was a problem hiding this comment.
Maybe this optimization isn't worth it and we should always set the PassiveSubtreeTag on the parent when a child is deleted.
Given that the concept of static flags seems like a positive– and so keeping PassiveStatic around doesn't add any net overhead– the only question seems to be whether the few extra bytes required for us to check effectTag and subtreeTag are worth it, yeah?
With the current check, we might over traverse in the event that a child was deleted that had a passive effect, but no cleanup function. (Seems unlikely.) If we removed this check, we might over traverse any time a leaf node is deleted, period. Still, I guess that wouldn't be that bad since we'd only traverse the path to where the leaf was deleted.
TBH I think the cost is pretty small on either side (number or bytes or potential over traversing). Happy to remove the check for now.
There was a problem hiding this comment.
If we removed this check, we might over traverse any time a leaf node is deleted, period. Still, I guess that wouldn't be that bad since we'd only traverse the path to where the leaf was deleted.
Yeah I think that's fine. It's also related to the detachFiber discussion. Since we're moving most of that work to the passive phase, then we need to visit every deletion during the passive phase anyway.
There was a problem hiding this comment.
That's a good point I guess. I hadn't considered that aspect.
I consider the "detach" methods as kind of an optional optimization.
| // Note that we can't clear child or sibling pointers yet, | ||
| // because they may be required for passive effects. | ||
| // These pointers will be cleared in a separate pass. | ||
| // Ideally, we should clear the child pointer of the parent alternate to let this |
There was a problem hiding this comment.
If a Deletion always gets a passive effect scheduled on it, as suggested above, we could move this whole function to the passive phase. (Except maybe return and alternate, since those affect semantics, like if you call setState on an unmounted component.)
There was a problem hiding this comment.
If we don't clear return during layout, it would break a few things:
- The unmounted state warning
- How events bubble for the new API Dominic has been working on
That being said, it would fix a limitation of the Profiler API when it comes to measuring time spent in passive effect cleanup. That time currently doesn't get reported during an unmount (either a regular unmount or because of an error boundary) because the return pointer has been detached previously and so it can't bubble up to the nearest Profiler node. If we deferred detaching it until after passive, that time would be reported which is a plus.
Not sure if there would be a viable workaround for the event case, but for the unmounted state warning- we could walk the return path and look for Fibers with Deletion effects scheduled, except that this would have false positives for deletions that were scheduled and not committed so that's a bummer.
I guess we could always set another bit to mark a Fiber as having been deleted but at that point we aren't really saving much (aside from the Profiler fix).
There was a problem hiding this comment.
That being said, I believe we can safely defer everything except the return pointer cleanup to the passive effects phase which would be slightly better.
* Adds new Passive subtree tag value. * Bubbles passive flag to ancestors in the case of an unmount. * Adds recursive traversal for passive effects (mounts and unmounts). * Removes pendingPassiveHookEffectsMount and pendingPassiveHookEffectsUnmount arrays from work loop. * Re-adds sibling and child pointer detaching (temporarily removed in previous PR). * Addresses some minor TODO comments left over from previous PRs.
This allows certain concepts to persist without recalculting them, e.g. whether a subtree contains passive effects or portals. This helps for cases like nested unmounts that contain deep passive effects.
Passivesubtree tag value.pendingPassiveHookEffectsMountandpendingPassiveHookEffectsUnmountarrays from work loop.Co-authored-by: Luna Ruan [email protected]