Remove event pooling in the modern system #18216
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
React Core Team
gaearon
commented
Mar 5, 2020
| const Interface = this.constructor.Interface; | ||
| for (const propName in Interface) { | ||
| // Modern event system doesn't use pooling. | ||
| if (!enableModernEventSystem) { |
There was a problem hiding this comment.
Changes below are just whitespace.
|
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 029f99a:
|
Details of bundled changes.Comparing: 2fe0fbb...029f99a react-dom
react-native-renderer
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Details of bundled changes.Comparing: 2fe0fbb...029f99a react-native-renderer
react-dom
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
This is the wrong intuition and I think we should probably wait. There are a ton of things that we should be doing that we're not doing and we really need to hold the line hard. If we were to introduce breaking changes there are a number of other that would be higher pri than this. Any small breaking change adds to the risk that people can't upgrade to the next major. If they can't upgrade to the next major they can't isolate a small subsection of their site. If they can't do that, they won't be able to adopt the next major after that at all. All the breaking changes in the next major should only be in service of unlocking the ability to pin a subtree to an old version which is the point of the "modern system". I'd even go as far as saying we shouldn't enable new warnings in 17.0. Only in a future minor. |
|
Behind a different flag that we can do it next one would be make sense though. |
trueadm
reviewed
Mar 5, 2020
| @@ -152,71 +154,79 @@ Object.assign(SyntheticEvent.prototype, { | |||
| * won't be added back into the pool. | |||
| */ | |||
| persist: function() { | |||
| this.isPersistent = functionThatReturnsTrue; | |||
| // Modern event system doesn't use pooling. | |||
| if (!enableModernEventSystem) { | |||
There was a problem hiding this comment.
We'd probably want to add a warning on using persist(), as it'll just be dead code in apps otherwise.
trueadm
suggested changes
Mar 5, 2020
|
I wonder if we can go further and remove the heavy synthetic event system that requires all property names to be hardcoded so we can copy them to a small object that only sets the target and prototype links to the nativeEvent? |
|
@philipp-spiess This part of Seb's comment probably applies here:
|
|
I made a follow up to this PR in #18969. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a proposal.
Event pooling saves allocations during high firing events, but adds a bit overhead in "releasing", "destroying" and "reusing" instances. I'm not sure this is actually beneficial. Nobody else is doing it so it's not like this is a common practice. I think maybe we can experiment with dropping it altogether. We didn't do this before because it's technically a breaking change, but since we're changing a bunch of things about events anyway, maybe we can sneak it in? It's less of a chance than the move to roots, for example.
Pooling is confusing. For example, not being to access
e.targetin thesetStateupdater has been a pain. Although maybe reading that early is best practice so you capture the right value etc. But the way it currently fails is very unintuitive. So maybe it's worth fixing even if we add a lint warning against that. I think the conceptual simplicity win is still valuable.Effectively this moves all pooling related code behind a flag. I kept
getPooledbecause it's scattered across the codebase, but in modern mode this just calls the constructor immediately. I also kept the public API, butpersistis a no-op andisPersistentalways returns true in modern mode.