Remove event pooling #13613
Remove event pooling #13613
Conversation
(instead of development build, which remains the default)
|
ReactDOM: size: -0.7%, gzip: -0.7% Details of bundled changes.Comparing: f6fb03e...6dc4a99 react-dom
react-native-renderer
schedule
Generated by 🚫 dangerJS |
# Conflicts: # fixtures/dom/.gitignore # fixtures/dom/package.json # fixtures/dom/src/react-loader.js
poeschko
force-pushed
the
remove-event-pooling
branch
from
b43494b
to
6dc4a99
Compare
|
Awesome Jan, thanks for filing the PR 🙌. As I've already told you yesterday it might take a while until we have all the edge cases ironed out and can merge this - and there's a possibility that this will never work out. I hope this doesn't demotivate you to continue working on it. 🙂 I had this idea for a while and I'm glad we have a starting point for the discussion now. Ideally we can also get rid of the event property lists as well after removing pooling - So that we only end up adding the shims but remove properties with no change on the React side. One idea to achieve this is to use a prototype linked object like this this: let synthethicEvent = Object.create(nativeEvent);
synthethicEvent.stopPropagation = () => ...;But let's start with removing pooling for now. I think the next step is to get an understanding of the performance characteristics of this change. I'm having two different steps in mind that should give us an idea wether the event pooling added a significant difference to the event system or not:
Garbage collection during the events would lead to notable pauses so we also need to find out when the GC is triggered - Maybe we can compare this as well? It would be great if you can start working on this and post the devtools screenshots here so we get an idea. We want to test this for production but also for development builds so we don't regress anything. Feel free to reach out if you have questions or need help! Oh and we should probably add this behind the 🔥 React Fire feature flag 🙂 (#13554). |
|
I'm a bit stuck investigating the build failure during To debug this, I ended up adding some I don't know what this is caused by... Since it only happens for the production build, I guess it has something to do with JS minification. |
|
We were thinking it might be more practical to start from scratch instead of simplifying the existing event system. |
|
i'd looooove to try that ;) |
|
Understood – a bit sad but also exciting! So I'll stop pursueing this for now. Feel free to close this PR if it's not needed. Happy to help with something else. |
Oooh nice. |
|
I'll write something up in the next few days. |
|
Should I extract my changes to the fixtures GUI (so you can choose the production build of React) into a separate pull request? I found it very useful while testing the event pooling changes, but it's probably useful in other cases as well. |
|
@poeschko I think a toggle to switch between development and production React would be awesome. |
|
@poeschko If you're still interested in landing the DOM fixture change, take a look at this commit. It's basically your changes cherry picked onto the latest master plus a fix for React versions <16. I've also deployed it for you all to test it 🙂 |
|
Thanks @philipp-spiess! I opened #13786 with just that commit. Closing this pull request. |
I started working on this idea by @philipp-spiess during a joint programming session at the React Vienna meetup. It's my first pull request for React, so please go easy on me. :-)
The idea is to get rid of the event object pooling mechanism and instead just create new instances whenever needed. I kept the methods
persist(issueing a warning for now) andisPersistent(just always returningtruenow) onSyntheticEventsince at least persist is documented. That whole section in the documention would still have to be updated (or removed).Main benefits:
react-dom.production.min.js: 92.2 KB -> 91.54 KB, gzipped 30.01 KB -> 29.81 KB; that's -0.7%)I removed quite a few tests that were mostly concerned with checking the persistence of events, since that's not really needed anymore. Instead, I added a single test that checks the result from
isPersistentbeingtrueand thatpersistissues a warning (which would still need to be finalized, if we want it at all).The hope is that modern JS engines are optimizing object creation & release enough so that not maintaining a pool does not make a significant difference. (Longer-term, maybe JS engines could make this even faster than the pool implemented in user land?)
So far, I quickly tested the performance using the "Event Pooling" fixture in Mac Chrome 69 (moving the mouse around for a few seconds, creating a few hundred events) and did not notice a significant effect, certainly not visually and also not when glancing at Chrome's Performance flamechart (also tried with 6x CPU slowdown); I did not notice significant garbage collection activity either. But these are just some very preliminary observations (in a rather simple scenario), so the performance side would still need to be analyzed much more carefully, also across different operating systems and browsers.
To make the quick performance check using the fixture UI a bit more meaningful, I added a checkbox to choose the React production build instead of the development build (which remains the default). Please let me know if this should rather go into a separate PR (or if it's not desired at all).
This would take care of #13224 (where the idea of getting rid of object pooling is also mentioned).
I realize that this is quite a big change, so maybe it should have started as an RFC or so. But even if there's more discussion needed, I guess having the code along with it doesn't hurt. Just let me know and I could continue trying to measure the performance impact more carefully (suggestions welcome). But no hard feelings if this is rejected for whatever reason.
Thanks to @philipp-spiess for the introduction to the React code base and kicking off this idea!