DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

nhunzaker · 2017-11-14T02:02:56Z

This PR replaces the traditional event delegation technique employed by React with direct attachment to DOM nodes. This is just an investigation. I'm attracted to it for a number of reasons:

We can remove a lot of code
This is another way of addressing scroll jank issues (Fix wheel/touch browser locking in IE and Safari #9333)
This might fix some issues with disabled interactive element bubbling. I do not know.
I might discover a nice middle-ground that is ultimately better.

Intuitively, event delegation should be faster. It does less work. However I want to figure out how much faster it really is, particularly on weaker devices and IE 9/10. I'm curious about:

Start up time
Memory usage
Ongoing update costs

I put up a build here:

production:
http://react-local-listeners.surge.sh/react.production.min.js
http://react-local-listeners.surge.sh/react-dom.production.min.js

development:
http://react-local-listeners.surge.sh/react.development.js
http://react-local-listeners.surge.sh/react-dom.development.js

Now to find some benchmarks.

This commit replaces the traditional event delegation technique employed by React with direct attachment to DOM nodes. This is a test case to evaluate performance differences.

syranide · 2017-11-14T18:51:06Z

Not sure how you've implemented it, but it might be worth still having a proxy function in-between and not attaching the user-function immediately (so that updates don't interact with the DOM, which may be common due to binding).

nhunzaker · 2017-11-15T13:31:23Z

@syranide Totally. The proxy still exists. This actually just tells React to bind the proxy to local elements instead of the owner document. The actual event system change was surprisingly small.

Right now there are a few pieces of overhead that are making local attachment take more time that are reasonable to address:

binding all events to a top level type. I think we can get rid of this and just use the event type coming from the native event. Before that can work, we need to update SimulateEvent to pass the correct event type (Set correct event type into SimulateNative methods #11563).
Unnecessary work in EventListener.{listen,capture}. These utilities return an object with an unsubscribe method that isn't getting used by React.
Generally processing listener aliases for every single event subscription in ReactBrowserEventEmitter.

Fortunately, I think these are easy to fix and could even existing improvements for React (maybe not 3). Attaching more listeners is just going to be more expensive on startup. Hopefully I can prove that it's not that much more expensive, and that gains are made in other areas.

jquense

this so cool! We should make sure to do some benchmarking with events that have a few dependencies, i.e. ones that will attach multiple listeners per element.

Small side thing, maybe it makes sense to close over instances in the handlers instead of mapping back from nativeEvent.currentTarget which may be wonky in some cases? Some obvious concerns with GC there, but thought i'd bring it up

jquense · 2017-11-15T14:00:06Z

packages/react-dom/src/client/ReactDOMFiberComponent.js

-      break;
-    case 'form':
-      trapBubbledEvent('topReset', 'reset', domElement);
-      trapBubbledEvent('topSubmit', 'submit', domElement);


for these, is there a reason why we were calling them out specifically here vs in the ensureListeningTo above? Do they need to probably be attached like onChange now?

As far as I can tell, this is just a pre-emptive attachment because ensureListeningTo attaches on the owner document, not the DOM element. We need these here because they don't bubble, so there must be a listener on the element for the synthetic event system to know about it.

I think there is a change we could send to master here. We can attach reset, submit, load, etc... directly on the element. I do not think the savings is substantial because these events are almost only ever added to these elements (when was the last time you attached onError on a div or button?)

I'm going to check to see if we still need onChange to attach here.

☝️ edited with some clarification.

Made it consistent in 68b42c2

ah so since everything is being attached directly there is no need to handle it here anymore 👍 right

jquense · 2017-11-15T14:02:25Z

packages/react-dom/src/events/ReactBrowserEventEmitter.js

+  topMouseOut: 2,
+  topMouseEnter: 3,
+  topMouseLeave: 4,
+};


why do these need delegation? I wonder what the likelihood of this growing annoyingly is?

We need this so that the EnterLeavePlugin event plugin works with elements React isn't watching.

blah that's annoying, this should need to include topMouseEnter and topMouseLeave, at the moment we don't support the native event (we always polyfill it) and I think even if we did it wouldn't be needed, since those events always originate from the target element (react owned) whereas the other events originate from the relatedTarget, which may be not be react-owned

also did we end up some place on using the native event vs continuing to polyfill this forever?

Yeah. Still I can't help but wonder if there's work that could be done here to do the attachment local to the event plugin. It's a bummer that ReactBrowserEventEmitter and EnterLeavePlugin are coupled like this.

This is a similar problem for change events on inputs. We have to attach change events to controlled inputs even if there isn't an event listener. This is so that controlled inputs that don't have readOnly prevent value changes on user input.

IMO, we should warn users when they don't have a change event and tell them to use readOnly, but we might already do that. Or, at the very least, we should attach the event listeners inside of the input wrappers (input, select, and textarea).

