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.

Sign up for GitHub

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

[DevTools] Reconcile Fibers Against Previous Children Instances #30822

Merged
merged 2 commits into from
Aug 27, 2024
Merged

[DevTools] Reconcile Fibers Against Previous Children Instances #30822

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6997b7e
Find previous instances in the remaining children instead of Fiber map
sebmarkbage Aug 27, 2024
dde6c87
Remove previous sibling field and instead use it from the loop we jus…
sebmarkbage Aug 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 90 additions & 78 deletions packages/react-devtools-shared/src/backend/fiber/renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,6 @@ type FiberInstance = {
id: number,
parent: null | DevToolsInstance, // filtered parent, including virtual
firstChild: null | DevToolsInstance, // filtered first child, including virtual
previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual
nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual
flags: number, // Force Error/Suspense
componentStack: null | string,
Expand All @@ -164,7 +163,6 @@ function createFiberInstance(fiber: Fiber): FiberInstance {
id: getUID(),
parent: null,
firstChild: null,
previousSibling: null,
nextSibling: null,
flags: 0,
componentStack: null,
Expand All @@ -184,7 +182,6 @@ type VirtualInstance = {
id: number,
parent: null | DevToolsInstance, // filtered parent, including virtual
firstChild: null | DevToolsInstance, // filtered first child, including virtual
previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual
nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual
flags: number,
componentStack: null | string,
Expand All @@ -206,7 +203,6 @@ function createVirtualInstance(
id: getUID(),
parent: null,
firstChild: null,
previousSibling: null,
nextSibling: null,
flags: 0,
componentStack: null,
Expand Down Expand Up @@ -1075,8 +1071,6 @@ export function attach(
' '.repeat(indent) + '- ' + instance.id + ' (' + name + ')',
'parent',
instance.parent === null ? ' ' : instance.parent.id,
'prev',
instance.previousSibling === null ? ' ' : instance.previousSibling.id,
'next',
instance.nextSibling === null ? ' ' : instance.nextSibling.id,
);
Expand Down Expand Up @@ -2314,30 +2308,32 @@ export function attach(
if (previouslyReconciledSibling === null) {
previouslyReconciledSibling = instance;
parentInstance.firstChild = instance;
instance.previousSibling = null;
} else {
previouslyReconciledSibling.nextSibling = instance;
instance.previousSibling = previouslyReconciledSibling;
previouslyReconciledSibling = instance;
}
instance.nextSibling = null;
}

function moveChild(instance: DevToolsInstance): void {
removeChild(instance);
function moveChild(
instance: DevToolsInstance,
previousSibling: null | DevToolsInstance,
): void {
removeChild(instance, previousSibling);
insertChild(instance);
}

function removeChild(instance: DevToolsInstance): void {
function removeChild(
instance: DevToolsInstance,
previousSibling: null | DevToolsInstance,
): void {
if (instance.parent === null) {
if (remainingReconcilingChildren === instance) {
throw new Error(
'Remaining children should not have items with no parent',
);
} else if (instance.nextSibling !== null) {
throw new Error('A deleted instance should not have next siblings');
} else if (instance.previousSibling !== null) {
throw new Error('A deleted instance should not have previous siblings');
}
// Already deleted.
return;
Expand All @@ -2353,7 +2349,7 @@ export function attach(
}
// Remove an existing child from its current position, which we assume is in the
// remainingReconcilingChildren set.
if (instance.previousSibling === null) {
if (previousSibling === null) {
// We're first in the remaining set. Remove us.
if (remainingReconcilingChildren !== instance) {
throw new Error(
Expand All @@ -2362,13 +2358,9 @@ export function attach(
}
remainingReconcilingChildren = instance.nextSibling;
} else {
instance.previousSibling.nextSibling = instance.nextSibling;
}
if (instance.nextSibling !== null) {
instance.nextSibling.previousSibling = instance.previousSibling;
previousSibling.nextSibling = instance.nextSibling;
}
instance.nextSibling = null;
instance.previousSibling = null;
instance.parent = null;
}

Expand Down Expand Up @@ -2652,7 +2644,7 @@ export function attach(
} else {
recordVirtualUnmount(instance);
}
removeChild(instance);
removeChild(instance, null);
}

function recordProfilingDurations(fiber: Fiber) {
Expand Down Expand Up @@ -2851,8 +2843,7 @@ export function attach(
);
}
}
// TODO: Find the best matching existing child based on the key if defined.

let previousSiblingOfBestMatch = null;
let bestMatch = remainingReconcilingChildren;
if (componentInfo.key != null) {
// If there is a key try to find a matching key in the set.
Expand All @@ -2864,6 +2855,7 @@ export function attach(
) {
break;
}
previousSiblingOfBestMatch = bestMatch;
bestMatch = bestMatch.nextSibling;
}
}
Expand All @@ -2878,7 +2870,7 @@ export function attach(
// with the same name, then we claim it and reuse it for this update.
// Update it with the latest entry.
bestMatch.data = componentInfo;
moveChild(bestMatch);
moveChild(bestMatch, previousSiblingOfBestMatch);
previousVirtualInstance = bestMatch;
previousVirtualInstanceWasMount = false;
} else {
Expand Down Expand Up @@ -2927,42 +2919,93 @@ export function attach(
}
previousVirtualInstance = null;
}

// We've reached the end of the virtual levels, but not beyond,
// and now continue with the regular fiber.

// Do a fast pass over the remaining children to find the previous instance.
// TODO: This doesn't have the best O(n) for a large set of children that are
// reordered. Consider using a temporary map if it's not the very next one.
let prevChild;
if (prevChildAtSameIndex === nextChild) {
// This set is unchanged. We're just going through it to place all the
// children again.
prevChild = nextChild;
} else {
// We don't actually need to rely on the alternate here. We could also
// reconcile against stateNode, key or whatever. Doesn't have to be same
// Fiber pair.
prevChild = nextChild.alternate;
}
let previousSiblingOfExistingInstance = null;
let existingInstance = null;
if (prevChild !== null) {
existingInstance = remainingReconcilingChildren;
while (existingInstance !== null) {
if (existingInstance.data === prevChild) {
break;
}
previousSiblingOfExistingInstance = existingInstance;
existingInstance = existingInstance.nextSibling;
}
}
if (existingInstance !== null) {
// Common case. Match in the same parent.
const fiberInstance: FiberInstance = (existingInstance: any); // Only matches if it's a Fiber.

// We keep track if the order of the children matches the previous order.
// They are always different referentially, but if the instances line up
// conceptually we'll want to know that.
if (prevChild !== prevChildAtSameIndex) {
shouldResetChildren = true;
}

// Register the new alternate in case it's not already in.
fiberToFiberInstanceMap.set(nextChild, fiberInstance);

// Update the Fiber so we that we always keep the current Fiber on the data.
fiberInstance.data = nextChild;
moveChild(fiberInstance, previousSiblingOfExistingInstance);

if (
updateFiberRecursively(
fiberInstance,
nextChild,
nextChild,
(prevChild: any),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be gated with if (prevChild !== null) or prevChild made optional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can only get here if we have a non-null prevChild since otherwise existingInstance will be null.

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It helps avoid an annoying else branching.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can only get here if we have a non-null prevChild since otherwise existingInstance will be null.

Yeah, now I get it

traceNearestHostComponentUpdate,
)
) {
throw new Error('Updating the same fiber should not cause reorder');
// If a nested tree child order changed but it can't handle its own
// child order invalidation (e.g. because it's filtered out like host nodes),
// propagate the need to reset child order upwards to this Fiber.
shouldResetChildren = true;
}
} else if (nextChild.alternate) {
const prevChild = nextChild.alternate;
} else if (prevChild !== null && shouldFilterFiber(nextChild)) {
// If this Fiber should be filtered, we need to still update its children.
// This relies on an alternate since we don't have an Instance with the previous
// child on it. Ideally, the reconciliation wouldn't need previous Fibers that
// are filtered from the tree.
if (
updateFiberRecursively(
null,
nextChild,
prevChild,
traceNearestHostComponentUpdate,
)
) {
// If a nested tree child order changed but it can't handle its own
// child order invalidation (e.g. because it's filtered out like host nodes),
// propagate the need to reset child order upwards to this Fiber.
shouldResetChildren = true;
}
// However we also keep track if the order of the children matches
// the previous order. They are always different referentially, but
// if the instances line up conceptually we'll want to know that.
if (prevChild !== prevChildAtSameIndex) {
shouldResetChildren = true;
}
} else {
// It's possible for a FiberInstance to be reparented when virtual parents
// get their sequence split or change structure with the same render result.
// In this case we unmount the and remount the FiberInstances.
// This might cause us to lose the selection but it's an edge case.

// We let the previous instance remain in the "remaining queue" it is
// in to be deleted at the end since it'll have no match.

mountFiberRecursively(nextChild, traceNearestHostComponentUpdate);
// Need to mark the parent set to remount the new instance.
shouldResetChildren = true;
}
}
Expand Down Expand Up @@ -3021,6 +3064,7 @@ export function attach(

// Returns whether closest unfiltered fiber parent needs to reset its child list.
function updateFiberRecursively(
fiberInstance: null | FiberInstance, // null if this should be filtered
nextFiber: Fiber,
prevFiber: Fiber,
traceNearestHostComponentUpdate: boolean,
Expand Down Expand Up @@ -3054,34 +3098,10 @@ export function attach(
}
}

let fiberInstance: null | FiberInstance = null;
const shouldIncludeInTree = !shouldFilterFiber(nextFiber);
if (shouldIncludeInTree) {
const entry = fiberToFiberInstanceMap.get(prevFiber);
if (entry !== undefined && entry.parent === reconcilingParent) {
// Common case. Match in the same parent.
fiberInstance = entry;
// Register the new alternate in case it's not already in.
fiberToFiberInstanceMap.set(nextFiber, fiberInstance);

// Update the Fiber so we that we always keep the current Fiber on the data.
fiberInstance.data = nextFiber;
moveChild(fiberInstance);
} else {
// It's possible for a FiberInstance to be reparented when virtual parents
// get their sequence split or change structure with the same render result.
// In this case we unmount the and remount the FiberInstances.
// This might cause us to lose the selection but it's an edge case.

// We let the previous instance remain in the "remaining queue" it is
// in to be deleted at the end since it'll have no match.

mountFiberRecursively(nextFiber, traceNearestHostComponentUpdate);

// Need to mark the parent set to remount the new instance.
return true;
}

const stashedParent = reconcilingParent;
const stashedPrevious = previouslyReconciledSibling;
const stashedRemaining = remainingReconcilingChildren;
if (fiberInstance !== null) {
if (
mostRecentlyInspectedElement !== null &&
mostRecentlyInspectedElement.id === fiberInstance.id &&
Expand All @@ -3091,12 +3111,6 @@ export function attach(
// If it is inspected again, it may need to be re-run to obtain updated hooks values.
hasElementUpdatedSinceLastInspected = true;
}
}

const stashedParent = reconcilingParent;
const stashedPrevious = previouslyReconciledSibling;
const stashedRemaining = remainingReconcilingChildren;
if (fiberInstance !== null) {
// Push a new DevTools instance parent while reconciling this subtree.
reconcilingParent = fiberInstance;
previouslyReconciledSibling = null;
Expand Down Expand Up @@ -3151,7 +3165,7 @@ export function attach(
if (
nextFallbackChildSet != null &&
prevFallbackChildSet != null &&
updateFiberRecursively(
updateChildrenRecursively(
nextFallbackChildSet,
prevFallbackChildSet,
traceNearestHostComponentUpdate,
Expand Down Expand Up @@ -3236,7 +3250,7 @@ export function attach(
}
}

if (shouldIncludeInTree) {
if (fiberInstance !== null) {
const isProfilingSupported =
nextFiber.hasOwnProperty('treeBaseDuration');
if (isProfilingSupported) {
Expand All @@ -3246,10 +3260,8 @@ export function attach(
if (shouldResetChildren) {
// We need to crawl the subtree for closest non-filtered Fibers
// so that we can display them in a flat children set.
if (shouldIncludeInTree) {
if (reconcilingParent !== null) {
recordResetChildren(reconcilingParent);
}
if (fiberInstance !== null) {
recordResetChildren(fiberInstance);
// We've handled the child order change for this Fiber.
// Since it's included, there's no need to invalidate parent child order.
return false;
Expand All @@ -3261,7 +3273,7 @@ export function attach(
return false;
}
} finally {
if (shouldIncludeInTree) {
if (fiberInstance !== null) {
unmountRemainingChildren();
reconcilingParent = stashedParent;
previouslyReconciledSibling = stashedPrevious;
Expand Down Expand Up @@ -3451,7 +3463,7 @@ export function attach(
mountFiberRecursively(current, false);
} else if (wasMounted && isMounted) {
// Update an existing root.
updateFiberRecursively(current, alternate, false);
updateFiberRecursively(rootInstance, current, alternate, false);
} else if (wasMounted && !isMounted) {
// Unmount an existing root.
removeRootPseudoKey(currentRootID);
Expand Down
Loading