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

Jump to bottom

Remove Array.from() from hot path #19908

Merged
merged 2 commits into from
Sep 25, 2020
Merged

Remove Array.from() from hot path #19908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
697d3c4
Remove Array.from() from hot path
gaearon Sep 25, 2020
aa4bf79
Fix build
gaearon Sep 25, 2020
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
130 changes: 51 additions & 79 deletions packages/react-dom/src/events/DOMPluginEventSystem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just renames for clarity.

const captureName = bubbleName !== null ? bubbleName + 'Capture' : null;
const reactEventName = inCapturePhase ? captureName : bubbleName;
const listeners: Array<DispatchListener> = [];

let instance = targetFiber;
Expand All @@ -739,48 +740,34 @@ export function accumulateSinglePhaseListeners(
const {stateNode, tag} = instance;
// Handle listeners that are on HostComponents (i.e. <div>)
if (tag === HostComponent && stateNode !== null) {
const currentTarget = stateNode;
lastHostComponent = currentTarget;
// For Event Handle listeners
if (enableCreateEventHandleAPI) {
const eventHandlerlisteners = getEventHandlerListeners(currentTarget);
lastHostComponent = stateNode;

if (eventHandlerlisteners !== null) {
const eventHandlerlistenersArr = Array.from(eventHandlerlisteners);
for (let i = 0; i < eventHandlerlistenersArr.length; i++) {
const {
callback,
capture: isCapturePhaseListener,
type,
} = eventHandlerlistenersArr[i];
if (type === targetType) {
if (isCapturePhaseListener && inCapturePhase) {
listeners.push(
createDispatchListener(instance, callback, currentTarget),
);
} else if (!isCapturePhaseListener && !inCapturePhase) {
listeners.push(
createDispatchListener(instance, callback, currentTarget),
);
}
// createEventHandle listeners
if (enableCreateEventHandleAPI) {
const eventHandlerListeners = getEventHandlerListeners(
lastHostComponent,
);
if (eventHandlerListeners !== null) {
eventHandlerListeners.forEach(entry => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the change. Array.from -> Set.forEach.

Copy link
Contributor

@trueadm trueadm Sep 25, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I benchmarked this before and found looping over Array.from to be much faster than Set.forEach.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What was the benchmark? Do you mean there is a pronounced difference with the sizes we're operating (5-10 items)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (entry.type === targetType && entry.capture === inCapturePhase) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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
}

listeners.push(
createDispatchListener(
instance,
entry.callback,
(lastHostComponent: any),
),
);
}
}
}
}
// Standard React on* listeners, i.e. onClick prop
if (captured !== null && inCapturePhase) {
const captureListener = getListener(instance, captured);
if (captureListener != null) {
listeners.push(
createDispatchListener(instance, captureListener, currentTarget),
);
});
}
}
if (bubbled !== null && !inCapturePhase) {
const bubbleListener = getListener(instance, bubbled);
if (bubbleListener != null) {

// Standard React on* listeners, i.e. onClick or onClickCapture
if (reactEventName !== null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is only one active event name at any given time. I read it early so no need for two conditions here.

const listener = getListener(instance, reactEventName);
if (listener != null) {
listeners.push(
createDispatchListener(instance, bubbleListener, currentTarget),
createDispatchListener(instance, listener, lastHostComponent),
);
}
}
Expand All @@ -791,32 +778,23 @@ export function accumulateSinglePhaseListeners(
lastHostComponent !== null &&
stateNode !== null
) {
// Scopes
const reactScopeInstance = stateNode;
const eventHandlerlisteners = getEventHandlerListeners(
const eventHandlerListeners = getEventHandlerListeners(
reactScopeInstance,
);
const lastCurrentTarget = ((lastHostComponent: any): Element);

if (eventHandlerlisteners !== null) {
const eventHandlerlistenersArr = Array.from(eventHandlerlisteners);
for (let i = 0; i < eventHandlerlistenersArr.length; i++) {
const {
callback,
capture: isCapturePhaseListener,
type,
} = eventHandlerlistenersArr[i];
if (type === targetType) {
if (isCapturePhaseListener && inCapturePhase) {
listeners.push(
createDispatchListener(instance, callback, lastCurrentTarget),
);
} else if (!isCapturePhaseListener && !inCapturePhase) {
listeners.push(
createDispatchListener(instance, callback, lastCurrentTarget),
);
}
if (eventHandlerListeners !== null) {
eventHandlerListeners.forEach(entry => {
if (entry.type === targetType && entry.capture === inCapturePhase) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same change as above.

listeners.push(
createDispatchListener(
instance,
entry.callback,
(lastHostComponent: any),
),
);
}
}
});
}
}
// If we are only accumulating events for the target, then we don't
Expand Down Expand Up @@ -844,8 +822,8 @@ export function accumulateTwoPhaseListeners(
dispatchQueue: DispatchQueue,
event: ReactSyntheticEvent,
): void {
const bubbled = event._reactName;
const captured = bubbled !== null ? bubbled + 'Capture' : null;
const bubbleName = event._reactName;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just for consistency.

const captureName = bubbleName !== null ? bubbleName + 'Capture' : null;
const listeners: Array<DispatchListener> = [];
let instance = targetFiber;

Expand All @@ -856,16 +834,16 @@ export function accumulateTwoPhaseListeners(
if (tag === HostComponent && stateNode !== null) {
const currentTarget = stateNode;
// Standard React on* listeners, i.e. onClick prop
if (captured !== null) {
const captureListener = getListener(instance, captured);
if (captureName !== null) {
const captureListener = getListener(instance, captureName);
if (captureListener != null) {
listeners.unshift(
createDispatchListener(instance, captureListener, currentTarget),
);
}
}
if (bubbled !== null) {
const bubbleListener = getListener(instance, bubbled);
if (bubbleName !== null) {
const bubbleListener = getListener(instance, bubbleName);
if (bubbleListener != null) {
listeners.push(
createDispatchListener(instance, bubbleListener, currentTarget),
Expand Down Expand Up @@ -1026,20 +1004,14 @@ export function accumulateEventHandleNonManagedNodeListeners(

const eventListeners = getEventHandlerListeners(currentTarget);
if (eventListeners !== null) {
const listenersArr = Array.from(eventListeners);
const targetType = ((event.type: any): DOMEventName);

for (let i = 0; i < listenersArr.length; i++) {
const listener = listenersArr[i];
const {callback, capture: isCapturePhaseListener, type} = listener;
if (type === targetType) {
if (inCapturePhase && isCapturePhaseListener) {
listeners.push(createDispatchListener(null, callback, currentTarget));
} else if (!inCapturePhase && !isCapturePhaseListener) {
listeners.push(createDispatchListener(null, callback, currentTarget));
}
eventListeners.forEach(entry => {
if (entry.type === targetType && entry.capture === inCapturePhase) {
listeners.push(
createDispatchListener(null, entry.callback, currentTarget),
);
}
}
});
}
if (listeners.length !== 0) {
dispatchQueue.push(createDispatchEntry(event, listeners));
Expand Down
7 changes: 3 additions & 4 deletions packages/react-interactions/events/src/dom/create-event-handle/useEvent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,9 @@ export default function useEvent(
clears.set(target, clear);
},
clear(): void {
const clearsArr = Array.from(clears.values());
for (let i = 0; i < clearsArr.length; i++) {
clearsArr[i]();
}
clears.forEach(c => {
c();
});
clears.clear();
},
};
Expand Down