Skip to content

[DevTools] Ignore repeated removals of the same IO #34495

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

Merged
merged 2 commits into from
Sep 16, 2025
Merged

[DevTools] Ignore repeated removals of the same IO #34495

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a66800
[DevTools] Ignore repeated removals of the same IO
eps1lon Sep 15, 2025
7a543ad
Check an Array instead of a Set
eps1lon Sep 16, 2025
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
15 changes: 14 additions & 1 deletion 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 @@ -2902,9 +2902,22 @@ export function attach(
// Let's remove it from the parent SuspenseNode.
const ioInfo = asyncInfo.awaited;
const suspendedBySet = parentSuspenseNode.suspendedBy.get(ioInfo);
// A boundary can await the same IO multiple times.
// We still want to error if we're trying to remove IO that isn't present on
// this boundary so we need to check if we've already removed it.
// We're assuming previousSuspendedBy is a small array so this should be faster
// than allocating and maintaining a Set.
let alreadyRemovedIO = false;
for (let j = 0; j < i; j++) {
const removedIOInfo = previousSuspendedBy[j].awaited;
if (removedIOInfo === ioInfo) {
alreadyRemovedIO = true;
break;
}
}
if (
suspendedBySet === undefined ||
!suspendedBySet.delete(instance)
(!alreadyRemovedIO && !suspendedBySet.delete(instance))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally we'd check if !suspendedBySet.delete(instance) first before doing the loop so that in the common case when we do succeed in deleting it, there's no reason to check.

) {
throw new Error(
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could just check previousSuspendedBy.indexOf(asyncInfo) < i.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Instead of what? The async info in suspendedBy is unique in this case. It's the IO that's not.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Then you can do it one level below with a custom loop walking to this index. Still likely faster to check a small set like this in the existing array than even just running the .has(). Let alone allocating and maintaining the set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

7a543ad (#34495)

'We are cleaning up async info that was not on the parent Suspense boundary. ' +
Expand Down
Loading