also did we end up some place on using the native event vs continuing to polyfill this forever?

Let's give it a look. It looks like we might be able to use the native event now, but the support matrix for mouseleave on MDN is incomplete.

We should do this research, kill the event plugin if we can, which I think would also let us get rid of some tree traversal methods specific to this event.

At the very least we should update all of those question marks on MDN.

I did the basic work checking for support, and we definitely can use the native event, the problem is Portals now.

naive approach: #10247
hack with portal support: #10269

In terms of moving attachment to the plugin, there used to be (still are?) hooks on the plugins that would get called for this sort of thing, something like willAttachListener they may have been removed tho since i can't seem to find them...

jquense · 2017-11-15T14:15:48Z

packages/react-dom/src/events/ReactBrowserEventEmitter.js

+        trapBubbledEvent(
+          dependency,
+          topLevelTypes[dependency],
+          documentForRoot(root),


isn't root undefined here?

🙈. I'll look into this :)

Fixed in 68b42c2

nhunzaker · 2017-11-16T01:17:15Z

Looks like we can avoid the work of the event support checks by moving them over to the BrowserEventConstants module. Woo!

gaearon · 2018-01-05T17:02:09Z

I'll close as it's my understanding this was only a proof of concept. If there are valuable lessons here please create an issue so that we can track the future work.

nhunzaker · 2018-01-06T15:19:42Z

No worries. Quite a few things have already made it to master because of this, and some of the scroll event listener optimization work was impacted by it.

Having said that, I do think it is worth exploring using captured events for all event listening. I can follow up with an issue for that.

jquense · 2018-01-06T15:36:20Z

i'm trying out using local capture for all events in RDL, i'm interested to see how it works...

Another possible win could be if we removed the event whitelist completely and just attached whatever the user wanted. you'd lose a lot of config if we could do that

gaearon · 2018-01-06T16:14:36Z

if we removed the event whitelist completely and just attached whatever the user wanted

Yeah I'm very interested in seeing this explored.

jquense · 2018-01-06T17:02:48Z

I'll give it a shot!

Switch out event delegation for most events

0fcbfbd

This commit replaces the traditional event delegation technique employed by React with direct attachment to DOM nodes. This is a test case to evaluate performance differences.

facebook-github-bot added the CLA Signed label Nov 14, 2017

nhunzaker added 2 commits November 13, 2017 21:13

Copy over production builds to fixtures during build

fa2501f

Remove unused imports in ReactDOMFiberComponent

ca75e2e

nhunzaker force-pushed the local-listeners branch from 0074530 to ca75e2e Compare November 14, 2017 02:37

Revert ReactBrowserEventEmitter-test.js

cd44f7a

nhunzaker mentioned this pull request Nov 15, 2017

Set correct event type into SimulateNative methods #11563

Closed

jquense reviewed Nov 15, 2017

View reviewed changes

This was referenced Nov 16, 2017

Local listeners #11572

Closed

Move MouseWheel event type detection to BrowserEventConstants #11585

Closed

Investigate need to feature check cancel and close events. #11598

Closed

Remove IE8 event.target polyfill via srcElement #11515

Merged

gaearon closed this Jan 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

nhunzaker commented Nov 14, 2017

syranide commented Nov 14, 2017

nhunzaker commented Nov 15, 2017

jquense left a comment

jquense Nov 15, 2017

nhunzaker Nov 16, 2017 •

edited

Loading

nhunzaker Nov 16, 2017

nhunzaker Nov 16, 2017

jquense Nov 20, 2017

jquense Nov 15, 2017

nhunzaker Nov 16, 2017

jquense Nov 20, 2017

jquense Nov 20, 2017

nhunzaker Nov 20, 2017

nhunzaker Nov 20, 2017

nhunzaker Nov 20, 2017

jquense Nov 20, 2017

jquense Nov 15, 2017

nhunzaker Nov 16, 2017

nhunzaker Nov 16, 2017

nhunzaker commented Nov 16, 2017

gaearon commented Jan 5, 2018

nhunzaker commented Jan 6, 2018

jquense commented Jan 6, 2018

gaearon commented Jan 6, 2018

jquense commented Jan 6, 2018

DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

DO NOT MERGE: Investigate switching out event delegation for direct attachment (mostly) #11550

Conversation

nhunzaker commented Nov 14, 2017

syranide commented Nov 14, 2017

nhunzaker commented Nov 15, 2017

jquense left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker Nov 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Nov 16, 2017

gaearon commented Jan 5, 2018

nhunzaker commented Jan 6, 2018

jquense commented Jan 6, 2018

gaearon commented Jan 6, 2018

jquense commented Jan 6, 2018

nhunzaker Nov 16, 2017 •

edited

Loading