-
Notifications
You must be signed in to change notification settings - Fork 49.6k
[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.
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] Ignore repeated removals of the same IO #34495
Conversation
!suspendedBySet.delete(instance) | ||
(!removedIOInfos.has(ioInfo) && !suspendedBySet.delete(instance)) | ||
) { | ||
throw new Error( |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( | ||
suspendedBySet === undefined || | ||
!suspendedBySet.delete(instance) | ||
(!alreadyRemovedIO && !suspendedBySet.delete(instance)) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perf optimization would be good for the common case.
The alternative would be to make
instance.suspendedBy
unique on IO instead of async info:react/packages/react-devtools-shared/src/backend/fiber/renderer.js
Lines 2833 to 2835 in 47664de