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] Build Updater List from the Commit instead of Map #30897

Merged
merged 2 commits into from
Sep 7, 2024
Merged

[DevTools] Build Updater List from the Commit instead of Map #30897

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b83e93
Track currentRoot FiberInstance instead of ID
sebmarkbage Sep 6, 2024
bc61bd5
updaters
sebmarkbage Sep 6, 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
89 changes: 50 additions & 39 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 @@ -1293,11 +1293,11 @@ export function attach(
'Expected the root instance to already exist when applying filters',
);
}
currentRootID = rootInstance.id;
currentRoot = rootInstance;
unmountInstanceRecursively(rootInstance);
rootToFiberInstanceMap.delete(root);
flushPendingEvents(root);
currentRootID = -1;
currentRoot = (null: any);
});

applyComponentFilters(componentFilters);
Expand All @@ -1323,11 +1323,11 @@ export function attach(
mightBeOnTrackedPath = true;
}

currentRootID = newRoot.id;
setRootPseudoKey(currentRootID, root.current);
currentRoot = newRoot;
setRootPseudoKey(currentRoot.id, root.current);
mountFiberRecursively(root.current, false);
flushPendingEvents(root);
currentRootID = -1;
currentRoot = (null: any);
});

// Also re-evaluate all error and warning counts given the new filters.
Expand Down Expand Up @@ -1528,7 +1528,7 @@ export function attach(
}

// When a mount or update is in progress, this value tracks the root that is being operated on.
let currentRootID: number = -1;
let currentRoot: FiberInstance = (null: any);

// Returns a FiberInstance if one has already been generated for the Fiber or null if one has not been generated.
// Use this method while e.g. logging to avoid over-retaining Fibers.
Expand Down Expand Up @@ -1885,7 +1885,12 @@ export function attach(
3 + pendingOperations.length,
);
operations[0] = rendererID;
operations[1] = currentRootID;
if (currentRoot === null) {
// TODO: This is not always safe so this field is probably not needed.
operations[1] = -1;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I refactored currentRoot to be the instance. I used the (null: any) since any time it is accessed it should be known to be initialized.

However, this revealed that there were a couple of place where this was read outside a context where it's set.

These used to be -1. That suggests that these fields are not actually used by the protocol since it is apparently resilient to not receiving the right value. Could be an opportunity to clean up.

} else {
operations[1] = currentRoot.id;
}
operations[2] = 0; // String table size
for (let j = 0; j < pendingOperations.length; j++) {
operations[3 + j] = pendingOperations[j];
Expand Down Expand Up @@ -2038,7 +2043,12 @@ export function attach(
// Which in turn enables fiber props, states, and hooks to be inspected.
let i = 0;
operations[i++] = rendererID;
operations[i++] = currentRootID;
if (currentRoot === null) {
// TODO: This is not always safe so this field is probably not needed.
operations[i++] = -1;
} else {
operations[i++] = currentRoot.id;
}

// Now fill in the string table.
// [stringTableLength, str1Length, ...str1, str2Length, ...str2, ...]
Expand Down Expand Up @@ -2881,6 +2891,25 @@ export function attach(
}
}
}

// If this Fiber was in the set of memoizedUpdaters we need to record
// it to be included in the description of the commit.
const fiberRoot: FiberRoot = currentRoot.data.stateNode;
const updaters = fiberRoot.memoizedUpdaters;
if (
updaters != null &&
(updaters.has(fiber) ||
// We check the alternate here because we're matching identity and
// prevFiber might be same as fiber.
(fiber.alternate !== null && updaters.has(fiber.alternate)))
) {
const metadata =
((currentCommitProfilingMetadata: any): CommitProfilingData);
if (metadata.updaters === null) {
metadata.updaters = [];
}
metadata.updaters.push(instanceToSerializedElement(fiberInstance));
}
}
}

