Remove Array.from() from hot path #19908
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
| @@ -726,8 +726,9 @@ export function accumulateSinglePhaseListeners( | |||
| inCapturePhase: boolean, | |||
| accumulateTargetOnly: boolean, | |||
| ): void { | |||
| const bubbled = event._reactName; | |||
| const captured = bubbled !== null ? bubbled + 'Capture' : null; | |||
| const bubbleName = event._reactName; | |||
There was a problem hiding this comment.
Just renames for clarity.
| if (enableCreateEventHandleAPI) { | ||
| const eventHandlerListeners = getEventHandlerListeners(currentTarget); | ||
| if (eventHandlerListeners !== null) { | ||
| eventHandlerListeners.forEach(entry => { |
There was a problem hiding this comment.
This is the change. Array.from -> Set.forEach.
There was a problem hiding this comment.
I benchmarked this before and found looping over Array.from to be much faster than Set.forEach.
There was a problem hiding this comment.
What was the benchmark? Do you mean there is a pronounced difference with the sizes we're operating (5-10 items)?
There was a problem hiding this comment.
To be clear my concern isn't the iteration speed itself (I would expect it to be dwarfed by everything else we do, considering the Set has a small size), but that we're allocating a non-empty array for each level that has these handlers, for each event as it goes upwards. This adds up.
There was a problem hiding this comment.
It probably doesn't make much difference in either case tbh. Allocating small arrays is also cheap and we don't have a good way to measure either case here. To keep things simplistic though, we can keep the forEach as you've done.
| const eventHandlerListeners = getEventHandlerListeners(currentTarget); | ||
| if (eventHandlerListeners !== null) { | ||
| eventHandlerListeners.forEach(entry => { | ||
| if (entry.type === targetType && entry.capture === inCapturePhase) { |
There was a problem hiding this comment.
This is a simplification. We don't need to do
if (a && b) {
// do thing
} else if (!a && !b) {
// do same thing
}because it is equivalent to
if (a === b) {
// do thing
}
| if (bubbleListener != null) { | ||
|
|
||
| // Standard React on* listeners, i.e. onClick or onClickCapture | ||
| if (reactEventName !== null) { |
There was a problem hiding this comment.
There is only one active event name at any given time. I read it early so no need for two conditions here.
| } | ||
| if (eventHandlerListeners !== null) { | ||
| eventHandlerListeners.forEach(entry => { | ||
| if (entry.type === targetType && entry.capture === inCapturePhase) { |
There was a problem hiding this comment.
Same change as above.
| @@ -844,8 +817,8 @@ export function accumulateTwoPhaseListeners( | |||
| dispatchQueue: DispatchQueue, | |||
| event: ReactSyntheticEvent, | |||
| ): void { | |||
| const bubbled = event._reactName; | |||
| const captured = bubbled !== null ? bubbled + 'Capture' : null; | |||
| const bubbleName = event._reactName; | |||
There was a problem hiding this comment.
Just for consistency.
|
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 aa4bf79:
|
Details of bundled changes.Comparing: c91c1c4...f82ea53 react-dom
Size changes (experimental) |
Details of bundled changes.Comparing: c91c1c4...aa4bf79 react-dom
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
* Remove Array.from() from hot path * Fix build Don't declare block variables inside loops
We were turning every createEventHandle listener Set into an array during traversal, which is not necessary because we can iterate over the Set itself instead to reduce the GC churn.
The diff is simple but indentation changed so it's a bit messy. I'll comment inline.