Don't add top-level events for uncontrolled inputs

[nathansobo]

The document-wide handlers for native events that perform synthetic event dispatch execute in 0.2-0.7 milliseconds on my machine. I realize this doesn't seem like much time, but we're really trying to shave off any non-essential source of latency for typing and cursor movement in Atom, and every little bit helps. Disabling synthetic events on keydown and textinput is saving about 1ms of latency for a keystroke.

In our fork, I've added the ability to add a reactSkipEventDispatch property to the native event to opt out of synthetic dispatch, but I'd be interested in a more official mechanism for opting out of this feature for certain event types. Even better, perhaps React could maintain a cache of what event types are actually being listened for and bail out as soon as possible if handling an event.

[sophiebits]

React should only attach top-level handlers for events that you're using in your components – are you seeing otherwise?

[syranide]

@nathansobo I'm curious whether it is the simulated dispatcher (i.e, capturing, bubbling, etc) or the initialization of the synthetic event that is most costly for you.

I assume it is the simulated dispatcher, which currently does a lot of ugly string hacking to do its thing (chenglou is working on that I believe). While the SyntheticKeyboardEvent has a lot of polyfills, they're all very simple and I don't imagine that they're the issue (but perhaps they are... because browsers).

[syranide]

Also, you can easily opt-out of React's event dispatcher by just not using it and calling addEventListener from inside componentDidMount instead. You pay a slight up-front cost though.

PS. I imagine that since you're operating in a single modern and fast environment, React's entire event dispatcher is quite redundant for you, you could probably just replace it internally with immediate calls to addEventListener.

[nathansobo]

I'm as sure as I can be that we're not attaching event handlers for keydown and textInput events via React properties anywhere in our code. Are they perhaps attached by the input DOM component?

Here's a flame chart of a keystroke, with keydown and textInput events being processed by React.

Upon consideration we should probably just stopPropagation of these events and they'll never reach the handler. But for some reason they are indeed being handled in a non-trivial amount of time.

I'm quite intrigued by your idea of just using native APIs directly and skipping the global handling. I'm assuming that would require some more logic in our fork to interpret the event handler properties of DOM components?

[syranide]

@nathansobo Ah, that explains it. Yeah, React.DOM.input (overloaded by ReactDOMInput) always attaches an onChange handler (does not behave like HTML onchange), which is implemented by ChangeEventPlugin and listens to quite a number of events. If you're using controlled inputs it's obviously required, but it seems like it should be possible to discard it for uncontrolled inputs, but I'm not 100% sure without further examination of the code.

So if you're using uncontrolled inputs, it's probably something we should have a look at to see if it can avoided. If you're using controlled inputs, your best bet is probably to implement your own controlled inputs on-top of uncontrolled inputs (again, we should have a look at if the change handler can be avoided for them). However, you can remove ReactDOMInput from ReactDefaultInjection right now, and you get the raw input component in React.DOM.input instead, which you could use to implement controlled inputs using native events if you want.

As for using native APIs directly, I'm not super familiar with the event system code in React. But apart from onChange (which has a different HTML5 equivalent I believe), all the events represent the equivalent HTML5 events. So simply discarding/ignoring the entire synthetic React event system and updating ReactDOMComponent as necessary (look for putListener, deleteListener, etc, perhaps you could just update those instead...) to add native event listeners directly should be a piece of cake (and not a hack). Again, you pay a higher up-front cost for each event listener though, but that may be preferable in your case.

(You probably want to remove ReactDOMInput, etc, regardless of if you decide to switch ReactDOM to native events or not)

[nathansobo]

Thanks for your thoughtful explanation here. Yeah, this input definitely uncontrolled. It's not even visible, but just exists to receive input which we render ourselves in the editor. For now, stopping propagation of keydown and textInput events seems to eliminate the expensive handling from the flame graph.

I'm hesitant to remove controlled inputs entirely in case users want to use them elsewhere. For now we're exporting our copy of React to users until the multiple-instance issue is addressed, so I'd like inputs to work as expected for them.

Not sure disabling the synthetic event system is warranted yet, but if we see it costing us time elsewhere I'll definitely take a crack at it.

I'm going to close this now. Thanks for the conversation!

[sophiebits]

Pretty sure we don't need to attach the event handlers for uncontrolled components. I'll check and let you know.

[syranide]

@nathansobo If you're stopping propagation of textinput and keydown then you have not removed controlled inputs, you've broken them instead. (EDIT: Or perhaps that's not true now that I read it again, as it seems you're preventing the events further up the tree) You can very easily reimplement controlled inputs with the raw input component, if that's something you want to expose for users (but don't want to replace the synthetic event system entirely)... so no real harm done.

That being said, the synthetic event system in React exists for two reasons AFAIK:

1. Normalize browser inconsistencies (lots of them), especially old browsers.
2. Drop up-front O(n) addEventListener cost, for slightly more expensive event handling.

1 should be irrelevant for you, and 2 is probably not as important for you as you're in a faster modern environment. Also, dropping frames is far more noticeable (as you say) than a popup taking a few frames longer to open in worst-case. Just food for thought.

I definitely see Atom becoming my one-and-only editor in the future, so I'm just happy to help. :)

[sophiebits]

Just made #1968 with a possible fix, though I'm unsure how well I like it.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!