Expand Down Expand Up @@ -3568,8 +3597,8 @@ export function attach(
if (alternate) {
fiberToFiberInstanceMap.set(alternate, newRoot);
}
currentRootID = newRoot.id;
setRootPseudoKey(currentRootID, root.current);
currentRoot = newRoot;
setRootPseudoKey(currentRoot.id, root.current);

// Handle multi-renderer edge-case where only some v16 renderers support profiling.
if (isProfiling && rootSupportsProfiling(root)) {
Expand All @@ -3581,35 +3610,20 @@ export function attach(
commitTime: getCurrentTime() - profilingStartTime,
maxActualDuration: 0,
priorityLevel: null,
updaters: getUpdatersList(root),
updaters: null,
effectDuration: null,
passiveEffectDuration: null,
};
}

mountFiberRecursively(root.current, false);

flushPendingEvents(root);
currentRootID = -1;
currentRoot = (null: any);
});
}
}

function getUpdatersList(root: any): Array<SerializedElement> | null {
const updaters = root.memoizedUpdaters;
if (updaters == null) {
return null;
}
const result = [];
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const updater of updaters) {
const inst = getFiberInstanceUnsafe(updater);
if (inst !== null) {
result.push(instanceToSerializedElement(inst));
}
}
return result;
}

function handleCommitFiberUnmount(fiber: any) {
// This Hook is no longer used. After having shipped DevTools everywhere it is
// safe to stop calling it from Fiber.
Expand Down Expand Up @@ -3646,11 +3660,10 @@ export function attach(
if (alternate) {
fiberToFiberInstanceMap.set(alternate, rootInstance);
}
currentRootID = rootInstance.id;
} else {
currentRootID = rootInstance.id;
prevFiber = rootInstance.data;
}
currentRoot = rootInstance;

// Before the traversals, remember to start tracking
// our path in case we have selection to restore.
Expand All @@ -3675,9 +3688,7 @@ export function attach(
maxActualDuration: 0,
priorityLevel:
priorityLevel == null ? null : formatPriorityLevel(priorityLevel),

updaters: getUpdatersList(root),

updaters: null,
// Initialize to null; if new enough React version is running,
// these values will be read during separate handlePostCommitFiberRoot() call.
effectDuration: null,
Expand All @@ -3699,28 +3710,28 @@ export function attach(
current.memoizedState.isDehydrated !== true;
if (!wasMounted && isMounted) {
// Mount a new root.
setRootPseudoKey(currentRootID, current);
setRootPseudoKey(currentRoot.id, current);
mountFiberRecursively(current, false);
} else if (wasMounted && isMounted) {
// Update an existing root.
updateFiberRecursively(rootInstance, current, prevFiber, false);
} else if (wasMounted && !isMounted) {
// Unmount an existing root.
unmountInstanceRecursively(rootInstance);
removeRootPseudoKey(currentRootID);
removeRootPseudoKey(currentRoot.id);
rootToFiberInstanceMap.delete(root);
}
} else {
// Mount a new root.
setRootPseudoKey(currentRootID, current);
setRootPseudoKey(currentRoot.id, current);
mountFiberRecursively(current, false);
}

if (isProfiling && isProfilingSupported) {
if (!shouldBailoutWithPendingOperations()) {
const commitProfilingMetadata =
((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).get(
currentRootID,
currentRoot.id,
);

if (commitProfilingMetadata != null) {
Expand All @@ -3729,7 +3740,7 @@ export function attach(
);
} else {
((rootToCommitProfilingMetadataMap: any): CommitProfilingMetadataMap).set(
currentRootID,
currentRoot.id,
[((currentCommitProfilingMetadata: any): CommitProfilingData)],
);
}
Expand All @@ -3743,7 +3754,7 @@ export function attach(
hook.emit('traceUpdates', traceUpdatesForNodes);
}

currentRootID = -1;
currentRoot = (null: any);
}

function getResourceInstance(fiber: Fiber): HostInstance | null {
Expand Down
Loading